{"thread":{"id":"56530","subject":"[PATCH] connect: also update offset for features without values","startedAt":"2021-09-18T13:14:38Z","lastAt":"2021-09-27T19:47:51Z","messageCount":17,"participants":["Andrzej Hunt via GitGitGadget","Taylor Blau","brian m. carlson","Jeff King","Eric Sunshine","Junio C Hamano","Andrzej Hunt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"436303","messageId":"pull.1091.git.git.1631970872884.gitgitgadget@gmail.com","threadId":"56530","inReplyTo":null,"subject":"[PATCH] connect: also update offset for features without values","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-18T13:14:32Z","receivedAt":"2021-09-18T13:14:38Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt <andrzej@ahunt.org>\n\nparse_feature_value() does not update offset if the feature being\nsearched for does not specify a value. A loop that uses\nparse_feature_value() to find a feature which was specified without a\nvalue therefore might never exit (such loops will typically use\nnext_server_feature_value() as opposed to parse_feature_value() itself).\nThis usually isn't an issue: there's no point in using\nnext_server_feature_value() to search for repeated instances of the same\ncapability unless that capability typically specifies a value - but a\nbroken server could send a response that omits the value for a feature\neven when we are expecting a value.\n\nTherefore we add an offset update calculation for the no-value case,\nwhich helps ensure that loops using next_server_feature_value() will\nalways terminate.\n\nnext_server_feature_value(), and the offset calculation, were first\nadded in 2.28 in:\n  2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)\n\nThanks to Peff for authoring the test.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Andrzej Hunt <andrzej@ahunt.org>\n---\n    connect: also update offset for features without values\n    \n    This is a small patch to avoid an infinite loop which can occur when a\n    broken server forgets to include a value when specifying symref in the\n    capabilities list.\n    \n    Thanks to Peff for writing the test.\n    \n    Note: I modified the test by adding and object-format=... to the\n    injected server response, because the oid that we're using is the\n    default hash (which will be e.g. sha256 for some CI jobs), but our\n    protocol handler assumes sha1 unless a different hash has been\n    explicitly specified. I'm open to alternative suggestions.\n    \n    ATB,\n    \n    Andrzej\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1\nPull-Request: https://github.com/git/git/pull/1091\n\n connect.c                      |  2 ++\n t/t5704-protocol-violations.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex aff13a270e6..eaf7d6d2618 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\tif (!*value || isspace(*value)) {\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = 0;\n+\t\t\t\tif (offset)\n+\t\t\t\t\t*offset = found + len - feature_list;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 5c941949b98..34538cebf01 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -32,4 +32,17 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \ttest_i18ngrep \"expected flush after fetch arguments\" err\n '\n \n+test_expect_success 'bogus symref in v0 capabilities' '\n+\ttest_commit foo &&\n+\toid=$(git rev-parse HEAD) &&\n+\t{\n+\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n+\t\tprintf \"0000\"\n+\t} >input &&\n+\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n+\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 186eaaae567db501179c0af0bf89b34cbea02c26\n-- \ngitgitgadget\n"},{"id":"436309","messageId":"YUYLXKN8U9AMa5ke@nand.local","threadId":"56530","inReplyTo":"pull.1091.git.git.1631970872884.gitgitgadget@gmail.com","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-18T15:53:00Z","receivedAt":"2021-09-18T15:53:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Andrzej,\n\nOn Sat, Sep 18, 2021 at 01:14:32PM +0000, Andrzej Hunt via GitGitGadget wrote:\n> From: Andrzej Hunt <andrzej@ahunt.org>\n\nThanks for writing this patch. I have seen a copy of this on the\nsecurity list, but the modified version here looks good to me, too. I\nleft a few notes throughout.\n\nRecapping our discussion on the security list, we decided that this\ndidn't merit an embargoed release because a misbehaving server can still\ncause a client to hang if it simply printed half of its ref\nadvertisement. So this issue isn't new, but fixing this instance of it\nis good nonetheless.\n\n> parse_feature_value() does not update offset if the feature being\n> searched for does not specify a value. A loop that uses\n> parse_feature_value() to find a feature which was specified without a\n> value therefore might never exit (such loops will typically use\n> next_server_feature_value() as opposed to parse_feature_value() itself).\n> This usually isn't an issue: there's no point in using\n> next_server_feature_value() to search for repeated instances of the same\n> capability unless that capability typically specifies a value - but a\n> broken server could send a response that omits the value for a feature\n> even when we are expecting a value.\n\nIt may be worth adding a little detail here. parse_feature_value takes\nan offset, and uses it to seek past the point in features_list that\nwe've already seen. But if we get a value-less feature, then offset is\nnever updated, and we'll keep parsing the same thing over and over in a\nloop.\n\n(I know that you know all of that, but I think it is worth spelling out\na little more clearly in the patch message).\n\n> Therefore we add an offset update calculation for the no-value case,\n> which helps ensure that loops using next_server_feature_value() will\n> always terminate.\n\n> next_server_feature_value(), and the offset calculation, were first\n> added in 2.28 in:\n>   2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)\n\nThis line wrapping is a little odd, but not a big deal.\n\n>\n> Thanks to Peff for authoring the test.\n>\n> Co-authored-by: Jeff King <peff@peff.net>\n> Signed-off-by: Jeff King <peff@peff.net>\n> Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>\n> ---\n>     connect: also update offset for features without values\n>\n>     This is a small patch to avoid an infinite loop which can occur when a\n>     broken server forgets to include a value when specifying symref in the\n>     capabilities list.\n>\n>     Thanks to Peff for writing the test.\n>\n>     Note: I modified the test by adding and object-format=... to the\n>     injected server response, because the oid that we're using is the\n>     default hash (which will be e.g. sha256 for some CI jobs), but our\n>     protocol handler assumes sha1 unless a different hash has been\n>     explicitly specified. I'm open to alternative suggestions.\n>\n>     ATB,\n>\n>     Andrzej\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1\n> Pull-Request: https://github.com/git/git/pull/1091\n>\n>  connect.c                      |  2 ++\n>  t/t5704-protocol-violations.sh | 13 +++++++++++++\n>  2 files changed, 15 insertions(+)\n>\n> diff --git a/connect.c b/connect.c\n> index aff13a270e6..eaf7d6d2618 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n>  \t\t\tif (!*value || isspace(*value)) {\n>  \t\t\t\tif (lenp)\n>  \t\t\t\t\t*lenp = 0;\n> +\t\t\t\tif (offset)\n> +\t\t\t\t\t*offset = found + len - feature_list;\n\nThe critical piece :-). Since feature_list is a superset of found, this\nis perfectly safe. It calculates first the offset of the found string\nwithin feature_list, and then adds the length of the feature name.\n\nI would have found this easier to read if it were spelled out as:\n\n    *offset = found - features_list + len;\n\nwhich is the same thing but follows the order of how I spelled out this\nexpression in English. But the way you wrote it matches how\nparse_feature_value() sets the offset when there is a value, so I think\nit's worth being consistent with that.\n\n> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> index 5c941949b98..34538cebf01 100755\n> --- a/t/t5704-protocol-violations.sh\n> +++ b/t/t5704-protocol-violations.sh\n> @@ -32,4 +32,17 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n>  \ttest_i18ngrep \"expected flush after fetch arguments\" err\n>  '\n>\n> +test_expect_success 'bogus symref in v0 capabilities' '\n> +\ttest_commit foo &&\n> +\toid=$(git rev-parse HEAD) &&\n> +\t{\n> +\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n\nI'm actually really happy with this modification to add the non-empty\nobject-format after the broken \"symref\" part, since it ensures that your\noffset calculation is right (and that we can continue to parse features\nwith or without values after a value-less one).\n\n> +\t\tprintf \"0000\"\n> +\t} >input &&\n> +\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> +\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nLooks great to me.\n\nThanks,\nTaylor\n"},{"id":"436315","messageId":"YUYfVM8jMkPG85qT@camp.crustytoothpaste.net","threadId":"56530","inReplyTo":"pull.1091.git.git.1631970872884.gitgitgadget@gmail.com","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2021-09-18T17:18:12Z","receivedAt":"2021-09-18T17:21:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2021-09-18 at 13:14:32, Andrzej Hunt via GitGitGadget wrote:\n> From: Andrzej Hunt <andrzej@ahunt.org>\n> \n> parse_feature_value() does not update offset if the feature being\n> searched for does not specify a value. A loop that uses\n> parse_feature_value() to find a feature which was specified without a\n> value therefore might never exit (such loops will typically use\n> next_server_feature_value() as opposed to parse_feature_value() itself).\n> This usually isn't an issue: there's no point in using\n> next_server_feature_value() to search for repeated instances of the same\n> capability unless that capability typically specifies a value - but a\n> broken server could send a response that omits the value for a feature\n> even when we are expecting a value.\n> \n> Therefore we add an offset update calculation for the no-value case,\n> which helps ensure that loops using next_server_feature_value() will\n> always terminate.\n> \n> next_server_feature_value(), and the offset calculation, were first\n> added in 2.28 in:\n>   2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)\n> \n> Thanks to Peff for authoring the test.\n\nThanks to both of you for the patch and test.\n\n>     Note: I modified the test by adding and object-format=... to the\n>     injected server response, because the oid that we're using is the\n>     default hash (which will be e.g. sha256 for some CI jobs), but our\n>     protocol handler assumes sha1 unless a different hash has been\n>     explicitly specified. I'm open to alternative suggestions.\n\nThis is a fine solution, I think.\n\n>     ATB,\n>     \n>     Andrzej\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v1\n> Pull-Request: https://github.com/git/git/pull/1091\n> \n>  connect.c                      |  2 ++\n>  t/t5704-protocol-violations.sh | 13 +++++++++++++\n>  2 files changed, 15 insertions(+)\n> \n> diff --git a/connect.c b/connect.c\n> index aff13a270e6..eaf7d6d2618 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n>  \t\t\tif (!*value || isspace(*value)) {\n>  \t\t\t\tif (lenp)\n>  \t\t\t\t\t*lenp = 0;\n> +\t\t\t\tif (offset)\n> +\t\t\t\t\t*offset = found + len - feature_list;\n\nYeah, this seems sensible.  A few lines above, we compute the value\noffset (\"value\") as \"found + len\", where \"found\" starts as\n\"feature_list\", so this will be either the space following this value or\nthe NUL byte at the end of the string.  That means that we'll make\nprogress next time because strstr will start searching from that point.\n\n(I'm sure all of this is obvious to you, but I'm just mentioning it to\nensure that my understanding of the code is the same as everyone\nelse's.)\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"436322","messageId":"YUZinXsGdL19l/tQ@coredump.intra.peff.net","threadId":"56530","inReplyTo":"YUYLXKN8U9AMa5ke@nand.local","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-18T22:05:17Z","receivedAt":"2021-09-18T22:05:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 18, 2021 at 11:53:00AM -0400, Taylor Blau wrote:\n\n> > +test_expect_success 'bogus symref in v0 capabilities' '\n> > +\ttest_commit foo &&\n> > +\toid=$(git rev-parse HEAD) &&\n> > +\t{\n> > +\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> > +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n> \n> I'm actually really happy with this modification to add the non-empty\n> object-format after the broken \"symref\" part, since it ensures that your\n> offset calculation is right (and that we can continue to parse features\n> with or without values after a value-less one).\n\nI don't think it quite does that, though. If I understand the parsing\ncode correctly, it walks through the list looking for entries for a\n_particular_ capability. I.e., it will look for any \"symref\" entries,\nadvancing the offset counter. And then separately it will start again\nlooking for any object-format entries, with a brand-new offset counter\nstarting at 0.\n\nSo if you want to confirm that the parsing continues after the\nunexpected entry, you'd want a second symref entry, and then to make\nsure it was correctly parsed.  Perhaps something like this:\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 34538cebf0..98d7f4981a 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -35,13 +35,15 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n test_expect_success 'bogus symref in v0 capabilities' '\n \ttest_commit foo &&\n \toid=$(git rev-parse HEAD) &&\n+\tdst=refs/heads/foo &&\n \t{\n-\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n+\t\tprintf \"%s HEAD\\0symref object-format=%s symref=HEAD:%s\\n\" \\\n+\t\t\t\"$oid\" \"$GIT_DEFAULT_HASH\" \"$dst\" |\n \t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf \"0000\"\n \t} >input &&\n-\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n-\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n+\tgit ls-remote --symref --upload-pack=\"cat input ;:\" . >actual &&\n+\tprintf \"ref: %s\\tHEAD\\n%s\\tHEAD\\n\" \"$dst\" \"$oid\" >expect &&\n \ttest_cmp expect actual\n '\n \n\nI don't think it's hugely important (after all, this is something that\nthe server isn't supposed to send in the first place). But given that we\ndid make it work correctly (and the original on the security list\ndidn't), it's not too bad to test it on top.\n\nSwapping out the \"printf >expect\" for a here-doc might make it a bit\nmore readable. I used printf because of the tab handling, but:\n\n  tab=$(printf \"\\t\")\n  cat >expect <<-EOF\n  ref: ${dst}${tab}HEAD\n  ${oid}${tab}HEAD\n  EOF\n\nisn't too bad.\n\n-Peff\n"},{"id":"436324","messageId":"YUZpwi2HhflICd4Z@nand.local","threadId":"56530","inReplyTo":"YUZinXsGdL19l/tQ@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-18T22:35:46Z","receivedAt":"2021-09-18T22:35:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 18, 2021 at 06:05:17PM -0400, Jeff King wrote:\n> On Sat, Sep 18, 2021 at 11:53:00AM -0400, Taylor Blau wrote:\n>\n> > > +test_expect_success 'bogus symref in v0 capabilities' '\n> > > +\ttest_commit foo &&\n> > > +\toid=$(git rev-parse HEAD) &&\n> > > +\t{\n> > > +\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> > > +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n> >\n> > I'm actually really happy with this modification to add the non-empty\n> > object-format after the broken \"symref\" part, since it ensures that your\n> > offset calculation is right (and that we can continue to parse features\n> > with or without values after a value-less one).\n>\n> I don't think it quite does that, though. If I understand the parsing\n> code correctly, it walks through the list looking for entries for a\n> _particular_ capability. I.e., it will look for any \"symref\" entries,\n> advancing the offset counter. And then separately it will start again\n> looking for any object-format entries, with a brand-new offset counter\n> starting at 0.\n\nAh; you're absolutely right. We call next_server_feature_value from\nannotate_refs_with_symref_info() and server_supports_hash(), each of\nwhich initializes their own offset from zero.\n\n> So if you want to confirm that the parsing continues after the\n> unexpected entry, you'd want a second symref entry, and then to make\n> sure it was correctly parsed.  Perhaps something like this:\n>\n> [...]\n\nYeah, I agree that would exercise it, and I also agree that it isn't\nhugely important. But this patch does make an effort to handle that\ncase, so it's probably worth testing.\n\nThanks,\nTaylor\n"},{"id":"436335","messageId":"CAPig+cSSxgVU47wCNpcW2HTwCA60e1oZ6Yzkb5i-W2HDijq+MQ@mail.gmail.com","threadId":"56530","inReplyTo":"YUZinXsGdL19l/tQ@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-19T01:02:37Z","receivedAt":"2021-09-19T01:04:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 18, 2021 at 6:05 PM Jeff King <peff@peff.net> wrote:\n> Swapping out the \"printf >expect\" for a here-doc might make it a bit\n> more readable. I used printf because of the tab handling, but:\n>\n>   tab=$(printf \"\\t\")\n>   cat >expect <<-EOF\n>   ref: ${dst}${tab}HEAD\n>   ${oid}${tab}HEAD\n>   EOF\n>\n> isn't too bad.\n\nOr just use q_to_tab():\n\n    q_to_tab >expect <<-EOF\n    ref: ${dst}QHEAD\n    ${oid}QHEAD\n    EOF\n\nHowever, the typical use-case for q_to_tab() is when we need a leading\nor trailing TAB character. When TAB is embedded within the line, we\noften just use a literal TAB character; indeed, many tests in the\nsuite do exactly that, so that would be an even simpler option.\n"},{"id":"436345","messageId":"YUaeUuX7aoXtS3jQ@coredump.intra.peff.net","threadId":"56530","inReplyTo":"CAPig+cSSxgVU47wCNpcW2HTwCA60e1oZ6Yzkb5i-W2HDijq+MQ@mail.gmail.com","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-19T02:20:02Z","receivedAt":"2021-09-19T02:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 18, 2021 at 09:02:37PM -0400, Eric Sunshine wrote:\n\n> On Sat, Sep 18, 2021 at 6:05 PM Jeff King <peff@peff.net> wrote:\n> > Swapping out the \"printf >expect\" for a here-doc might make it a bit\n> > more readable. I used printf because of the tab handling, but:\n> >\n> >   tab=$(printf \"\\t\")\n> >   cat >expect <<-EOF\n> >   ref: ${dst}${tab}HEAD\n> >   ${oid}${tab}HEAD\n> >   EOF\n> >\n> > isn't too bad.\n> \n> Or just use q_to_tab():\n> \n>     q_to_tab >expect <<-EOF\n>     ref: ${dst}QHEAD\n>     ${oid}QHEAD\n>     EOF\n> \n> However, the typical use-case for q_to_tab() is when we need a leading\n> or trailing TAB character.\n\nAh, yeah, I forgot we had that. I _thought_ we had a variable ($HT or\nsomething) for this, but it looks like we only define and use it in a\nfew scripts.\n\nI'm not sure using q_to_tab() is all that readable here, because it\nblends into the HEAD token.\n\n> When TAB is embedded within the line, we\n> often just use a literal TAB character; indeed, many tests in the\n> suite do exactly that, so that would be an even simpler option.\n\nYeah, that'd probably be OK. I usually shy away from embedded tabs\nbecause they can cause confusion in editors. But we have them already,\nand this kind of expected output is not touched all that often.\n\n-Peff\n"},{"id":"436348","messageId":"CAPig+cQM+pkorVv=xBaVjJm9n99-8ZnO2hqY7YyrswwcFshaHw@mail.gmail.com","threadId":"56530","inReplyTo":"YUaeUuX7aoXtS3jQ@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-19T02:53:42Z","receivedAt":"2021-09-19T02:53:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 18, 2021 at 10:20 PM Jeff King <peff@peff.net> wrote:\n> On Sat, Sep 18, 2021 at 09:02:37PM -0400, Eric Sunshine wrote:\n> > Or just use q_to_tab():\n>\n> Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or\n> something) for this, but it looks like we only define and use it in a\n> few scripts.\n\nPerhaps you were thinking about the LF or SQ variables defined by t/test-lib.sh?\n"},{"id":"436352","messageId":"xmqq35q1f398.fsf@gitster.g","threadId":"56530","inReplyTo":"YUaeUuX7aoXtS3jQ@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-19T07:12:03Z","receivedAt":"2021-09-19T07:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Ah, yeah, I forgot we had that. I _thought_ we had a variable ($HT or\n> something) for this, but it looks like we only define and use it in a\n> few scripts.\n\nI do not mind seeing a patch that consolidates them to test-lib.sh\nand make HT sit next to SQ and LF.\n\n> Yeah, that'd probably be OK. I usually shy away from embedded tabs\n> because they can cause confusion in editors. But we have them already,\n> and this kind of expected output is not touched all that often.\n\nHmph, I do shy away from trailing whitespaces and we cannot do\nleading tabs when doing <<-EOF, but I haven't seen much problem with\nthem in the middle of a line.\n\nIt does become annoying when they happen to be at the 7th column\nbecause it is hard to tell (even with one-letter-at-a-time move\ncommand in your editor) if it is a SP or HT, but q-to-tab in the\nmiddle of lines would make the result much harder to read and the\ncure is worse than the disease.\n\n"},{"id":"436897","messageId":"xmqq4kabyoo3.fsf@gitster.g","threadId":"56530","inReplyTo":"pull.1091.git.git.1631970872884.gitgitgadget@gmail.com","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T21:20:28Z","receivedAt":"2021-09-23T21:20:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Andrzej Hunt via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> diff --git a/connect.c b/connect.c\n> index aff13a270e6..eaf7d6d2618 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n>  \t\t\tif (!*value || isspace(*value)) {\n>  \t\t\t\tif (lenp)\n>  \t\t\t\t\t*lenp = 0;\n> +\t\t\t\tif (offset)\n> +\t\t\t\t\t*offset = found + len - feature_list;\n>  \t\t\t\treturn value;\n>  \t\t\t}\n>  \t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> index 5c941949b98..34538cebf01 100755\n> --- a/t/t5704-protocol-violations.sh\n> +++ b/t/t5704-protocol-violations.sh\n> @@ -32,4 +32,17 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n>  \ttest_i18ngrep \"expected flush after fetch arguments\" err\n>  '\n>  \n> +test_expect_success 'bogus symref in v0 capabilities' '\n> +\ttest_commit foo &&\n> +\toid=$(git rev-parse HEAD) &&\n> +\t{\n> +\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n> +\t\tprintf \"0000\"\n> +\t} >input &&\n> +\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> +\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n>\n> base-commit: 186eaaae567db501179c0af0bf89b34cbea02c26\n\nI've been seeing an occasional and not-reliably-reproducible test\nfailure from t5704 in 'seen' these days---since this is the only\ncommit that touches t5704, I am suspecting if there is something\nracy about it, but I am coming up empty after staring at it for a\nfew minutes.\n\nBuilding 87446480 (connect: also update offset for features without\nvalues, 2021-09-18), which is an application of the patch directly on\ntop of v2.33.0, and doing\n\n    $ cd t\n    $ while sh t5704-*.sh; do :; done\n\nI can get it fail in a dozen iterations or so when the box is\nloaded, so it does seem timing dependent.\n\n"},{"id":"436899","messageId":"YUzzwCwlR9AwSeOD@coredump.intra.peff.net","threadId":"56530","inReplyTo":"xmqq4kabyoo3.fsf@gitster.g","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T21:38:08Z","receivedAt":"2021-09-23T21:38:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 23, 2021 at 02:20:28PM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success 'bogus symref in v0 capabilities' '\n> > +\ttest_commit foo &&\n> > +\toid=$(git rev-parse HEAD) &&\n> > +\t{\n> > +\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> > +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n> > +\t\tprintf \"0000\"\n> > +\t} >input &&\n> > +\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> > +\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> >  test_done\n> >\n> > base-commit: 186eaaae567db501179c0af0bf89b34cbea02c26\n> \n> I've been seeing an occasional and not-reliably-reproducible test\n> failure from t5704 in 'seen' these days---since this is the only\n> commit that touches t5704, I am suspecting if there is something\n> racy about it, but I am coming up empty after staring at it for a\n> few minutes.\n> \n> Building 87446480 (connect: also update offset for features without\n> values, 2021-09-18), which is an application of the patch directly on\n> top of v2.33.0, and doing\n> \n>     $ cd t\n>     $ while sh t5704-*.sh; do :; done\n> \n> I can get it fail in a dozen iterations or so when the box is\n> loaded, so it does seem timing dependent.\n\nI think the problem is that our fake upload-pack exits immediately, so\nls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say\n\"0000\" to indicate that it's not interested in fetching anything (in v2,\nit doesn't bother, since fetching would be a separate request that it\njust declines to make).\n\nThis seems to fix it:\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 34538cebf0..0983c2b507 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '\n \t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf \"0000\"\n \t} >input &&\n-\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n+\tgit ls-remote --upload-pack=\"cat input; read junk;:\" . >actual &&\n \tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n \ttest_cmp expect actual\n '\n\n-Peff\n"},{"id":"436901","messageId":"xmqqr1dfx8lm.fsf@gitster.g","threadId":"56530","inReplyTo":"YUzzwCwlR9AwSeOD@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T21:52:53Z","receivedAt":"2021-09-23T21:53:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think the problem is that our fake upload-pack exits immediately, so\n> ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say\n> \"0000\" to indicate that it's not interested in fetching anything (in v2,\n> it doesn't bother, since fetching would be a separate request that it\n> just declines to make).\n\nAh, Makes sense---the usual SIGPIPE problem ;-)\n\n> This seems to fix it:\n>\n> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> index 34538cebf0..0983c2b507 100755\n> --- a/t/t5704-protocol-violations.sh\n> +++ b/t/t5704-protocol-violations.sh\n> @@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '\n>  \t\t\ttest-tool pkt-line pack-raw-stdin &&\n>  \t\tprintf \"0000\"\n>  \t} >input &&\n> -\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> +\tgit ls-remote --upload-pack=\"cat input; read junk;:\" . >actual &&\n>  \tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n>  \ttest_cmp expect actual\n>  '\n\nYup.  In the original thread there was some further back-and-forth\nabout further improving the test, if I recall correctly; has the\nissue been settled there, or is everybody happy with the above\nversion?\n\nThanks.\n"},{"id":"436904","messageId":"YUz5dPB6/jFmdSRU@coredump.intra.peff.net","threadId":"56530","inReplyTo":"xmqqr1dfx8lm.fsf@gitster.g","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-23T22:02:28Z","receivedAt":"2021-09-23T22:02:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 23, 2021 at 02:52:53PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I think the problem is that our fake upload-pack exits immediately, so\n> > ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say\n> > \"0000\" to indicate that it's not interested in fetching anything (in v2,\n> > it doesn't bother, since fetching would be a separate request that it\n> > just declines to make).\n> \n> Ah, Makes sense---the usual SIGPIPE problem ;-)\n\nYes, though it definitely took some head-scratching for me to see where\nit was. ;)\n\nDoing: \"./t5704-* --stress\" made it pretty clear. It fails almost\nimmediately, and mentions SIGPIPE (well, exit code 141, but by now I\nhave that one memorized).\n\n> > This seems to fix it:\n> >\n> > diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> > index 34538cebf0..0983c2b507 100755\n> > --- a/t/t5704-protocol-violations.sh\n> > +++ b/t/t5704-protocol-violations.sh\n> > @@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '\n> >  \t\t\ttest-tool pkt-line pack-raw-stdin &&\n> >  \t\tprintf \"0000\"\n> >  \t} >input &&\n> > -\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> > +\tgit ls-remote --upload-pack=\"cat input; read junk;:\" . >actual &&\n> >  \tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n> >  \ttest_cmp expect actual\n> >  '\n> \n> Yup.  In the original thread there was some further back-and-forth\n> about further improving the test, if I recall correctly; has the\n> issue been settled there, or is everybody happy with the above\n> version?\n\nI think the change I showed earlier (to use ls-remote --symref) is worth\ndoing. There was lots of discussion about how to format a tab, but in\nthe end I don't think it really matters.\n\nSo here's that patch again, with this race fix on top, which could be\nsquashed in, and then I hope we can call it good.\n\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 34538cebf0..bc393d7c31 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -35,13 +35,15 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n test_expect_success 'bogus symref in v0 capabilities' '\n \ttest_commit foo &&\n \toid=$(git rev-parse HEAD) &&\n+\tdst=refs/heads/foo &&\n \t{\n-\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n+\t\tprintf \"%s HEAD\\0symref object-format=%s symref=HEAD:%s\\n\" \\\n+\t\t\t\"$oid\" \"$GIT_DEFAULT_HASH\" \"$dst\" |\n \t\t\ttest-tool pkt-line pack-raw-stdin &&\n \t\tprintf \"0000\"\n \t} >input &&\n-\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n-\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n+\tgit ls-remote --symref --upload-pack=\"cat input; read junk;:\" . >actual &&\n+\tprintf \"ref: %s\\tHEAD\\n%s\\tHEAD\\n\" \"$dst\" \"$oid\" >expect &&\n \ttest_cmp expect actual\n '\n \n"},{"id":"437083","messageId":"14ff5661-e06a-8348-7088-387fa7e3e094@ahunt.org","threadId":"56530","inReplyTo":"YUYLXKN8U9AMa5ke@nand.local","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-09-26T15:14:35Z","receivedAt":"2021-09-26T15:14:43Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 18/09/2021 17:53, Taylor Blau wrote:\n>> parse_feature_value() does not update offset if the feature being\n>> searched for does not specify a value. A loop that uses\n>> parse_feature_value() to find a feature which was specified without a\n>> value therefore might never exit (such loops will typically use\n>> next_server_feature_value() as opposed to parse_feature_value() itself).\n>> This usually isn't an issue: there's no point in using\n>> next_server_feature_value() to search for repeated instances of the same\n>> capability unless that capability typically specifies a value - but a\n>> broken server could send a response that omits the value for a feature\n>> even when we are expecting a value.\n> \n> It may be worth adding a little detail here. parse_feature_value takes\n> an offset, and uses it to seek past the point in features_list that\n> we've already seen. But if we get a value-less feature, then offset is\n> never updated, and we'll keep parsing the same thing over and over in a\n> loop.\n> \n> (I know that you know all of that, but I think it is worth spelling out\n> a little more clearly in the patch message).\n\nGood point - I've tried to improve this for V2 (I've mostly just copied \nyour description verbatim).\n\n> \n>> Therefore we add an offset update calculation for the no-value case,\n>> which helps ensure that loops using next_server_feature_value() will\n>> always terminate.\n> \n>> next_server_feature_value(), and the offset calculation, were first\n>> added in 2.28 in:\n>>    2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)\n> \n> This line wrapping is a little odd, but not a big deal.\n\nI'll fix this for V2 - I think I tried too hard to make this look nice, \nbut putting the reference inline does look better (and I've now realised \nthis seems to be the usual way of doing things here).\n\nATB,\n\nAndrzej\n"},{"id":"437084","messageId":"e1395ff2-e697-83b2-082b-d5468b7a11ac@ahunt.org","threadId":"56530","inReplyTo":"YUz5dPB6/jFmdSRU@coredump.intra.peff.net","subject":"Re: [PATCH] connect: also update offset for features without values","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-09-26T15:16:22Z","receivedAt":"2021-09-26T15:16:32Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\nOn 24/09/2021 00:02, Jeff King wrote:\n> On Thu, Sep 23, 2021 at 02:52:53PM -0700, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>> I think the problem is that our fake upload-pack exits immediately, so\n>>> ls-remote gets SIGPIPE. In a v0 conversation, ls-remote expects to say\n>>> \"0000\" to indicate that it's not interested in fetching anything (in v2,\n>>> it doesn't bother, since fetching would be a separate request that it\n>>> just declines to make).\n>>\n>> Ah, Makes sense---the usual SIGPIPE problem ;-)\n> \n> Yes, though it definitely took some head-scratching for me to see where\n> it was. ;)\n> \n> Doing: \"./t5704-* --stress\" made it pretty clear. It fails almost\n> immediately, and mentions SIGPIPE (well, exit code 141, but by now I\n> have that one memorized).\n> \n>>> This seems to fix it:\n>>>\n>>> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n>>> index 34538cebf0..0983c2b507 100755\n>>> --- a/t/t5704-protocol-violations.sh\n>>> +++ b/t/t5704-protocol-violations.sh\n>>> @@ -40,7 +40,7 @@ test_expect_success 'bogus symref in v0 capabilities' '\n>>>   \t\t\ttest-tool pkt-line pack-raw-stdin &&\n>>>   \t\tprintf \"0000\"\n>>>   \t} >input &&\n>>> -\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n>>> +\tgit ls-remote --upload-pack=\"cat input; read junk;:\" . >actual &&\n>>>   \tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n>>>   \ttest_cmp expect actual\n>>>   '\n>>\n>> Yup.  In the original thread there was some further back-and-forth\n>> about further improving the test, if I recall correctly; has the\n>> issue been settled there, or is everybody happy with the above\n>> version?\n> \n> I think the change I showed earlier (to use ls-remote --symref) is worth\n> doing. There was lots of discussion about how to format a tab, but in\n> the end I don't think it really matters.\n> \n> So here's that patch again, with this race fix on top, which could be\n> squashed in, and then I hope we can call it good.\n\nThanks again for doing the actual work - I'll send out a V2 with your \nchanges squashed in, along with an attempt at improving the commit \nmessage (as discussed in reply to Taylor's comments).\n\n> \n> diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\n> index 34538cebf0..bc393d7c31 100755\n> --- a/t/t5704-protocol-violations.sh\n> +++ b/t/t5704-protocol-violations.sh\n> @@ -35,13 +35,15 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n>   test_expect_success 'bogus symref in v0 capabilities' '\n>   \ttest_commit foo &&\n>   \toid=$(git rev-parse HEAD) &&\n> +\tdst=refs/heads/foo &&\n>   \t{\n> -\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n> +\t\tprintf \"%s HEAD\\0symref object-format=%s symref=HEAD:%s\\n\" \\\n> +\t\t\t\"$oid\" \"$GIT_DEFAULT_HASH\" \"$dst\" |\n>   \t\t\ttest-tool pkt-line pack-raw-stdin &&\n>   \t\tprintf \"0000\"\n>   \t} >input &&\n> -\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n> -\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n> +\tgit ls-remote --symref --upload-pack=\"cat input; read junk;:\" . >actual &&\n> +\tprintf \"ref: %s\\tHEAD\\n%s\\tHEAD\\n\" \"$dst\" \"$oid\" >expect &&\n>   \ttest_cmp expect actual\n>   '\n>   \n> \n"},{"id":"437085","messageId":"pull.1091.v2.git.git.1632671913693.gitgitgadget@gmail.com","threadId":"56530","inReplyTo":"pull.1091.git.git.1631970872884.gitgitgadget@gmail.com","subject":"[PATCH v2] connect: also update offset for features without values","fromName":"Andrzej Hunt via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-26T15:58:33Z","receivedAt":"2021-09-26T15:58:40Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"From: Andrzej Hunt <andrzej@ahunt.org>\n\nparse_feature_value() takes an offset, and uses it to seek past the\npoint in features_list that we've already seen. However if the feature\nbeing searched for does not specify a value, the offset is not\nupdated. Therefore if we call parse_feature_value() in a loop on a\nvalue-less feature, we'll keep on parsing the same feature over and over\nagain. This usually isn't an issue: there's no point in using\nnext_server_feature_value() to search for repeated instances of the same\ncapability unless that capability typically specifies a value - but a\nbroken server could send a response that omits the value for a feature\neven when we are expecting a value.\n\nTherefore we add an offset update calculation for the no-value case,\nwhich helps ensure that loops using next_server_feature_value() will\nalways terminate.\n\nnext_server_feature_value(), and the offset calculation, were first\nadded in 2.28 in 2c6a403d96 (connect: add function to parse multiple\nv1 capability values, 2020-05-25).\n\nThanks to Peff for authoring the test.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Andrzej Hunt <andrzej@ahunt.org>\n---\n    connect: also update offset for features without values\n    \n    V2 incorporates Peff's test and test stability improvements, and\n    attempts to improve the commit message.\n    \n    ATB,\n    \n    Andrzej\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1091%2Fahunt%2Fconnectloop-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1091/ahunt/connectloop-v2\nPull-Request: https://github.com/git/git/pull/1091\n\nRange-diff vs v1:\n\n 1:  dcbb05ddc4b ! 1:  908e4e6c4ed connect: also update offset for features without values\n     @@ Metadata\n       ## Commit message ##\n          connect: also update offset for features without values\n      \n     -    parse_feature_value() does not update offset if the feature being\n     -    searched for does not specify a value. A loop that uses\n     -    parse_feature_value() to find a feature which was specified without a\n     -    value therefore might never exit (such loops will typically use\n     -    next_server_feature_value() as opposed to parse_feature_value() itself).\n     -    This usually isn't an issue: there's no point in using\n     +    parse_feature_value() takes an offset, and uses it to seek past the\n     +    point in features_list that we've already seen. However if the feature\n     +    being searched for does not specify a value, the offset is not\n     +    updated. Therefore if we call parse_feature_value() in a loop on a\n     +    value-less feature, we'll keep on parsing the same feature over and over\n     +    again. This usually isn't an issue: there's no point in using\n          next_server_feature_value() to search for repeated instances of the same\n          capability unless that capability typically specifies a value - but a\n          broken server could send a response that omits the value for a feature\n     @@ Commit message\n          always terminate.\n      \n          next_server_feature_value(), and the offset calculation, were first\n     -    added in 2.28 in:\n     -      2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)\n     +    added in 2.28 in 2c6a403d96 (connect: add function to parse multiple\n     +    v1 capability values, 2020-05-25).\n      \n          Thanks to Peff for authoring the test.\n      \n     @@ t/t5704-protocol-violations.sh: test_expect_success 'extra delim packet in v2 fe\n      +test_expect_success 'bogus symref in v0 capabilities' '\n      +\ttest_commit foo &&\n      +\toid=$(git rev-parse HEAD) &&\n     ++\tdst=refs/heads/foo &&\n      +\t{\n     -+\t\tprintf \"%s HEAD\\0symref object-format=%s\\n\" \"$oid\" \"$GIT_DEFAULT_HASH\" |\n     ++\t\tprintf \"%s HEAD\\0symref object-format=%s symref=HEAD:%s\\n\" \\\n     ++\t\t\t\"$oid\" \"$GIT_DEFAULT_HASH\" \"$dst\" |\n      +\t\t\ttest-tool pkt-line pack-raw-stdin &&\n      +\t\tprintf \"0000\"\n      +\t} >input &&\n     -+\tgit ls-remote --upload-pack=\"cat input ;:\" . >actual &&\n     -+\tprintf \"%s\\tHEAD\\n\" \"$oid\" >expect &&\n     ++\tgit ls-remote --symref --upload-pack=\"cat input; read junk;:\" . >actual &&\n     ++\tprintf \"ref: %s\\tHEAD\\n%s\\tHEAD\\n\" \"$dst\" \"$oid\" >expect &&\n      +\ttest_cmp expect actual\n      +'\n      +\n\n\n connect.c                      |  2 ++\n t/t5704-protocol-violations.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/connect.c b/connect.c\nindex aff13a270e6..eaf7d6d2618 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -557,6 +557,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\tif (!*value || isspace(*value)) {\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = 0;\n+\t\t\t\tif (offset)\n+\t\t\t\t\t*offset = found + len - feature_list;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\ndiff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh\nindex 5c941949b98..bc393d7c319 100755\n--- a/t/t5704-protocol-violations.sh\n+++ b/t/t5704-protocol-violations.sh\n@@ -32,4 +32,19 @@ test_expect_success 'extra delim packet in v2 fetch args' '\n \ttest_i18ngrep \"expected flush after fetch arguments\" err\n '\n \n+test_expect_success 'bogus symref in v0 capabilities' '\n+\ttest_commit foo &&\n+\toid=$(git rev-parse HEAD) &&\n+\tdst=refs/heads/foo &&\n+\t{\n+\t\tprintf \"%s HEAD\\0symref object-format=%s symref=HEAD:%s\\n\" \\\n+\t\t\t\"$oid\" \"$GIT_DEFAULT_HASH\" \"$dst\" |\n+\t\t\ttest-tool pkt-line pack-raw-stdin &&\n+\t\tprintf \"0000\"\n+\t} >input &&\n+\tgit ls-remote --symref --upload-pack=\"cat input; read junk;:\" . >actual &&\n+\tprintf \"ref: %s\\tHEAD\\n%s\\tHEAD\\n\" \"$dst\" \"$oid\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 4c38ced6901a8523cea197b31b2616240ec9fb6e\n-- \ngitgitgadget\n"},{"id":"437177","messageId":"YVIf5I4BYWrHgzg9@coredump.intra.peff.net","threadId":"56530","inReplyTo":"pull.1091.v2.git.git.1632671913693.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] connect: also update offset for features without values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-27T19:47:48Z","receivedAt":"2021-09-27T19:47:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 26, 2021 at 03:58:33PM +0000, Andrzej Hunt via GitGitGadget wrote:\n\n>     connect: also update offset for features without values\n>     \n>     V2 incorporates Peff's test and test stability improvements, and\n>     attempts to improve the commit message.\n\nThanks, this looks great to me.\n\n-Peff\n"}]}