Re: [PATCH] fetch-pack: fix unadvertised requests validation

From: Jeff King <peff@peff.net>
Date: Feb 27, 2016, 19:07 UTC
Message-ID: <20160227190712.GC12822@sigill.intra.peff.net>
In-Reply-To: <xmqqd1riggd7.fsf@gitster.mtv.corp.google.com>

On Sat, Feb 27, 2016 at 10:25:40AM -0800, Junio C Hamano wrote:

Show 22 quoted lines

> Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:
> 
> > Check was introduced in b791642 (filter_ref: avoid overwriting
> > ref->old_sha1 with garbage, 2015-03-19), but was always false because
> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
> >
> > Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> > ---
> 
> Peff, that commit points me at your direction.  And I can see the
> original patch avoids overwriting old_sha1 by saving the result from
> get_sha1_hex() in a temporary, it is true that old_sha1 is not
> updated from the temporary.
> 
> The original code before b791642 wanted to say "if ref->name is not
> 40-hex, continue, and otherwise, do the ref->matched thing" and an
> implementation of b791642 that is more faithful to the original
> would indeed have been the result of applying this patch from
> Gabriel, but I am scratching my head why we have hashcmp() there.
> 
> Was it to avoid adding the same thing twice to the resulting list,
> or something?

It is a sanity check. The code is looking in our list of things to fetch for items that are pure objects, not refs (we already matched the refs by name, but obviously would not have matched any pure-sha1 requests to refnames). So the conditional really is:

   if (!is_a_pure_sha1(ref))
	continue;

We can implement that as:

  if (get_sha1_hex(ref->name, sha1) || ref->name[40] != '\0')

but as noted in the commit message for b791642:

  We could just check that we have exactly 40 characters of
  sha1. But let's be even more careful and make sure that we
  have a 40-char hex refname that matches what is in old_sha1.
  This is perhaps overly defensive, but spells out our
  assumptions clearly.

E.g., if you did this:

  git fetch-pack --stdin $remote <<\EOF
  1234567890123456789012345678901234567890 abcdef1234abcdef1234abcdef1234abcdef1234
  EOF

you'd have a "struct ref" with a 40-hex sha1, but which does _not_ want the object of the same name. This is not a pure-object request, and we should only request 1234... if the ref abcd... is present on the remote.

I doubt it would ever come up in real life; refs tend to start with "refs/", and I suspect short of manual prodding as above, you could not get anything without "refs/" to this point of the code.

So the patch:

Show 18 quoted lines

> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 01e34b6..83b937b 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,
> >  			if (ref->matched)
> >  				continue;
> >  			if (get_sha1_hex(ref->name, sha1) ||
> > -			    ref->name[40] != '\0' ||
> > -			    hashcmp(sha1, ref->old_oid.hash))
> > +			    ref->name[40] != '\0')
> >  				continue;
> >  
> >  			ref->matched = 1;
> > +			hashcpy(ref->old_oid.hash, sha1);
> >  			*newtail = copy_ref(ref);
> >  			newtail = &(*newtail)->next;
> >  		}

is a wrong direction, I think. It removes the extra safety check that skips the ref above. But worse, in the example above, it overwrites the real object "1234..." with the name of the ref "abcd..." in the sha1 field. We'll ask for an object that may not even exist.

The commit message for Gabriel's patch says:

> > Check was introduced in b791642 (filter_ref: avoid overwriting
> > ref->old_sha1 with garbage, 2015-03-19), but was always false because
> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.

but I don't think ref->old_oid.hash _is_ empty. At least, that was not the conclusion from our discussion in:

   http://thread.gmane.org/gmane.comp.version-control.git/265480

We expect whoever creates the "sought" list to fill in the name and sha1 as appropriate. If that is not happening in some code path, then yeah, filter_refs() will not work as intended. But I think the solution there would be to fix the caller to set up the "struct ref" more completely.

Gabriel, did this come from a bug you noticed in practice, or was it just an intended cleanup?

-Peff