[PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Pierre Habouzit]

aka don't do pointer arithmetics on structs that have a FLEX_ARRAY member,
or you'll end up believing your array is 1 cell off its real address.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 diff-delta.c |    2 +-
 1 files changed, 1 insertions(+), 1 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;
-- 
1.5.4.rc0.1153.gb1b3d-dirty

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Tue, Dec 18, 2007 at 01:39:57AM +0000, Pierre Habouzit wrote:
> aka don't do pointer arithmetics on structs that have a FLEX_ARRAY member,
> or you'll end up believing your array is 1 cell off its real address.

  I wonder if we could teach sparse to prevent us from using pointer
arithmetics on some types… because I obviously didn't read all the git
code, and I wouldn't be surprised an instance of this still remains
somehwere.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Linus Torvalds]

On Tue, 18 Dec 2007, Pierre Habouzit wrote:
> 
>   I wonder if we could teach sparse to prevent us from using pointer
> arithmetics on some types… because I obviously didn't read all the git
> code, and I wouldn't be surprised an instance of this still remains
> somehwere.

This should do it.

What this does is:
 - make flex structures not have a size at all (so "sizeof()" will fail)
 - add warnings for trying to add or subtract unsized pointers

so now you can try it on git with

	make CC=cgcc

and while it finds a fair number of "sizeof(..)" things and complains 
about them, the only invalid pointer arithmetic it finds is the 

	mem = index + 1;

line in diff-delta.c.

Whether it is worth fixing all the "sizeof()" calls too, I dunno. They 
result in a slight waste of memory (ie we allocate too much memory), but 
I guess they should be harmless.

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. I didn't check them all, 
there's something like 44 complaints like

	builtin-fetch.c:306:21: error: cannot size expression

from sparse with this patch.

		Linus

---
 evaluate.c |    8 ++++++++
 symbol.c   |    2 ++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 54fcd3f..cd816a8 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -576,6 +576,10 @@ static struct symbol *evaluate_ptr_add(struct expression *expr, struct symbol *i
 		expression_error(expr, "arithmetics on pointers to functions");
 		return NULL;
 	}
+	if (base->bit_size & 7) {
+		expression_error(expr, "arithmetic on unsized pointers");
+		return NULL;
+	}
 
 	/* Get the size of whatever the pointer points to */
 	multiply = base->bit_size >> 3;
@@ -820,6 +824,10 @@ static struct symbol *evaluate_ptr_sub(struct expression *expr)
 		expression_error(expr, "subtraction of functions? Share your drugs");
 		return NULL;
 	}
+	if (lbase->bit_size & 7) {
+		expression_error(expr, "subtracting unsized pointers");
+		return NULL;
+	}
 
 	expr->ctype = ssize_t_ctype;
 	if (lbase->bit_size > bits_in_char) {
diff --git a/symbol.c b/symbol.c
index 7539817..8b390ac 100644
--- a/symbol.c
+++ b/symbol.c
@@ -124,8 +124,10 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 	 * structure size
 	 */
 	if (base_size < 0) {
+		info->bit_size = -1;
 		info->align_size = 0;
 		base_size = 0;
+		return;
 	}
 
 	align_bit_mask = (sym->ctype.alignment << 3) - 1;

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Linus Torvalds]

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);

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> 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));

This is harmless and in fact unnecessary clearing, immediately before
calling free(3).

>  - diff-delta.c:250        memsize = sizeof(*index)

I haven't studied this codepath.

>  - object-refs.c:23        size_t size = sizeof(*refs) + count*sizeof(struct object *);

Overallocation to have at least "count" pointers to "struct object".

>  - object-refs.c:61        size_t size = sizeof(*refs) + j*sizeof(struct object *);

Ditto for "j" pointers.

>  - attr.c:220              sizeof(*res) +

Overallocation to have at least "num_attr" instances of "struct
attr_state" (plus name string if needed, which is stored using location
past state[num_attr]).

>  - remote.c:467            memset(ret, 0, sizeof(struct ref) + namelen);

Clearing an arena that was overallocated to have at least namelen
elements of char[] on the line immediately before this, with matching
size.  All callers pass namelen = strlen(name) + 1 so we are Ok even
when FLEX_ARRAY gives no extra space.

>  - remote.c:474            memcpy(ret, ref, sizeof(struct ref) + strlen(ref->name) + 1);

Ditto.

>  - transport.c:491         memset(ref, 0, sizeof(struct ref));

A line above overallocates to have enough room for strlen(ref_name) plus
terminating NUL, and after this lines clears the non-flex part, name is
copied.  So this overclears a bit, but is harmless.

> 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);

Portability of offsetof() is slightly worrisome, but giving a
compatibility macro is trivial if this turns out to be problematic for
some people.

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>>  - diff-delta.c:250        memsize = sizeof(*index)
>
> I haven't studied this codepath.

My proposed patch should have addressed this as well.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Fix segfault in diff-delta.c when FLEX_ARRAY is 1

[Nicolas Pitre]

On Tue, 18 Dec 2007, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > 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):
> >
> >  - diff-delta.c:250        memsize = sizeof(*index)
> 
> I haven't studied this codepath.

Harmless overallocation.  Nothing to worry about.


Nicolas