connect: also update offset for features without values

From: Andrzej Hunt <[email protected]>

parse_feature_value() does not update offset if the feature being searched for does not specify a value. A loop that uses parse_feature_value() to find a feature which was specified without a value therefore might never exit (such loops will typically use next_server_feature_value() as opposed to parse_feature_value() itself). This usually isn't an issue: there's no point in using next_server_feature_value() to search for repeated instances of the same capability unless that capability typically specifies a value - but a broken server could send a response that omits the value for a feature even when we are expecting a value.

Therefore we add an offset update calculation for the no-value case, which helps ensure that loops using next_server_feature_value() will always terminate.

next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)

Thanks to Peff for authoring the test.

Co-authored-by Jeff King <[email protected]>

Signed-off-by Jeff King <[email protected]>

Signed-off-by Andrzej Hunt <[email protected]>

---
connect: also update offset for features without values

This is a small patch to avoid an infinite loop which can occur when a broken server forgets to include a value when specifying symref in the capabilities list.

Thanks to Peff for writing the test.

Note: I modified the test by adding and object-format=... to the injected server response, because the oid that we're using is the default hash (which will be e.g. sha256 for some CI jobs), but our protocol handler assumes sha1 unless a different hash has been explicitly specified. I'm open to alternative suggestions.

ATB,

Andrzej

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1
Pull-Request: https://github.com/git/git/pull/1091

 connect.c                      |  2 ++
 t/t5704-protocol-violations.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+)

connect.c +0 -0

index aff13a270e6..eaf7d6d2618 100644

t/t5704-protocol-violations.sh +0 -0

index 5c941949b98..34538cebf01 100755

From: Andrzej Hunt <[email protected]>

parse_feature_value() does not update offset if the feature being searched for does not specify a value. A loop that uses parse_feature_value() to find a feature which was specified without a value therefore might never exit (such loops will typically use next_server_feature_value() as opposed to parse_feature_value() itself). This usually isn't an issue: there's no point in using next_server_feature_value() to search for repeated instances of the same capability unless that capability typically specifies a value - but a broken server could send a response that omits the value for a feature even when we are expecting a value.

Therefore we add an offset update calculation for the no-value case, which helps ensure that loops using next_server_feature_value() will always terminate.

next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)

Thanks to Peff for authoring the test.

Note: I modified the test by adding and object-format=... to the injected server response, because the oid that we're using is the default hash (which will be e.g. sha256 for some CI jobs), but our protocol handler assumes sha1 unless a different hash has been explicitly specified. I'm open to alternative suggestions.

ATB,

Andrzej

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1
Pull-Request: https://github.com/git/git/pull/1091

 connect.c                      |  2 ++
 t/t5704-protocol-violations.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+)

connect.c +0 -0

index aff13a270e6..eaf7d6d2618 100644

Review

Quoted 6 lines

(quoted diff) +6 -0

+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).

Quoted 11 lines · Taylor Blau

Review

Quoted 6 lines

(quoted diff) +6 -0

+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:

[...]

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.

Quoted 10 lines · Jeff King

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.

Or just use q_to_tab():

    q_to_tab >expect <<-EOF
    ref: ${dst}QHEAD
    ${oid}QHEAD
    EOF

However, the typical use-case for q_to_tab() is when we need a leading or trailing TAB character.

When TAB is embedded within the line, we
often just use a literal TAB character; indeed, many tests in the suite do exactly that, so that would be an even simpler option.

Quoted 1 line · Eric Sunshine

Or just use q_to_tab():

Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or something) for this, but it looks like we only define and use it in a few scripts.

Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or something) for this, but it looks like we only define and use it in a few scripts.

Yeah, that'd probably be OK. I usually shy away from embedded tabs because they can cause confusion in editors. But we have them already, and this kind of expected output is not touched all that often.

connect.c +2 -0

index aff13a270e6..eaf7d6d2618 100644
@@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i
 			if (!*value || isspace(*value)) {
 				if (lenp)
 					*lenp = 0;
+				if (offset)
+					*offset = found + len - feature_list;
 				return value;
 			}
 			/* feature with a value (e.g., "agent=git/1.2.3") */

t/t5704-protocol-violations.sh +13 -0

index 5c941949b98..34538cebf01 100755
@@ -32,4 +32,17 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 	test_i18ngrep "expected flush after fetch arguments" err
 '
 
+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 &&
+		printf "0000"
+	} >input &&
+	git ls-remote --upload-pack="cat input ;:" . >actual &&
+	printf "%s\tHEAD\n" "$oid" >expect &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: 186eaaae567db501179c0af0bf89b34cbea02c26

Quoted 15 lines

(quoted diff) +13 -0

+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 &&
+		printf "0000"
+	} >input &&
+	git ls-remote --upload-pack="cat input ;:" . >actual &&
+	printf "%s\tHEAD\n" "$oid" >expect &&
+	test_cmp expect actual
+'
+
 test_done

Quoted 1 line

base-commit: 186eaaae567db501179c0af0bf89b34cbea02c26

