Re: [RESEND PATCH 1/2] Fix builtin-fetch memory corruption by not overstepping array

From: Shawn O. Pearce <spearce@spearce.org>
Date: Sep 14, 2007, 23:14 UTC
Message-ID: <20070914231441.GW3099@spearce.org>
In-Reply-To: <20070914225953.GC16512@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> wrote:

Show 5 quoted lines

> Unfortunately we cannot continue to NULL terminate the array at
> this point as the array may only have been allocated large enough
> to match the input of nr_heads.  If there are no duplicates than
> we would be trying to store NULL into heads[nr_heads] and that may
> be outside of the array.

...

Show 12 quoted lines

> diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
> index b0936cc..2977a94 100644
> --- a/builtin-fetch-pack.c
> +++ b/builtin-fetch-pack.c
> @@ -654,7 +654,6 @@ static int remove_duplicates(int nr_heads, char **heads)
>  			heads[dst] = heads[src];
>  		dst++;
>  	}
> -	heads[dst] = 0;
>  	return dst;
>  }
>

This fortunately does not appear to be a bug in `maint`, `master`, or `next`. It appears to be isolated to the builtin-fetch topic. The builtin-fetch topic changed fetch-pack to be builtin-fetch-pack and in doing so changed things such that the heads array might not have room for this final entry.

In maint/master/next the heads array is actually backed by the argv passed into main by the kernel, and that does have an existing NULL terminator, which the above "heads[dst] = 0" is simply going to overwrite if "heads == nr_heads".

-- 
Shawn.