Re: Bug in fetch-pack.c, please confirm

[Junio C Hamano <gitster@pobox.com>]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> Hi guys,
>
> So I was looking at fetch-pack.c (from master @ 52cae643, but I think  
> it's the same everywhere):
>
> # Lines 626-648
>

49bb805e (Do not ask for objects known to be complete., 2005-10-19)
seems to lose the assignment to local[].

> Something's very wrong here.
>
> It looks to me like the bug was introduced in 49bb805e (Do not ask for  
> objects known to be complete. 2005-10-19).

I read that "lines 626-648", stopped reading your message and did a
blame session myself, and arrived at the same culprit.

The very original code that read from the ref directly here; I do
not think it was an attempt to catch a race condition where
ref->old_sha1 got stale and the ref on the filesystem has a newer
value.  Hence, I think copying from o->sha1 like you did is a fix
that is the most faithful to the original code.

o is lookup_object(remote) which is lookup_object(ref->old_sha1);
that makes o->sha1 always be ref->old_sha1, so either would be fine,
but o->sha1 would be more appropriate in this codeflow.  We grab o,
if it is NULL or if we do not like it, we do something and continue,
otherwise we like that o so grabbing o->sha1 out of it makes the
logic look flowing right, at least to me ;-)

> -- 8< --
> Subject: [PATCH] fetch-pack.c: do not use uninitialized sha1 value
>
> Since 49bb805e (Do not ask for objects known to be complete. 2005-10-19)
> when the read_ref call was replaced with a parse_object call, the
> automatic variable 'local' containing an sha1 has been left uninitialized.
>
> Subsequently in 1baaae5e (Make maximal use of the remote refs, 2005-10-28)
> the parse_object call was replaced with a lookup_object call but still
> the 'local' variable was left uninitialized.
>
> However, it's used as the source to update another sha1 value in the case
> that we already have it and in that case the other ref will end up with
> whatever garbage was in the 'local' variable.
>
> Fix this by removing the 'local' variable and using the value from the
> result of the lookup_object call instead.
>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---
>  fetch-pack.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 655ee642..c0b4d84f 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -621,29 +621,28 @@ static int everything_local(struct fetch_pack_args *args,
>  		}
>  	}
>  
>  	filter_refs(args, refs, sought, nr_sought);
>  
>  	for (retval = 1, ref = *refs; ref ; ref = ref->next) {
>  		const unsigned char *remote = ref->old_sha1;
> -		unsigned char local[20];
>  		struct object *o;
>  
>  		o = lookup_object(remote);
>  		if (!o || !(o->flags & COMPLETE)) {
>  			retval = 0;
>  			if (!args->verbose)
>  				continue;
>  			fprintf(stderr,
>  				"want %s (%s)\n", sha1_to_hex(remote),
>  				ref->name);
>  			continue;
>  		}
>  
> -		hashcpy(ref->new_sha1, local);
> +		hashcpy(ref->new_sha1, o->sha1);
>  		if (!args->verbose)
>  			continue;
>  		fprintf(stderr,
>  			"already have %s (%s)\n", sha1_to_hex(remote),
>  			ref->name);
>  	}
>  	return retval;
> ---