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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2007, 09:07 UTC
Message-ID: <7vwsrc1idm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.0.9999.0712172146070.21557@woody.linux-foundation.org>

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

Show 5 quoted lines

> 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.

Show 13 quoted lines

> 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.