Bug in fetch-pack.c, please confirm

["Kyle J. McKay" <mackyle@gmail.com>]

Hi guys,

So I was looking at fetch-pack.c (from master @ 52cae643, but I think  
it's the same everywhere):

# Lines 626-648

     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);
             if (!args->verbose)
                     continue;
             fprintf(stderr,
                     "already have %s (%s)\n", sha1_to_hex(remote),
                     ref->name);
     }

Peff, weren't you having some issue with want and have and hide refs?

Tell me please how the "local" variable above gets initialized?

It's declared on the stack inside the for loop scope so only  
guaranteed to have garbage.

It's passed to hashcpy which has this prototype:

  inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src);

So it looks to me like garbage is copied into rev->new_sha1, yes?

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've taken a stab a a fix below.

-Kyle

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