Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1
On Mon, 17 Dec 2007, Linus Torvalds wrote:
Show 5 quoted lines
>
> 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(-)
Show changes to 2 files +2 −2
diff-delta.c, unpack-trees.c
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);
Pierre Habouzit
