git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Bug in fetch-pack.c, please confirm
@ 2015-03-15  6:37 Kyle J. McKay
  2015-03-15  7:27 ` Junio C Hamano
  2015-03-16  1:13 ` Jeff King
  0 siblings, 2 replies; 14+ messages in thread
From: Kyle J. McKay @ 2015-03-15  6:37 UTC (permalink / raw)
  To: Jeff King, Junio C Hamano; +Cc: Git mailing list

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

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-03-19 21:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-03-15  6:37 Bug in fetch-pack.c, please confirm Kyle J. McKay
2015-03-15  7:27 ` Junio C Hamano
2015-03-15  7:30   ` Junio C Hamano
2015-03-15 22:21     ` Kyle J. McKay
2015-03-16  1:13 ` Jeff King
2015-03-19 17:41   ` Junio C Hamano
2015-03-19 18:55     ` Jeff King
2015-03-19 19:01       ` Junio C Hamano
2015-03-19 20:31         ` Jeff King
2015-03-19 20:34           ` [PATCH 1/4] filter_ref: avoid overwriting ref->old_sha1 with garbage Jeff King
2015-03-19 21:09             ` Junio C Hamano
2015-03-19 20:37           ` [PATCH 2/4] filter_ref: make a copy of extra "sought" entries Jeff King
2015-03-19 20:38           ` [PATCH 3/4] fetch_refs_via_pack: free extra copy of refs Jeff King
2015-03-19 20:39           ` [PATCH 4/4] fetch-pack: remove dead assignment to ref->new_sha1 Jeff King

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).