Re: [PATCH] connect: also update offset for features without values

From: Jeff King <peff@peff.net>
Date: Sep 18, 2021, 22:05 UTC
Message-ID: <YUZinXsGdL19l/tQ@coredump.intra.peff.net>
In-Reply-To: <YUYLXKN8U9AMa5ke@nand.local>

On Sat, Sep 18, 2021 at 11:53:00AM -0400, Taylor Blau wrote:

Show 11 quoted lines

> > +test_expect_success 'bogus symref in v0 capabilities' '
> > +	test_commit foo &&
> > +	oid=$(git rev-parse HEAD) &&
> > +	{
> > +		printf "%s HEAD\0symref object-format=%s\n" "$oid" "$GIT_DEFAULT_HASH" |
> > +			test-tool pkt-line pack-raw-stdin &&
> 
> I'm actually really happy with this modification to add the non-empty
> object-format after the broken "symref" part, since it ensures that your
> offset calculation is right (and that we can continue to parse features
> with or without values after a value-less one).

I don't think it quite does that, though. If I understand the parsing code correctly, it walks through the list looking for entries for a _particular_ capability. I.e., it will look for any "symref" entries, advancing the offset counter. And then separately it will start again looking for any object-format entries, with a brand-new offset counter starting at 0.

So if you want to confirm that the parsing continues after the unexpected entry, you'd want a second symref entry, and then to make sure it was correctly parsed. Perhaps something like this:

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 34538cebf0..98d7f4981a 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -35,13 +35,15 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 test_expect_success 'bogus symref in v0 capabilities' '
 	test_commit foo &&
 	oid=$(git rev-parse HEAD) &&
+	dst=refs/heads/foo &&
 	{
-		printf "%s HEAD\0symref object-format=%s\n" "$oid" "$GIT_DEFAULT_HASH" |
+		printf "%s HEAD\0symref object-format=%s symref=HEAD:%s\n" \
+			"$oid" "$GIT_DEFAULT_HASH" "$dst" |
 			test-tool pkt-line pack-raw-stdin &&
 		printf "0000"
 	} >input &&
-	git ls-remote --upload-pack="cat input ;:" . >actual &&
-	printf "%s\tHEAD\n" "$oid" >expect &&
+	git ls-remote --symref --upload-pack="cat input ;:" . >actual &&
+	printf "ref: %s\tHEAD\n%s\tHEAD\n" "$dst" "$oid" >expect &&
 	test_cmp expect actual
 '
 

I don't think it's hugely important (after all, this is something that
the server isn't supposed to send in the first place). But given that we
did make it work correctly (and the original on the security list
didn't), it's not too bad to test it on top.

Swapping out the "printf >expect" for a here-doc might make it a bit
more readable. I used printf because of the tab handling, but:

  tab=$(printf "\t")
  cat >expect <<-EOF
  ref: ${dst}${tab}HEAD
  ${oid}${tab}HEAD
  EOF

isn't too bad.

-Peff