connect: also update offset for features without values

parse_feature_value() takes an offset, and uses it to seek past the
point in features_list that we've already seen. However if the feature
being searched for does not specify a value, the offset is not
updated. Therefore if we call parse_feature_value() in a loop on a
value-less feature, we'll keep on parsing the same feature over and over
again. 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 <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/connect.c b/connect.c
index 70b13389ba5f706045b2b77114fa6f4797b58d28..71e8757e24ddbab1f69c073cef429f8bd5d3f590 100644 (file)
--- a/connect.c
+++ b/connect.c
@@ -555,6 +555,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") */

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 5c941949b980b49607fb9b3876487110cf0acabc..bc393d7c31939f6e592815614521741c3e8dc087 100755 (executable)
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -32,4 +32,19 @@ 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) &&
+       dst=refs/heads/foo &&
+       {
+               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 --symref --upload-pack="cat input; read junk;:" . >actual &&
+       printf "ref: %s\tHEAD\n%s\tHEAD\n" "$dst" "$oid" >expect &&
+       test_cmp expect actual
+'
+
 test_done