I've been seeing an occasional and not-reliably-reproducible test failure from t5704 in 'seen' these days---since this is the only commit that touches t5704, I am suspecting if there is something racy about it, but I am coming up empty after staring at it for a few minutes.

Building 87446480 (connect: also update offset for features without values, 2021-09-18), which is an application of the patch directly on top of v2.33.0, and doing

    $ cd t
    $ while sh t5704-*.sh; do :; done

I can get it fail in a dozen iterations or so when the box is loaded, so it does seem timing dependent.

I think the problem is that our fake upload-pack exits immediately, so ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say "0000" to indicate that it's not interested in fetching anything (in v2, it doesn't bother, since fetching would be a separate request that it just declines to make).

This seems to fix it:

t/t5704-protocol-violations.sh +0 -0

index 34538cebf0..0983c2b507 100755

Quoted 5 lines · Jeff King

I think the problem is that our fake upload-pack exits immediately, so ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say "0000" to indicate that it's not interested in fetching anything (in v2, it doesn't bother, since fetching would be a separate request that it just declines to make).

Ah, Makes sense---the usual SIGPIPE problem ;-)

Quoted 12 lines

This seems to fix it:

t/t5704-protocol-violations.sh +1 -1

index 34538cebf0..0983c2b507 100755
@@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '
 			test-tool pkt-line pack-raw-stdin &&
 		printf "0000"
 	} >input &&
-	git ls-remote --upload-pack="cat input ;:" . >actual &&
+	git ls-remote --upload-pack="cat input; read junk;:" . >actual &&

t/t5704-protocol-violations.sh test_expect_success 'bogus symref in v0 capabilities' ' · line 46

Quoted 3 lines

t/t5704-protocol-violations.sh +0 -0

 	printf "%s\tHEAD\n" "$oid" >expect &&
 	test_cmp expect actual
 '

Yup. In the original thread there was some further back-and-forth about further improving the test, if I recall correctly; has the issue been settled there, or is everybody happy with the above version?

Quoted 10 lines

parse_feature_value() does not update offset if the feature being searched for does not specify a value. A loop that uses parse_feature_value() to find a feature which was specified without a value therefore might never exit (such loops will typically use next_server_feature_value() as opposed to parse_feature_value() itself). This usually isn't an issue: there's no point in using next_server_feature_value() to search for repeated instances of the same capability unless that capability typically specifies a value - but a broken server could send a response that omits the value for a feature even when we are expecting a value.

It may be worth adding a little detail here. parse_feature_value takes an offset, and uses it to seek past the point in features_list that we've already seen. But if we get a value-less feature, then offset is never updated, and we'll keep parsing the same thing over and over in a loop.

(I know that you know all of that, but I think it is worth spelling out a little more clearly in the patch message).

Quoted 8 lines

Therefore we add an offset update calculation for the no-value case, which helps ensure that loops using next_server_feature_value() will always terminate.

next_server_feature_value(), and the offset calculation, were first
added in 2.28 in:
2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)

This line wrapping is a little odd, but not a big deal.

Quoted 9 lines · Junio C Hamano

Quoted 5 lines · Jeff King

I think the problem is that our fake upload-pack exits immediately, so ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say "0000" to indicate that it's not interested in fetching anything (in v2, it doesn't bother, since fetching would be a separate request that it just declines to make).

Ah, Makes sense---the usual SIGPIPE problem ;-)

Yes, though it definitely took some head-scratching for me to see where it was. ;)

Doing: "./t5704-* --stress" made it pretty clear. It fails almost immediately, and mentions SIGPIPE (well, exit code 141, but by now I have that one memorized).

Quoted 12 lines

Quoted 12 lines

This seems to fix it:

t/t5704-protocol-violations.sh +1 -1

index 34538cebf0..0983c2b507 100755
@@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '
  			test-tool pkt-line pack-raw-stdin &&
  		printf "0000"
  	} >input &&
-	git ls-remote --upload-pack="cat input ;:" . >actual &&
+	git ls-remote --upload-pack="cat input; read junk;:" . >actual &&

Quoted 8 lines

t/t5704-protocol-violations.sh test_expect_success 'extra delim packet in v2 fetch args' ' · line 48

Quoted 3 lines

t/t5704-protocol-violations.sh +0 -0

  	printf "%s\tHEAD\n" "$oid" >expect &&
  	test_cmp expect actual
  '

Yup. In the original thread there was some further back-and-forth about further improving the test, if I recall correctly; has the issue been settled there, or is everybody happy with the above version?

I think the change I showed earlier (to use ls-remote --symref) is worth doing. There was lots of discussion about how to format a tab, but in the end I don't think it really matters.

So here's that patch again, with this race fix on top, which could be squashed in, and then I hope we can call it good.

t/t5704-protocol-violations.sh +5 -3

index 34538cebf0..bc393d7c31 100755
@@ -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; read junk;:" . >actual &&
+	printf "ref: %s\tHEAD\n%s\tHEAD\n" "$dst" "$oid" >expect &&
  	test_cmp expect actual
  '
 

connect: also update offset for features without values

V2 incorporates Peff's test and test stability improvements, and attempts to improve the commit message.