On Mon, 17 Dec 2007, Linus Torvalds wrote:
>
> However, one indication that there may still be something wrong is that if
> you re-make git with FLEX_ARRAY set to some big insane value (say, 1234),
> then git will still fail the test-suite. So maybe there's a "sizeof()"
> that isn't just used for allocation sizes.
No, that's a different thing. In at least unpack-trees.c (line 593), we do
..
if (same(old, merge)) {
*merge = *old;
} else {
..
and that "merge" is a cache_entry pointer. If we have a non-zero
FLEX_ARRAY size, it will cause us to copy the first few bytes of the name
too.
That is technically wrong even for FLEX_ARRAY being 1, but you'll never
notice, since the names are always at least one byte, and the filenames
should basically always be the same. But if we do the same thing for a
rename, we'd be screwed.
So we probably should look out for those things too. A quick hack to
sparse shows that that was the only such occurrence in the current tree,
though.
Anyway, with this patch to current git, it passes all the test-suite with
FLEX_ARRAY artificially set to 123, and the only complaints from my
hacked-up sparse are about "sizeof()" calls (ie no pointer arithmetic, and
no assignments to flex-array-structures).
*Most* of the remaining sizeof() uses, in turn, are allocation-size
related, ie passed in to calloc() and friends, and while I didn't check
them all, such uses of sizeof() is fine (even if it causes some
unnecessarily big allocations).
But there's a few that aren't obviously allocations (this is a list done
with grep and sparse, I didn't look at whether the values used are then
all allocation-related):
- builtin-blame.c:128 memset(o, 0, sizeof(*o));
- diff-delta.c:250 memsize = sizeof(*index)
- object-refs.c:23 size_t size = sizeof(*refs) + count*sizeof(struct object *);
- object-refs.c:61 size_t size = sizeof(*refs) + j*sizeof(struct object *);
- attr.c:220 sizeof(*res) +
- remote.c:467 memset(ret, 0, sizeof(struct ref) + namelen);
- remote.c:474 memcpy(ret, ref, sizeof(struct ref) + strlen(ref->name) + 1);
- transport.c:491 memset(ref, 0, sizeof(struct ref));
maybe somebody else can check them out (I've not sent out the hacky patch
to 'sparse' that found that assignment in unpack-trees.c, but since that
one was the only one it found, nobody should care - and it really was
pretty hacky).
Linus
---
diff-delta.c | 2 +-
unpack-trees.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index 9e440a9..601b49e 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -264,7 +264,7 @@ struct delta_index * create_delta_index(const void *buf, unsigned long bufsize)
index->src_size = bufsize;
index->hash_mask = hmask;
- mem = index + 1;
+ mem = index->hash;
packed_hash = mem;
mem = packed_hash + (hsize+1);
packed_entry = mem;
diff --git a/unpack-trees.c b/unpack-trees.c
index e9eb795..aa2513e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -590,7 +590,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,
* a match.
*/
if (same(old, merge)) {
- *merge = *old;
+ memcpy(merge, old, offsetof(struct cache_entry, name));
} else {
verify_uptodate(old, o);
invalidate_ce_path(old);
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
