Bug in fetch-pack.c, please confirm

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