{"thread":{"id":"59579","subject":"Infinite loop + memory leak in annotate_refs_with_symref_info","startedAt":"2023-04-11T20:03:45Z","lastAt":"2023-04-17T16:06:55Z","messageCount":30,"participants":["Jonas Haag","Taylor Blau","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"475167","messageId":"39035D34-8548-44B0-BBBB-5C36B3876C4A@lophus.org","threadId":"59579","inReplyTo":null,"subject":"Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Jonas Haag","fromEmail":"jonas@lophus.org","sentAt":"2023-04-11T19:53:59Z","receivedAt":"2023-04-11T20:03:45Z","isPatch":false,"sender":{"key":"jonas@lophus.org","avatar":null},"body":"Hello!\n\nThere is an infinite loop with an accompanying memory leak in annotate_refs_with_symref_info that was introduced in Git 2.28 (I think in commit 2c6a403: “connect: add function to parse multiple v1 capability values”).\n\nTo reproduce the issue, start Klaus [1] using the --smarthttp option and attempt to clone a repository. git-remote-http will enter an infinite loop.\n\nI think this is triggered by a bug in Dulwich, the Python Git implementation that Klaus uses. I’m assuming that Dulwich sends some invalid responses that make the Git client go into an infinite loop.\n\nI believe the bug in Git is in connect.c, function parse_feature_value, in the updating of `*offset`: It doesn’t seem to take into account that `feature_list` has already been offset by `*offset`. I believe the update needs to use `*offset +=` instead of `*offset =`. When I make this change, the infinite loop seems to go away, and cloning via Klaus/Dulwich will fail with “invalid index-pack output”. Cloning from github.com works, although I’m not sure if that’s a relevant smoke test in this case.\n\nJonas\n\n[1] https://github.com/jonashaag/klaus\n"},{"id":"475168","messageId":"ZDXCKecwxo36fALm@nand.local","threadId":"59579","inReplyTo":"39035D34-8548-44B0-BBBB-5C36B3876C4A@lophus.org","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T20:25:13Z","receivedAt":"2023-04-11T20:25:36Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Jonas,\n\nOn Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:\n> Hello!\n>\n> There is an infinite loop with an accompanying memory leak in\n> annotate_refs_with_symref_info that was introduced in Git 2.28 (I\n> think in commit 2c6a403: “connect: add function to parse multiple v1\n> capability values”).\n\nI'm not familiar with Klaus and don't have it installed, but a couple of\nquestions: were you able to reproduce this result with any other forges\nor tools, and were you able to confirm that 2c6a403 is the culprit via a\nbisection?\n\nIn case the answer to the latter question is \"yes\", I cc'd brian carlson\non this thread, since they are the original author of that patch.\n\n> I believe the bug in Git is in connect.c, function\n> parse_feature_value, in the updating of `*offset`: It doesn’t seem to\n> take into account that `feature_list` has already been offset by\n> `*offset`. I believe the update needs to use `*offset +=` instead of\n> `*offset =`. When I make this change, the infinite loop seems to go\n> away, and cloning via Klaus/Dulwich will fail with “invalid index-pack\n> output”. Cloning from github.com works, although I’m not sure if\n> that’s a relevant smoke test in this case.\n\nI'm not sure I understand. Looking at the relevant bits in\nconnect.c::parse_feature_value(), it all seems correct to me, since the\nbeginning of `feature_list` is adjusted by the current value of\n`*offset`.\n\n> [1] https://github.com/jonashaag/klaus\n\nThanks,\nTaylor\n"},{"id":"475169","messageId":"20230411210633.GA626331@coredump.intra.peff.net","threadId":"59579","inReplyTo":"39035D34-8548-44B0-BBBB-5C36B3876C4A@lophus.org","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-11T21:06:33Z","receivedAt":"2023-04-11T21:06:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:\n\n> There is an infinite loop with an accompanying memory leak in\n> annotate_refs_with_symref_info that was introduced in Git 2.28 (I\n> think in commit 2c6a403: “connect: add function to parse multiple v1\n> capability values”).\n\nHave you tried to reproduce with a more recent version of Git? This\nsounds a lot like the bug fixed in 44d2aec6e8 (connect: also update\noffset for features without values, 2021-09-26), which is in v2.33.1.\n\n-Peff\n"},{"id":"475172","messageId":"20230411211604.GB626331@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230411210633.GA626331@coredump.intra.peff.net","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-11T21:16:04Z","receivedAt":"2023-04-11T21:16:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2023 at 05:06:33PM -0400, Jeff King wrote:\n\n> On Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:\n> \n> > There is an infinite loop with an accompanying memory leak in\n> > annotate_refs_with_symref_info that was introduced in Git 2.28 (I\n> > think in commit 2c6a403: “connect: add function to parse multiple v1\n> > capability values”).\n> \n> Have you tried to reproduce with a more recent version of Git? This\n> sounds a lot like the bug fixed in 44d2aec6e8 (connect: also update\n> offset for features without values, 2021-09-26), which is in v2.33.1.\n\nNever mind. I was able to reproduce (I never used klaus, but it's\npackaged for Debian, so it was pretty easy to do). And yes, the problem\nstill exists today. And bisection confirms it's from 2c6a403.\n\n-Peff\n"},{"id":"475173","messageId":"ZDXPemH0d3YWnpjL@nand.local","threadId":"59579","inReplyTo":"20230411211604.GB626331@coredump.intra.peff.net","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T21:22:02Z","receivedAt":"2023-04-11T21:22:11Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 05:16:04PM -0400, Jeff King wrote:\n> On Tue, Apr 11, 2023 at 05:06:33PM -0400, Jeff King wrote:\n>\n> > On Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:\n> >\n> > > There is an infinite loop with an accompanying memory leak in\n> > > annotate_refs_with_symref_info that was introduced in Git 2.28 (I\n> > > think in commit 2c6a403: “connect: add function to parse multiple v1\n> > > capability values”).\n> >\n> > Have you tried to reproduce with a more recent version of Git? This\n> > sounds a lot like the bug fixed in 44d2aec6e8 (connect: also update\n> > offset for features without values, 2021-09-26), which is in v2.33.1.\n>\n> Never mind. I was able to reproduce (I never used klaus, but it's\n> packaged for Debian, so it was pretty easy to do). And yes, the problem\n> still exists today. And bisection confirms it's from 2c6a403.\n\nYeah, same here. I hadn't used it either, but it's easily installable\nvia pip, too. Indeed, you can see the value of *offset jumping backwards\nin `connect.c::parse_value_value()` (whose caller in this case is\n`connect.c::annotate_refs_with_symref_info()`).\n\nThanks,\nTaylor\n"},{"id":"475178","messageId":"20230411215845.GA678138@coredump.intra.peff.net","threadId":"59579","inReplyTo":"ZDXPemH0d3YWnpjL@nand.local","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-11T21:58:45Z","receivedAt":"2023-04-11T21:58:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2023 at 05:22:02PM -0400, Taylor Blau wrote:\n\n> > Never mind. I was able to reproduce (I never used klaus, but it's\n> > packaged for Debian, so it was pretty easy to do). And yes, the problem\n> > still exists today. And bisection confirms it's from 2c6a403.\n> \n> Yeah, same here. I hadn't used it either, but it's easily installable\n> via pip, too. Indeed, you can see the value of *offset jumping backwards\n> in `connect.c::parse_value_value()` (whose caller in this case is\n> `connect.c::annotate_refs_with_symref_info()`).\n\nYep. And Jonas's suggested fix is the right thing. Assigning offset\ndirectly _would_ be the right thing, since we are taking the distance\nback to the beginning of the feature_list string. Except that earlier in\nthe function we incremented feature_list by the incoming value of\nthe offset!\n\nHere's a test that demonstrates the problem (and passes with the\naccompanying code change):\n\ndiff --git a/connect.c b/connect.c\nindex c0c8a38178..5e265ba9d7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -616,7 +616,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\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\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@@ -628,7 +628,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = end;\n \t\t\t\tif (offset)\n-\t\t\t\t\t*offset = value + end - feature_list;\n+\t\t\t\t\t*offset += value + end - feature_list;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/*\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 20d063fb9a..c8422d66e7 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -360,4 +360,39 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \ttest_cmp expect actual.v2\n '\n \n+test_expect_success 'v0 clients can handle multiple symrefs' '\n+\t# Git will not generate multiple symref entries for v0 these days, but it\n+\t# is technically allowed, and we did so until d007dbf7d6 (Revert\n+\t# \"upload-pack: send non-HEAD symbolic refs\", 2013-11-18). Test the\n+\t# client handling here by faking that older behavior.\n+\t#\n+\t# Note that our oid is hard-coded to always be sha1, and not using\n+\t# test_oid. Since our fake capabilities line does not have an\n+\t# object-format entry, the client will always use sha1 mode.\n+\toid=1234567890123456789012345678901234567890 &&\n+\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n+\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n+\n+\ttest-tool pkt-line pack >input.q <<-EOF &&\n+\t$oid HEADQ$symrefs\n+\t$oid refs/heads/main\n+\t$oid refs/remotes/origin/HEAD\n+\t$oid refs/remotes/origin/main\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n+\tcat >expect <<-EOF &&\n+\tref: refs/heads/main\tHEAD\n+\t$oid\tHEAD\n+\t$oid\trefs/heads/main\n+\tref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/main\n+\tEOF\n+\n+\tgit ls-remote --symref --upload-pack=\"cat input; cat;:\" . >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n\nI was going to ask if Jonas wanted to wrap this up as a patch, but I\nthink that's pretty much it right there. :) I'll write up a commit\nmessage for it later tonight.\n\nI also wondered if we tested this multiple-symref case for protocol v2\n(where it works fine), but it looks like we may not. There are earlier\ntests which _would_ trigger it, but we force them into v0 mode, due to\nb2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n2019-02-25). I think we really should be letting ls-remote use the\nprotocol it prefers (v2 by default, and v0 if the suite is run in that\nmode), and the expected output should be adjusted based on the mode.\nI'll see if I can do that as well, to make this a two-patch series.\n\n-Peff\n"},{"id":"475179","messageId":"xmqqa5ze9glg.fsf@gitster.g","threadId":"59579","inReplyTo":"20230411215845.GA678138@coredump.intra.peff.net","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-11T22:52:43Z","receivedAt":"2023-04-11T22:52:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yep. And Jonas's suggested fix is the right thing. Assigning offset\n> directly _would_ be the right thing, since we are taking the distance\n> back to the beginning of the feature_list string. Except that earlier in\n> the function we incremented feature_list by the incoming value of\n> the offset!\n\nSigh. Thanks for finding the problem with a fix. The data flow in\nthis function is horrible, but yes, \"found + len - feature_list\" is\nsmaller than the code expects to be because feature_list is moved\nforward before entering the loop, and I can see how the patch fixes\nthe problem.\n\n> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\n> index 20d063fb9a..c8422d66e7 100755\n> --- a/t/t5512-ls-remote.sh\n> +++ b/t/t5512-ls-remote.sh\n> @@ -360,4 +360,39 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '\n> \ttest_cmp expect actual.v2\n> '\n> \n> +test_expect_success 'v0 clients can handle multiple symrefs' '\n> +\t# Git will not generate multiple symref entries for v0 these days, but it\n> +\t# is technically allowed, and we did so until d007dbf7d6 (Revert\n> +\t# \"upload-pack: send non-HEAD symbolic refs\", 2013-11-18). Test the\n> +\t# client handling here by faking that older behavior.\n\nYeah, I remember that fix where somebody had tons of symbolic refs\nand busted the protocol limit. Is \"multiple symref\" used here\nbecause it is the easiest to reproduce the issue, or have we saw\nsuch a potentially broken server in the wild?\n\n> +\t# Note that our oid is hard-coded to always be sha1, and not using\n> +\t# test_oid. Since our fake capabilities line does not have an\n> +\t# object-format entry, the client will always use sha1 mode.\n\nIt probably is OK to run the test in that \"undeclared - assume\nSHA-1\" mode, even though I think we give an explicit \"object-format\"\ncapability even when talking from the SHA-1 repository these days.\n\n> +\toid=1234567890123456789012345678901234567890 &&\n> +\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n> +\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n\n> I also wondered if we tested this multiple-symref case for protocol v2\n> (where it works fine), but it looks like we may not. There are earlier\n> tests which _would_ trigger it, but we force them into v0 mode, due to\n> b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n> 2019-02-25). I think we really should be letting ls-remote use the\n> protocol it prefers (v2 by default, and v0 if the suite is run in that\n> mode), and the expected output should be adjusted based on the mode.\n> I'll see if I can do that as well, to make this a two-patch series.\n\nThanks. I really appreciate your being almost always thorough and\nwish more contributors took inspirations.\n\n\n"},{"id":"475182","messageId":"ZDX0SWLpPQGf9BPl@nand.local","threadId":"59579","inReplyTo":"ZDXCKecwxo36fALm@nand.local","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-04-11T23:59:05Z","receivedAt":"2023-04-11T23:59:13Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 11, 2023 at 04:25:13PM -0400, Taylor Blau wrote:\n> > I believe the bug in Git is in connect.c, function\n> > parse_feature_value, in the updating of `*offset`: It doesn’t seem to\n> > take into account that `feature_list` has already been offset by\n> > `*offset`. I believe the update needs to use `*offset +=` instead of\n> > `*offset =`. When I make this change, the infinite loop seems to go\n> > away, and cloning via Klaus/Dulwich will fail with “invalid index-pack\n> > output”. Cloning from github.com works, although I’m not sure if\n> > that’s a relevant smoke test in this case.\n>\n> I'm not sure I understand. Looking at the relevant bits in\n> connect.c::parse_feature_value(), it all seems correct to me, since the\n> beginning of `feature_list` is adjusted by the current value of\n> `*offset`.\n\nOops. This was exactly[1] the problem as you suggested, I was just\nthinking about it backwards. When we write into `*offset`, we need to\ntake into account that `feature_list` has already been moved forward by\n`*offset`.\n\nObviously the discussion can continue below [1], but just wanted to\ncorrect my wrong here and acknowledge that you were absolutely right in\nyour original report.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/20230411215845.GA678138@coredump.intra.peff.net/\n"},{"id":"475183","messageId":"ZDYBJwCuXD6UCI5p@tapette.crustytoothpaste.net","threadId":"59579","inReplyTo":"ZDXCKecwxo36fALm@nand.local","subject":"Re: Infinite loop + memory leak in annotate_refs_with_symref_info","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2023-04-12T00:53:59Z","receivedAt":"2023-04-12T00:54:06Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2023-04-11 at 20:25:13, Taylor Blau wrote:\n> Hi Jonas,\n> \n> On Tue, Apr 11, 2023 at 10:53:59PM +0300, Jonas Haag wrote:\n> > Hello!\n> >\n> > There is an infinite loop with an accompanying memory leak in\n> > annotate_refs_with_symref_info that was introduced in Git 2.28 (I\n> > think in commit 2c6a403: “connect: add function to parse multiple v1\n> > capability values”).\n> \n> I'm not familiar with Klaus and don't have it installed, but a couple of\n> questions: were you able to reproduce this result with any other forges\n> or tools, and were you able to confirm that 2c6a403 is the culprit via a\n> bisection?\n> \n> In case the answer to the latter question is \"yes\", I cc'd brian carlson\n> on this thread, since they are the original author of that patch.\n\nI may be the author of the patch, but I honestly don't remember much\nabout that code except that it was a bear to write, and I am honestly\nnot terribly surprised that it came out less than perfect. I do\napologize for the buggy code, though.\n\nIt looks like Peff has come up with a patch downthread, and a test even,\nwhich I think will probably fix the problem, so I'll refrain from\nsending one myself for now.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"475185","messageId":"20230412062300.GA838367@coredump.intra.peff.net","threadId":"59579","inReplyTo":"xmqqa5ze9glg.fsf@gitster.g","subject":"[PATCH 0/7] v0 multiple-symref infinite loop fix and test cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:23:00Z","receivedAt":"2023-04-12T06:23:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2023 at 03:52:43PM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success 'v0 clients can handle multiple symrefs' '\n> > +\t# Git will not generate multiple symref entries for v0 these days, but it\n> > +\t# is technically allowed, and we did so until d007dbf7d6 (Revert\n> > +\t# \"upload-pack: send non-HEAD symbolic refs\", 2013-11-18). Test the\n> > +\t# client handling here by faking that older behavior.\n> \n> Yeah, I remember that fix where somebody had tons of symbolic refs\n> and busted the protocol limit. Is \"multiple symref\" used here\n> because it is the easiest to reproduce the issue, or have we saw\n> such a potentially broken server in the wild?\n\nBoth. :) It was what we saw in the wild, but it's also one of only two\ncapabilities that can cause the problem, because it requires passing an\noffset parameter, which we only do for capabilities we expect to see\nmultiple times. The other is \"object-format\", but we only ever print it\nonce. More details in the commit message to follow.\n\n> > +\t# Note that our oid is hard-coded to always be sha1, and not using\n> > +\t# test_oid. Since our fake capabilities line does not have an\n> > +\t# object-format entry, the client will always use sha1 mode.\n> \n> It probably is OK to run the test in that \"undeclared - assume\n> SHA-1\" mode, even though I think we give an explicit \"object-format\"\n> capability even when talking from the SHA-1 repository these days.\n\nWe do, but I think it is OK to ignore that. The test will continue to\nwork even in a world where sha256 becomes the default. If we eventually\nremove all vestiges of backwards-compatible sha1 support, it will have\nto be updated, but I'm OK with that (keep in mind that the test is also\nv0-only, as the v2 parser is totally different).\n\n> > I also wondered if we tested this multiple-symref case for protocol v2\n> > (where it works fine), but it looks like we may not. There are earlier\n> > tests which _would_ trigger it, but we force them into v0 mode, due to\n> > b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n> > 2019-02-25). I think we really should be letting ls-remote use the\n> > protocol it prefers (v2 by default, and v0 if the suite is run in that\n> > mode), and the expected output should be adjusted based on the mode.\n> > I'll see if I can do that as well, to make this a two-patch series.\n> \n> Thanks. I really appreciate your being almost always thorough and\n> wish more contributors took inspirations.\n\nWell, no good deed goes unpunished. :) These tests have not aged well,\nso there were a number of fixes to make. Here's the series I came up\nwith.\n\nThe first one is the bug fix; I put it at the front because it's\nobviously the most urgent. Patches 2-6 are test cleanups, and as such\nshould not be very risky, but I didn't want to hold up the fix. But they\ndo depend on the helper script introduced by patch 1, so they can't be\napplied separately.\n\nPatch 7 is a cleanup in the code which should have no behavior change.\nIt could be applied separately (or even dropped if you don't like it).\n\n [1/7]: v0 protocol: fix infinite loop when parsing multi-valued capabilities\n [2/7]: t5512: stop referring to \"v1\" protocol\n [3/7]: t5512: stop using jgit for capabilities^{} test\n [4/7]: t5512: add v2 support for \"ls-remote --symref\" test\n [5/7]: t5512: allow any protocol version for filtered symref test\n [6/7]: t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2\n [7/7]: v0 protocol: use size_t for capability length/offset\n\n builtin/receive-pack.c | 2 +-\n connect.c | 26 ++++----\n connect.h | 4 +-\n fetch-pack.c | 4 +-\n send-pack.c | 2 +-\n t/t5512-ls-remote.sh | 148 ++++++++++++++++++++++-------------------\n transport.c | 2 +-\n upload-pack.c | 2 +-\n 8 files changed, 101 insertions(+), 89 deletions(-)\n\n-Peff\n"},{"id":"475188","messageId":"20230412062924.GA1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:29:24Z","receivedAt":"2023-04-12T06:29:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If Git's client-side parsing of an upload-pack response (so git-fetch or\nls-remote) sees multiple instances of a single capability, it can enter\nan infinite loop due to a bug in advancing the \"offset\" parameter in the\nparser.\n\nThis bug can't happen between a client and server of the same Git\nversion. The client bug is in parse_feature_value() when the caller\npasses in an offset parameter. And that only happens when the v0\nprotocol is parsing \"symref\" and \"object-format\" capabilities, via\nnext_server_feature_value(). But Git has never produced multiple\nobject-format capabilities, and it stopped producing multiple symref\nvalues in d007dbf7d6 (Revert \"upload-pack: send non-HEAD symbolic refs\",\n2013-11-18).\n\nHowever, upload-pack did produce multiple symref entries for a while,\nand they are valid. Plus other implementations, such as Dulwich will\nstill do so. So we should handle them. And even if we do not expect it,\nit is obviously a bug for the parser to enter an infinite loop.\n\nThe bug itself is pretty simple. Commit 2c6a403d96 (connect: add\nfunction to parse multiple v1 capability values, 2020-05-25) added the\n\"offset\" parameter, which is used as both an in- and out-parameter. When\nparsing the first \"symref\" capability, *offset will be 0 on input, and\nafter parsing the capability, we set *offset to an index just past the\nvalue by taking a pointer difference \"(value + end) - feature_list\".\n\nBut on the second call, now *offset is set to that larger index, which\nlets us skip past the first \"symref\" capability. However, we do so by\nincrementing feature_list. That means our pointer difference is now too\nsmall; it is counting from where we resumed parsing, not from the start\nof the original feature_list pointer. And because we incremented\nfeature_list only inside our function, and not the caller, that\nincrement is lost next time the function is called.\n\nThe simplest solution is to account for those skipped bytes by\nincrementing *offset, rather than assigning to it. (The other possible\nsolution is to add an extra level of pointer indirection to feature_list\nso that the caller knows we moved it forward, but that seems more\ncomplicated).\n\nSince the bug can't be reproduced using the current version of\ngit-upload-pack, we'll instead hard-code an input which triggers the\nproblem. Before this patch it loops forever re-parsing the second symref\nentry. Now we check both that it finishes, and that it parses both\nentries correctly (a case we could not test at all before).\n\nWe don't need to worry about testing v2 here; it communicates the\ncapabilities in a completely different way, and doesn't use this code at\nall. There are tests earlier in t5512 that are meant to cover this (they\ndon't, but we'll address that in a future patch).\n\nReported-by: Jonas Haag \nSigned-off-by: Jeff King \n---\n connect.c | 4 ++--\n t/t5512-ls-remote.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0c8a38178..5e265ba9d7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -616,7 +616,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\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\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@@ -628,7 +628,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = end;\n \t\t\t\tif (offset)\n-\t\t\t\t\t*offset = value + end - feature_list;\n+\t\t\t\t\t*offset += value + end - feature_list;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/*\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 20d063fb9a..64dc6fa91c 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -15,6 +15,19 @@ generate_references () {\n \tdone\n }\n \n+test_expect_success 'set up fake upload-pack' '\n+\t# This can be used to simulate an upload-pack that just shows the\n+\t# contents of the \"input\" file (prepared with the test-tool pkt-line\n+\t# helper), and does not do any negotiation (since ls-remote does not\n+\t# need it).\n+\twrite_script cat-input <<-\\EOF\n+\t# send our initial advertisement/response\n+\tcat input\n+\t# soak up the flush packet from the client\n+\tcat\n+\tEOF\n+'\n+\n test_expect_success 'dies when no remote found' '\n \ttest_must_fail git ls-remote\n '\n@@ -360,4 +373,35 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \ttest_cmp expect actual.v2\n '\n \n+test_expect_success 'v0 clients can handle multiple symrefs' '\n+\t# Modern versions of Git will not return multiple symref capabilities\n+\t# for v0, so we have to hard-code the response. Note that we will\n+\t# always use both v0 and object-format=sha1 here, as the hard-coded\n+\t# response reflects a server that only supports those.\n+\toid=1234567890123456789012345678901234567890 &&\n+\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n+\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n+\n+\ttest-tool pkt-line pack >input.q <<-EOF &&\n+\t$oid HEADQ$symrefs\n+\t$oid refs/heads/main\n+\t$oid refs/remotes/origin/HEAD\n+\t$oid refs/remotes/origin/main\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n+\tcat >expect <<-EOF &&\n+\tref: refs/heads/main\tHEAD\n+\t$oid\tHEAD\n+\t$oid\trefs/heads/main\n+\tref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/main\n+\tEOF\n+\n+\tgit ls-remote --symref --upload-pack=./cat-input . >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475189","messageId":"20230412062954.GB1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 2/7] t5512: stop referring to \"v1\" protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:29:54Z","receivedAt":"2023-04-12T06:29:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There really isn't a \"v1\" Git protocol. It's just v0 with an extra probe\nwhich we used to test compatibility in preparation for v2. Any tests\nthat are looking for before/after behavior for v2 really care about\n\"v0\". Mentioning \"v1\" in these tests is just making things more\nconfusing, because we don't care about that probe; we're really testing\nv0. So let's say so.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 64dc6fa91c..ce7a9f1594 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -358,17 +358,17 @@ test_expect_success 'ls-remote --sort fails gracefully outside repository' '\n test_expect_success 'ls-remote patterns work with all protocol versions' '\n \tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n \t\trefs/heads/main refs/remotes/origin/main >expect &&\n-\tgit -c protocol.version=1 ls-remote . main >actual.v1 &&\n-\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=0 ls-remote . main >actual.v0 &&\n+\ttest_cmp expect actual.v0 &&\n \tgit -c protocol.version=2 ls-remote . main >actual.v2 &&\n \ttest_cmp expect actual.v2\n '\n \n test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n \t\trefs/heads/ refs/tags/ >expect &&\n-\tgit -c protocol.version=1 ls-remote --heads --tags . >actual.v1 &&\n-\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=0 ls-remote --heads --tags . >actual.v0 &&\n+\ttest_cmp expect actual.v0 &&\n \tgit -c protocol.version=2 ls-remote --heads --tags . >actual.v2 &&\n \ttest_cmp expect actual.v2\n '\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475190","messageId":"20230412063118.GC1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 3/7] t5512: stop using jgit for capabilities^{} test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:31:18Z","receivedAt":"2023-04-12T06:31:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit eb398797cd (connect: advertized capability is not a ref,\n2016-09-09) added support for an upload-pack server responding with:\n\n 0000000000000000000000000000000000000000 capabilities^{}\n\nfollowed by a NUL and capabilities. This is not something Git itself has\never produced for upload-pack, but JGit does. And hence the test used\nJGit to reproduce the real-world situation. That was good for verifying\nthat the incompatibility was fixed, but it's a lousy regression test for\na few reasons:\n\n - hardly anybody runs it, because you have to have jgit installed\n\n - we're depending on jgit's behavior for the test to do anything\n useful. In particular, this behavior is only relevant to the v0\n protocol, but these days we ask for the v2 protocol by default. So\n for modern jgit, this is probably testing nothing.\n\n - it's complicated and slow. We had to do some fifo trickery to handle\n races, and this one test makes up 40% of the runtime of the total\n script.\n\nInstead, let's just hard-code the response that's of interest to us.\nThat will test exactly what we want for every run.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 51 +++++++++++++-------------------------------\n 1 file changed, 15 insertions(+), 36 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex ce7a9f1594..4e21ab60bf 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -300,44 +300,23 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n-test_lazy_prereq GIT_DAEMON '\n-\ttest_bool_env GIT_TEST_GIT_DAEMON true\n-'\n-\n-# This test spawns a daemon, so run it only if the user would be OK with\n-# testing with git-daemon.\n-test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n-\ttest_set_port JGIT_DAEMON_PORT &&\n-\tJGIT_DAEMON_PID= &&\n-\tgit init --bare empty.git &&\n-\t>empty.git/git-daemon-export-ok &&\n-\tmkfifo jgit_daemon_output &&\n-\t{\n-\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n-\t\tJGIT_DAEMON_PID=$!\n-\t} &&\n-\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n-\t{\n-\t\tread line &&\n-\t\tcase $line in\n-\t\tExporting*)\n-\t\t\t;;\n-\t\t*)\n-\t\t\techo \"Expected: Exporting\" &&\n-\t\t\tfalse;;\n-\t\tesac &&\n-\t\tread line &&\n-\t\tcase $line in\n-\t\t\"Listening on\"*)\n-\t\t\t;;\n-\t\t*)\n-\t\t\techo \"Expected: Listening on\" &&\n-\t\t\tfalse;;\n-\t\tesac\n-\t} input.q <<-EOF &&\n+\t$oid capabilities^{}Qcaps-go-here\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n \t# --exit-code asks the command to exit with 2 when no\n \t# matching refs are found.\n-\ttest_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n+\ttest_expect_code 2 git ls-remote --exit-code --upload-pack=./cat-input .\n '\n \n test_expect_success 'ls-remote works outside repository' '\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475191","messageId":"20230412063433.GD1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 4/7] t5512: add v2 support for \"ls-remote --symref\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:34:33Z","receivedAt":"2023-04-12T06:34:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n2019-02-25) configured this test to always run with protocol v0, since\nthe output is different for v2.\n\nBut that means we are not getting any test coverage of the feature with\nv2 at all. We could obviously switch to using and expecting v2, but then\nthat leaves v0 behind (and while we don't use it by default, it's still\nimportant for testing interoperability with older servers). Likewise, we\ncould switch the expected output based on $GIT_TEST_PROTOCOL_VERSION,\nbut hardly anybody runs the tests for v0 these days.\n\nInstead, let's explicitly run it for both protocol versions to make sure\nthey're well behaved. This matches other similar tests added later in\n6a139cdd74 (ls-remote: pass heads/tags prefixes to transport,\n2018-10-31), etc.\n\nSigned-off-by: Jeff King \n---\nI wondered briefly if anybody ever runs with GIT_TEST_PROTOCOL_VERSION=0,\nbut I'm pretty sure the answer is \"no\", because a bunch of test scripts\nfail. Those are almost certainly all non-bugs, but rather just tests\nthat assume modern v2 behavior. I'm not sure if it's worth putting any\neffort into fixing. I didn't do so here (though before and after my\npatches t5512 itself runs OK in either mode).\n\n t/t5512-ls-remote.sh | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 4e21ab60bf..74f0204c5b 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -244,22 +244,25 @@ test_expect_success 'protocol v2 supports hiderefs' '\n \n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n-\techo \"ref: refs/heads/main\tHEAD\" >expect &&\n+\techo \"ref: refs/heads/main\tHEAD\" >expect.v2 &&\n \tgenerate_references \\\n \t\tHEAD \\\n-\t\trefs/heads/main >>expect &&\n+\t\trefs/heads/main >>expect.v2 &&\n+\techo \"ref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\" >>expect.v2 &&\n \toid=$(git rev-parse HEAD) &&\n-\techo \"$oid\trefs/remotes/origin/HEAD\" >>expect &&\n+\techo \"$oid\trefs/remotes/origin/HEAD\" >>expect.v2 &&\n \tgenerate_references \\\n \t\trefs/remotes/origin/main \\\n \t\trefs/tags/mark \\\n \t\trefs/tags/mark1.1 \\\n \t\trefs/tags/mark1.10 \\\n-\t\trefs/tags/mark1.2 >>expect &&\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&\n-\ttest_cmp expect actual\n+\t\trefs/tags/mark1.2 >>expect.v2 &&\n+\t# v0 does not show non-HEAD symrefs\n+\tgrep -v \"ref: refs/remotes\" expect.v0 &&\n+\tgit -c protocol.version=0 ls-remote --symref >actual.v0 &&\n+\ttest_cmp expect.v0 actual.v0 &&\n+\tgit -c protocol.version=2 ls-remote --symref >actual.v2 &&\n+\ttest_cmp expect.v2 actual.v2\n '\n \n test_expect_success 'ls-remote with filtered symref (refname)' '\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475192","messageId":"20230412063543.GE1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 5/7] t5512: allow any protocol version for filtered symref test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:35:43Z","receivedAt":"2023-04-12T06:35:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a test that checks that ls-remote, when asked only about HEAD,\nwill report the HEAD symref, and not others. This was marked to always\nrun with the v0 protocol by b2f73b70b2 (t5512: compensate for v0 only\nsending HEAD symrefs, 2019-02-25).\n\nBut in v0 this test is doing nothing! For v0, upload-pack only reports\nthe HEAD symref anyway, so we'd never have any other symref to report.\n\nFor v2, it is useful; we learn about all symrefs (and the test repo has\nmultiple), so this demonstrates that we correctly avoid showing them.\n\nWe could perhaps mark this to test explicitly with v2, but since that is\nthe default these days, it's sufficient to just run ls-remote without\nany protocol specification. It still passes if somebody does an explicit\nGIT_TEST_PROTOCOL_VERSION=0; it's just testing less.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 74f0204c5b..1ac3b50ca5 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -271,9 +271,7 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \tref: refs/heads/main\tHEAD\n \t$rev\tHEAD\n \tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . HEAD >actual &&\n+\tgit ls-remote --symref . HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475193","messageId":"20230412063719.GF1681312@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 6/7] t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:37:19Z","receivedAt":"2023-04-12T06:37:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two overlapping tests for checking the behavior of \"ls-remote\n--symref\" when filtering output. The first test checks that using\n\"--heads\" will omit the symref for HEAD (since we don't print anything\nabout HEAD at all), but still prints other symrefs.\n\nThis has been marked as expecting failure since it was added in\n99c08d4eb2 (ls-remote: add support for showing symrefs, 2016-01-19).\nThat's because back then, we only had the v0 protocol, and it only\nreported on the HEAD symref, not others. But these days we have v2,\nwhich does exactly what the test wants. It would even have started\nunexpectedly passing when we switched to v2 by default, except that\nb2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n2019-02-25) over-zealously marked it to run only in v0 mode.\n\nSo let's run it with both protocol versions, and adjust the expected\noutput for each. It passes in v2 without modification. In v0 mode, we'll\ndrop the extra symref, but this is still testing something useful: it\nensures that we do omit HEAD.\n\nThe test after this checks \"--heads\" again, this time using the expected\nv0 output. That's now redundant. It also checks that limiting with a\npattern like \"refs/heads/*\" works similarly, but that's redundant with a\ntest earlier in the script which limits by HEAD (again, back then the\n\"HEAD\" test was less interesting because there were no other symrefs to\nomit, but in a modern v2 world, there are). So we can just delete that\nsecond test entirely.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 26 +++++++-------------------\n 1 file changed, 7 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 1ac3b50ca5..92cb9fed78 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -275,30 +275,18 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'ls-remote with filtered symref (--heads)' '\n+test_expect_success 'ls-remote with filtered symref (--heads)' '\n \tgit symbolic-ref refs/heads/foo refs/tags/mark &&\n-\tcat >expect <<-EOF &&\n+\tcat >expect.v2 <<-EOF &&\n \tref: refs/tags/mark\trefs/heads/foo\n \t$rev\trefs/heads/foo\n \t$rev\trefs/heads/main\n \tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ls-remote --symref omits filtered-out matches' '\n-\tcat >expect <<-EOF &&\n-\t$rev\trefs/heads/foo\n-\t$rev\trefs/heads/main\n-\tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n-\ttest_cmp expect actual &&\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . \"refs/heads/*\" >actual &&\n-\ttest_cmp expect actual\n+\tgrep -v \"^ref: refs/tags/\" expect.v0 &&\n+\tgit -c protocol.version=0 ls-remote --symref --heads . >actual.v0 &&\n+\ttest_cmp expect.v0 actual.v0 &&\n+\tgit -c protocol.version=2 ls-remote --symref --heads . >actual.v2 &&\n+\ttest_cmp expect.v2 actual.v2\n '\n \n test_expect_success 'indicate no refs in v0 standards-compliant empty remote' '\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475194","messageId":"20230412064045.GA1681575@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062300.GA838367@coredump.intra.peff.net","subject":"[PATCH 7/7] v0 protocol: use size_t for capability length/offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:40:45Z","receivedAt":"2023-04-12T06:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing server capabilities, we use \"int\" to store lengths and\noffsets. At first glance this seems like a spot where our parser may be\nconfused by integer overflow if somebody sent us a malicious response.\n\nIn practice these strings are all bounded by the 64k limit of a\npkt-line, so using \"int\" is OK. However, it makes the code simpler to\naudit if they just use size_t everywhere. Note that because we take\nthese parameters as pointers, this also forces many callers to update\ntheir declared types.\n\nSigned-off-by: Jeff King \n---\nI guess you could argue that \"int\" is fine here, given the context.\nMostly I just did a double-take while looking at the code, so I thought\nthis might save the next person from doing the same.\n\n builtin/receive-pack.c | 2 +-\n connect.c | 22 +++++++++++-----------\n connect.h | 4 ++--\n fetch-pack.c | 4 ++--\n send-pack.c | 2 +-\n transport.c | 2 +-\n upload-pack.c | 2 +-\n 7 files changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9109552533..3b495ecc84 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2093,7 +2093,7 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n \t\t\tconst char *client_sid;\n-\t\t\tint len = 0;\n+\t\t\tsize_t len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n \t\t\tif (parse_feature_request(feature_list, \"report-status-v2\"))\ndiff --git a/connect.c b/connect.c\nindex 5e265ba9d7..5685551cf0 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -22,7 +22,7 @@\n \n static char *server_capabilities_v1;\n static struct strvec server_capabilities_v2 = STRVEC_INIT;\n-static const char *next_server_feature_value(const char *feature, int *len, int *offset);\n+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset);\n \n static int check_ref(const char *name, unsigned int flags)\n {\n@@ -205,10 +205,10 @@ static void parse_one_symref_info(struct string_list *symref, const char *val, i\n static void annotate_refs_with_symref_info(struct ref *ref)\n {\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n-\tint offset = 0;\n+\tsize_t offset = 0;\n \n \twhile (1) {\n-\t\tint len;\n+\t\tsize_t len;\n \t\tconst char *val;\n \n \t\tval = next_server_feature_value(\"symref\", &len, &offset);\n@@ -231,7 +231,7 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n static void process_capabilities(struct packet_reader *reader, int *linelen)\n {\n \tconst char *feat_val;\n-\tint feat_len;\n+\tsize_t feat_len;\n \tconst char *line = reader->line;\n \tint nul_location = strlen(line);\n \tif (nul_location == *linelen)\n@@ -595,9 +595,9 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \treturn list;\n }\n \n-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset)\n {\n-\tint len;\n+\tsize_t len;\n \n \tif (!feature_list)\n \t\treturn NULL;\n@@ -621,7 +621,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t}\n \t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n \t\t\telse if (*value == '=') {\n-\t\t\t\tint end;\n+\t\t\t\tsize_t end;\n \n \t\t\t\tvalue++;\n \t\t\t\tend = strcspn(value, \" \\t\\n\");\n@@ -643,8 +643,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \n int server_supports_hash(const char *desired, int *feature_supported)\n {\n-\tint offset = 0;\n-\tint len;\n+\tsize_t offset = 0;\n+\tsize_t len;\n \tconst char *hash;\n \n \thash = next_server_feature_value(\"object-format\", &len, &offset);\n@@ -668,12 +668,12 @@ int parse_feature_request(const char *feature_list, const char *feature)\n \treturn !!parse_feature_value(feature_list, feature, NULL, NULL);\n }\n \n-static const char *next_server_feature_value(const char *feature, int *len, int *offset)\n+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset)\n {\n \treturn parse_feature_value(server_capabilities_v1, feature, len, offset);\n }\n \n-const char *server_feature_value(const char *feature, int *len)\n+const char *server_feature_value(const char *feature, size_t *len)\n {\n \treturn parse_feature_value(server_capabilities_v1, feature, len, NULL);\n }\ndiff --git a/connect.h b/connect.h\nindex f41a0b4c1f..1645126c17 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -12,14 +12,14 @@ int finish_connect(struct child_process *conn);\n int git_connection_is_socket(struct child_process *conn);\n int server_supports(const char *feature);\n int parse_feature_request(const char *features, const char *feature);\n-const char *server_feature_value(const char *feature, int *len_ret);\n+const char *server_feature_value(const char *feature, size_t *len_ret);\n int url_is_local_not_ssh(const char *url);\n \n struct packet_reader;\n enum protocol_version discover_version(struct packet_reader *reader);\n \n int server_supports_hash(const char *desired, int *feature_supported);\n-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset);\n+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset);\n int server_supports_v2(const char *c);\n void ensure_server_supports_v2(const char *c);\n int server_feature_v2(const char *c, const char **v);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 368f2ed25a..97a44ed582 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1099,7 +1099,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tstruct object_id oid;\n \tconst char *agent_feature;\n-\tint agent_len;\n+\tsize_t agent_len;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n@@ -1117,7 +1117,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tagent_supported = 1;\n \t\tif (agent_len)\n \t\t\tprint_verbose(args, _(\"Server version is %.*s\"),\n-\t\t\t\t agent_len, agent_feature);\n+\t\t\t\t (int)agent_len, agent_feature);\n \t}\n \n \tif (!server_supports(\"session-id\"))\ndiff --git a/send-pack.c b/send-pack.c\nindex f81bbb28d7..97344b629e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -538,7 +538,7 @@ int send_pack(struct send_pack_args *args,\n \t\tdie(_(\"the receiving end does not support this repository's hash algorithm\"));\n \n \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n-\t\tint len;\n+\t\tsize_t len;\n \t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n \t\tif (push_cert_nonce) {\n \t\t\treject_invalid_nonce(push_cert_nonce, len);\ndiff --git a/transport.c b/transport.c\nindex 89a220425e..6223dc3de2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -317,7 +317,7 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n-\tint sid_len;\n+\tsize_t sid_len;\n \tconst char *server_sid;\n \n \tconnect_setup(transport, for_push);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e23f16dfdd..565e46058f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1067,7 +1067,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n \t\tconst char *arg;\n-\t\tint feature_len;\n+\t\tsize_t feature_len;\n \n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n-- \n2.40.0.493.gfc602f1919\n"},{"id":"475195","messageId":"20230412064651.GA1681676@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412062924.GA1681312@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T06:46:51Z","receivedAt":"2023-04-12T06:47:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2023 at 02:29:24AM -0400, Jeff King wrote:\n\n> But on the second call, now *offset is set to that larger index, which\n> lets us skip past the first \"symref\" capability. However, we do so by\n> incrementing feature_list. That means our pointer difference is now too\n> small; it is counting from where we resumed parsing, not from the start\n> of the original feature_list pointer. And because we incremented\n> feature_list only inside our function, and not the caller, that\n> increment is lost next time the function is called.\n> \n> The simplest solution is to account for those skipped bytes by\n> incrementing *offset, rather than assigning to it. (The other possible\n> solution is to add an extra level of pointer indirection to feature_list\n> so that the caller knows we moved it forward, but that seems more\n> complicated).\n\nHmph. So after convincing myself that was the end of it, now I'm not so\nsure. We also increment feature_list if we find a false positive in the\nmiddle of another entry. I.e., the code even after this patch looks\nlike:\n\n while (*feature_list) {\n const char *found = strstr(feature_list, feature);\n if (!found)\n return NULL;\n if (feature_list == found || isspace(found[-1])) {\n const char *value = found + len;\n /* feature with no value (e.g., \"thin-pack\") */\n if (!*value || isspace(*value)) {\n if (lenp)\n *lenp = 0;\n if (offset)\n *offset += found + len - feature_list;\n return value;\n }\n /* feature with a value (e.g., \"agent=git/1.2.3\") */\n else if (*value == '=') {\n size_t end;\n \n value++;\n end = strcspn(value, \" \\t\\n\");\n if (lenp)\n *lenp = end;\n if (offset)\n *offset += value + end - feature_list;\n return value;\n }\n /*\n * otherwise we matched a substring of another feature;\n * keep looking\n */\n }\n feature_list = found + 1;\n }\n return NULL;\n\nSo if we have something like:\n\n agent=i-like-symrefs symref=HEAD:refs/heads/foo\n\nthen we'd find the \"symref\" value in the agent line, increment\nfeature_list, and then find the real one. But our pointer difference\nwill again be too short! And incrementing \"offset\" rather than assigning\nit won't help, because those skipped bytes are not accounted for in the\nexisting value of \"offset\".\n\nSo what we probably want is a third possibility I didn't allow for: keep\nthe original value of feature_list intact, and use a separate pointer to\nincrement. And then assigning \"*offset = value + end - feature_list\"\nwill always be correct, because the offset will always be from the\noriginal, true beginning of the string.\n\nThe fix is easy, but let me see if I can come up with a test.\n\n-Peff\n"},{"id":"475197","messageId":"20230412072510.GA1694877@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412064651.GA1681676@coredump.intra.peff.net","subject":"[PATCH v2 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T07:25:10Z","receivedAt":"2023-04-12T07:25:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2023 at 02:46:51AM -0400, Jeff King wrote:\n\n> So if we have something like:\n> \n> agent=i-like-symrefs symref=HEAD:refs/heads/foo\n> \n> then we'd find the \"symref\" value in the agent line, increment\n> feature_list, and then find the real one. But our pointer difference\n> will again be too short! And incrementing \"offset\" rather than assigning\n> it won't help, because those skipped bytes are not accounted for in the\n> existing value of \"offset\".\n> \n> So what we probably want is a third possibility I didn't allow for: keep\n> the original value of feature_list intact, and use a separate pointer to\n> increment. And then assigning \"*offset = value + end - feature_list\"\n> will always be correct, because the offset will always be from the\n> original, true beginning of the string.\n> \n> The fix is easy, but let me see if I can come up with a test.\n\nOK, this is indeed buggy, but I'm not sure you can actually get it to\nproduce a user-visible result with the incrementing patch applied.\n\nThe problem is that \"offset\" is incremented by too-small an amount. So\nin the example above, we might end up partway through the value of\n\"symref=\", rather than at the end. In the worst case we might see the\nsame \"symref=\" entry again, which would cause us to attach it to the\nlist a second time, leaking the memory. But either way we always make\nforward progress through the string, so we'd never loop infinitely, nor\nproduce a wrong answer (we can never _skip_ an entry, only accidentally\nprocess it twice).\n\nSo I think it's worth fixing, and even worth beefing up the test to\ninclude this case, but sadly there's no specific case to check for. So I\njust rolled it into the fix in patch 1.\n\nHere's that patch (and I'll send a range-diff in a second). The rest are\nthe same, modulo a small textual conflict in patch 7 that touches a\nnearby line, but I'll refrain from re-sending the rest of them until\nI've gotten more review.\n\n-- >8 --\nSubject: v0 protocol: fix infinite loop when parsing multi-valued capabilities\n\nIf Git's client-side parsing of an upload-pack response (so git-fetch or\nls-remote) sees multiple instances of a single capability, it can enter\nan infinite loop due to a bug in advancing the \"offset\" parameter in the\nparser.\n\nThis bug can't happen between a client and server of the same Git\nversion. The client bug is in parse_feature_value() when the caller\npasses in an offset parameter. And that only happens when the v0\nprotocol is parsing \"symref\" and \"object-format\" capabilities, via\nnext_server_feature_value(). But Git has never produced multiple\nobject-format capabilities, and it stopped producing multiple symref\nvalues in d007dbf7d6 (Revert \"upload-pack: send non-HEAD symbolic refs\",\n2013-11-18).\n\nHowever, upload-pack did produce multiple symref entries for a while,\nand they are valid. Plus other implementations, such as Dulwich will\nstill do so. So we should handle them. And even if we do not expect it,\nit is obviously a bug for the parser to enter an infinite loop.\n\nThe bug itself is pretty simple. Commit 2c6a403d96 (connect: add\nfunction to parse multiple v1 capability values, 2020-05-25) added the\n\"offset\" parameter, which is used as both an in- and out-parameter. When\nparsing the first \"symref\" capability, *offset will be 0 on input, and\nafter parsing the capability, we set *offset to an index just past the\nvalue by taking a pointer difference \"(value + end) - feature_list\".\n\nBut on the second call, now *offset is set to that larger index, which\nlets us skip past the first \"symref\" capability. However, we do so by\nincrementing feature_list. That means our pointer difference is now too\nsmall; it is counting from where we resumed parsing, not from the start\nof the original feature_list pointer. And because we incremented\nfeature_list only inside our function, and not the caller, that\nincrement is lost next time the function is called.\n\nOne solution would be to account for those skipped bytes by incrementing\n*offset, rather than assigning to it. But wait, there's more!\n\nWe also increment feature_list if we have a near-miss. Say we are\nlooking for \"symref\" and find \"almost-symref\". In that case we'll point\nfeature_list to the \"y\" in \"almost-symref\" and restart our search. But\nthat again means our offset won't be correct, as it won't account for\nthe bytes between the start of the string and that \"y\".\n\nSo instead, let's just record the beginning of the feature_list string\nin a separate pointer that we never touch. That offset we take in and\nreturn is meant to be using that point as a base, and now we'll do so\nconsistently.\n\nSince the bug can't be reproduced using the current version of\ngit-upload-pack, we'll instead hard-code an input which triggers the\nproblem. Before this patch it loops forever re-parsing the second symref\nentry. Now we check both that it finishes, and that it parses both\nentries correctly (a case we could not test at all before).\n\nWe don't need to worry about testing v2 here; it communicates the\ncapabilities in a completely different way, and doesn't use this code at\nall. There are tests earlier in t5512 that are meant to cover this (they\ndon't, but we'll address that in a future patch).\n\nReported-by: Jonas Haag \nSigned-off-by: Jeff King \n---\n connect.c | 5 +++--\n t/t5512-ls-remote.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0c8a38178..0dc739c4e5 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -597,6 +597,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \n const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n {\n+\tconst char *orig_start = feature_list;\n \tint len;\n \n \tif (!feature_list)\n@@ -616,7 +617,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\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\t\t*offset = found + len - orig_start;\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@@ -628,7 +629,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = end;\n \t\t\t\tif (offset)\n-\t\t\t\t\t*offset = value + end - feature_list;\n+\t\t\t\t\t*offset = value + end - orig_start;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/*\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 20d063fb9a..cab67282df 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -15,6 +15,19 @@ generate_references () {\n \tdone\n }\n \n+test_expect_success 'set up fake upload-pack' '\n+\t# This can be used to simulate an upload-pack that just shows the\n+\t# contents of the \"input\" file (prepared with the test-tool pkt-line\n+\t# helper), and does not do any negotiation (since ls-remote does not\n+\t# need it).\n+\twrite_script cat-input <<-\\EOF\n+\t# send our initial advertisement/response\n+\tcat input\n+\t# soak up the flush packet from the client\n+\tcat\n+\tEOF\n+'\n+\n test_expect_success 'dies when no remote found' '\n \ttest_must_fail git ls-remote\n '\n@@ -360,4 +373,43 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \ttest_cmp expect actual.v2\n '\n \n+test_expect_success 'v0 clients can handle multiple symrefs' '\n+\t# Modern versions of Git will not return multiple symref capabilities\n+\t# for v0, so we have to hard-code the response. Note that we will\n+\t# always use both v0 and object-format=sha1 here, as the hard-coded\n+\t# response reflects a server that only supports those.\n+\toid=1234567890123456789012345678901234567890 &&\n+\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n+\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n+\n+\t# Likewise we want to make sure our parser is not fooled by the string\n+\t# \"symref\" appearing as part of an earlier cap. But there is no way to\n+\t# do that via upload-pack, as arbitrary strings can appear only in a\n+\t# \"symref\" value itself (where we skip past the values as a whole)\n+\t# and \"agent\" (which always appears after \"symref\", so putting our\n+\t# parser in a confused state is less interesting).\n+\tcaps=\"some other caps including a-fake-symref-cap\" &&\n+\n+\ttest-tool pkt-line pack >input.q <<-EOF &&\n+\t$oid HEADQ$caps $symrefs\n+\t$oid refs/heads/main\n+\t$oid refs/remotes/origin/HEAD\n+\t$oid refs/remotes/origin/main\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n+\tcat >expect <<-EOF &&\n+\tref: refs/heads/main\tHEAD\n+\t$oid\tHEAD\n+\t$oid\trefs/heads/main\n+\tref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/main\n+\tEOF\n+\n+\tgit ls-remote --symref --upload-pack=./cat-input . >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.493.gfc602f1919\n\n"},{"id":"475198","messageId":"20230412072622.GA1695073@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412072510.GA1694877@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T07:26:22Z","receivedAt":"2023-04-12T07:26:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2023 at 03:25:10AM -0400, Jeff King wrote:\n\n> So I think it's worth fixing, and even worth beefing up the test to\n> include this case, but sadly there's no specific case to check for. So I\n> just rolled it into the fix in patch 1.\n> \n> Here's that patch (and I'll send a range-diff in a second). The rest are\n> the same, modulo a small textual conflict in patch 7 that touches a\n> nearby line, but I'll refrain from re-sending the rest of them until\n> I've gotten more review.\n\nAnd here's the range-diff.\n\n1: c379efdcc0 ! 1: 3958a89100 v0 protocol: fix infinite loop when parsing multi-valued capabilities\n @@ Commit message\n feature_list only inside our function, and not the caller, that\n increment is lost next time the function is called.\n \n - The simplest solution is to account for those skipped bytes by\n - incrementing *offset, rather than assigning to it. (The other possible\n - solution is to add an extra level of pointer indirection to feature_list\n - so that the caller knows we moved it forward, but that seems more\n - complicated).\n + One solution would be to account for those skipped bytes by incrementing\n + *offset, rather than assigning to it. But wait, there's more!\n +\n + We also increment feature_list if we have a near-miss. Say we are\n + looking for \"symref\" and find \"almost-symref\". In that case we'll point\n + feature_list to the \"y\" in \"almost-symref\" and restart our search. But\n + that again means our offset won't be correct, as it won't account for\n + the bytes between the start of the string and that \"y\".\n +\n + So instead, let's just record the beginning of the feature_list string\n + in a separate pointer that we never touch. That offset we take in and\n + return is meant to be using that point as a base, and now we'll do so\n + consistently.\n \n Since the bug can't be reproduced using the current version of\n git-upload-pack, we'll instead hard-code an input which triggers the\n @@ Commit message\n Signed-off-by: Jeff King \n \n ## connect.c ##\n +@@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n + \n + const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n + {\n ++\tconst char *orig_start = feature_list;\n + \tint len;\n + \n + \tif (!feature_list)\n @@ connect.c: const char *parse_feature_value(const char *feature_list, const char *feature, i\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\t\t*offset += found + len - feature_list;\n ++\t\t\t\t\t*offset = found + len - orig_start;\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 @@ connect.c: const char *parse_feature_value(const char *feature_list, const char\n \t\t\t\t\t*lenp = end;\n \t\t\t\tif (offset)\n -\t\t\t\t\t*offset = value + end - feature_list;\n -+\t\t\t\t\t*offset += value + end - feature_list;\n ++\t\t\t\t\t*offset = value + end - orig_start;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/*\n @@ t/t5512-ls-remote.sh: test_expect_success 'ls-remote prefixes work with all prot\n +\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n +\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n +\n ++\t# Likewise we want to make sure our parser is not fooled by the string\n ++\t# \"symref\" appearing as part of an earlier cap. But there is no way to\n ++\t# do that via upload-pack, as arbitrary strings can appear only in a\n ++\t# \"symref\" value itself (where we skip past the values as a whole)\n ++\t# and \"agent\" (which always appears after \"symref\", so putting our\n ++\t# parser in a confused state is less interesting).\n ++\tcaps=\"some other caps including a-fake-symref-cap\" &&\n ++\n +\ttest-tool pkt-line pack >input.q <<-EOF &&\n -+\t$oid HEADQ$symrefs\n ++\t$oid HEADQ$caps $symrefs\n +\t$oid refs/heads/main\n +\t$oid refs/remotes/origin/HEAD\n +\t$oid refs/remotes/origin/main\n2: 2a8e6943e5 = 2: 4cb0739770 t5512: stop referring to \"v1\" protocol\n3: 7e411ed56b = 3: 4fe3be0476 t5512: stop using jgit for capabilities^{} test\n4: 65387a02bf = 4: 77875ff7f6 t5512: add v2 support for \"ls-remote --symref\" test\n5: 3409bd94b0 = 5: 642608ac2d t5512: allow any protocol version for filtered symref test\n6: c42a25ceca = 6: 20c78c9e41 t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2\n7: 86067753cb ! 7: 48a46fc6fc v0 protocol: use size_t for capability length/offset\n @@ connect.c: struct ref **get_remote_refs(int fd_out, struct packet_reader *reader\n -const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n +const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset)\n {\n + \tconst char *orig_start = feature_list;\n -\tint len;\n +\tsize_t len;\n \n"},{"id":"475202","messageId":"20230412090423.GA2187240@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412063118.GC1681312@coredump.intra.peff.net","subject":"Re: [PATCH 3/7] t5512: stop using jgit for capabilities^{} test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-12T09:04:23Z","receivedAt":"2023-04-12T09:04:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2023 at 02:31:18AM -0400, Jeff King wrote:\n\n> Commit eb398797cd (connect: advertized capability is not a ref,\n> 2016-09-09) added support for an upload-pack server responding with:\n> \n> 0000000000000000000000000000000000000000 capabilities^{}\n> \n> followed by a NUL and capabilities. This is not something Git itself has\n> ever produced for upload-pack, but JGit does. And hence the test used\n> JGit to reproduce the real-world situation. That was good for verifying\n> that the incompatibility was fixed, but it's a lousy regression test for\n> a few reasons:\n> \n> - hardly anybody runs it, because you have to have jgit installed\n> \n> - we're depending on jgit's behavior for the test to do anything\n> useful. In particular, this behavior is only relevant to the v0\n> protocol, but these days we ask for the v2 protocol by default. So\n> for modern jgit, this is probably testing nothing.\n\nI was worried that changing this one might be churn. But as it turns\nout, it reveals that there's a bug in the code which we've been missing\nall this time because nobody was running the test!\n\nWhen run with GIT_TEST_DEFAULT_HASH=sha256, this will fail because the\ncode uses the local idea of the hash algorithm, rather than what the\nother side advertises. We have a linux-sha256 CI job, but of course it\ndidn't have jgit installed, so it always skipped this test (until my\npatch, where it now reveals the bug).\n\nThe fix is just:\n\ndiff --git a/connect.c b/connect.c\nindex 66397cc911..c54adc652f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -263,7 +263,8 @@ static int process_dummy_ref(const struct packet_reader *reader)\n \t\treturn 0;\n \tname++;\n \n-\treturn oideq(null_oid(), &oid) && !strcmp(name, \"capabilities^{}\");\n+\treturn oideq(reader->hash_algo->null_oid, &oid) &&\n+\t\t!strcmp(name, \"capabilities^{}\");\n }\n \n static void check_no_capabilities(const char *line, int len)\n\nI'll squash that in and update the commit message before I do the next\nre-roll (but will still hold off a bit to get further comments).\n\n-Peff\n"},{"id":"475416","messageId":"20230414212404.GA639653@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230412090423.GA2187240@coredump.intra.peff.net","subject":"[PATCH v3 0/7] v0 multiple-symref infinite loop fix and test cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:24:04Z","receivedAt":"2023-04-14T21:24:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2023 at 05:04:23AM -0400, Jeff King wrote:\n\n> I'll squash that in and update the commit message before I do the next\n> re-roll (but will still hold off a bit to get further comments).\n\nNobody said anything, so I assume the rest of the series is perfect. ;)\n\nJunio, I see that you picked up this fix as a \"squash\", along with my\nother \"v2\" update. Here's a v3 that does the actual squash along with a\ncommit message update. That ties up all loose ends from my perspective,\nbut of course if anybody has review comments, please send them.\n\nThe range-diff against what you have in jk/protocol-cap-parse-fix\n(after squashing) is below.\n\n [1/7]: v0 protocol: fix infinite loop when parsing multi-valued capabilities\n [2/7]: t5512: stop referring to \"v1\" protocol\n [3/7]: v0 protocol: fix sha1/sha256 confusion for capabilities^{}\n [4/7]: t5512: add v2 support for \"ls-remote --symref\" test\n [5/7]: t5512: allow any protocol version for filtered symref test\n [6/7]: t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2\n [7/7]: v0 protocol: use size_t for capability length/offset\n\n builtin/receive-pack.c | 2 +-\n connect.c | 30 ++++----\n connect.h | 4 +-\n fetch-pack.c | 4 +-\n send-pack.c | 2 +-\n t/t5512-ls-remote.sh | 156 +++++++++++++++++++++++------------------\n transport.c | 2 +-\n upload-pack.c | 2 +-\n 8 files changed, 112 insertions(+), 90 deletions(-)\n\n1: 5471cf388b = 1: 117f371be1 v0 protocol: fix infinite loop when parsing multi-valued capabilities\n2: eb3e6e6d1c = 2: de695291b0 t5512: stop referring to \"v1\" protocol\n3: c77b8ae4a0 ! 3: 515149d67a t5512: stop using jgit for capabilities^{} test\n @@ Metadata\n Author: Jeff King \n \n ## Commit message ##\n - t5512: stop using jgit for capabilities^{} test\n + v0 protocol: fix sha1/sha256 confusion for capabilities^{}\n \n Commit eb398797cd (connect: advertized capability is not a ref,\n 2016-09-09) added support for an upload-pack server responding with:\n \n 0000000000000000000000000000000000000000 capabilities^{}\n \n - followed by a NUL and capabilities. This is not something Git itself has\n - ever produced for upload-pack, but JGit does. And hence the test used\n - JGit to reproduce the real-world situation. That was good for verifying\n - that the incompatibility was fixed, but it's a lousy regression test for\n - a few reasons:\n + followed by a NUL and the actual capabilities. We correctly parse the\n + oid using the packet_reader's hash_algo field, but then we compare it to\n + null_oid(), which will instead use our current repo's default algorithm.\n + If we're defaulting to sha256 locally but the other side is sha1, they\n + won't match and we'll fail to parse the line (and thus die()).\n \n - - hardly anybody runs it, because you have to have jgit installed\n + This can cause a test failure when the suite is run with\n + GIT_TEST_DEFAULT_HASH=sha256, and we even do so regularly via the\n + linux-sha256 CI job. But since the test requires JGit to run, it's\n + usually just skipped, and nobody noticed the problem.\n +\n + The reason the original patch used JGit is that Git itself does not ever\n + produce such a line via upload-pack; the feature was added to fix a\n + real-world problem when interacting with JGit. That was good for\n + verifying that the incompatibility was fixed, but it's not a good\n + regression test:\n +\n + - hardly anybody runs it, because you have to have jgit installed;\n + hence this bug going unnoticed\n \n - we're depending on jgit's behavior for the test to do anything\n useful. In particular, this behavior is only relevant to the v0\n @@ Commit message\n script.\n \n Instead, let's just hard-code the response that's of interest to us.\n - That will test exactly what we want for every run.\n + That will test exactly what we want for every run, and reveals the bug\n + when run in sha256 mode. And of course we'll fix the actual bug by using\n + the correct hash_algo struct.\n \n Signed-off-by: Jeff King \n \n4: 8db5b3c3bf = 4: 152d904a4a t5512: add v2 support for \"ls-remote --symref\" test\n5: f1cd63e16e = 5: 87053ab90b t5512: allow any protocol version for filtered symref test\n6: b6b9d1ad44 = 6: 37d300d244 t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2\n7: 870d6e0a3b = 7: 4db6853ea2 v0 protocol: use size_t for capability length/offset\n"},{"id":"475417","messageId":"20230414212416.GA639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 1/7] v0 protocol: fix infinite loop when parsing multi-valued capabilities","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:24:16Z","receivedAt":"2023-04-14T21:24:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If Git's client-side parsing of an upload-pack response (so git-fetch or\nls-remote) sees multiple instances of a single capability, it can enter\nan infinite loop due to a bug in advancing the \"offset\" parameter in the\nparser.\n\nThis bug can't happen between a client and server of the same Git\nversion. The client bug is in parse_feature_value() when the caller\npasses in an offset parameter. And that only happens when the v0\nprotocol is parsing \"symref\" and \"object-format\" capabilities, via\nnext_server_feature_value(). But Git has never produced multiple\nobject-format capabilities, and it stopped producing multiple symref\nvalues in d007dbf7d6 (Revert \"upload-pack: send non-HEAD symbolic refs\",\n2013-11-18).\n\nHowever, upload-pack did produce multiple symref entries for a while,\nand they are valid. Plus other implementations, such as Dulwich will\nstill do so. So we should handle them. And even if we do not expect it,\nit is obviously a bug for the parser to enter an infinite loop.\n\nThe bug itself is pretty simple. Commit 2c6a403d96 (connect: add\nfunction to parse multiple v1 capability values, 2020-05-25) added the\n\"offset\" parameter, which is used as both an in- and out-parameter. When\nparsing the first \"symref\" capability, *offset will be 0 on input, and\nafter parsing the capability, we set *offset to an index just past the\nvalue by taking a pointer difference \"(value + end) - feature_list\".\n\nBut on the second call, now *offset is set to that larger index, which\nlets us skip past the first \"symref\" capability. However, we do so by\nincrementing feature_list. That means our pointer difference is now too\nsmall; it is counting from where we resumed parsing, not from the start\nof the original feature_list pointer. And because we incremented\nfeature_list only inside our function, and not the caller, that\nincrement is lost next time the function is called.\n\nOne solution would be to account for those skipped bytes by incrementing\n*offset, rather than assigning to it. But wait, there's more!\n\nWe also increment feature_list if we have a near-miss. Say we are\nlooking for \"symref\" and find \"almost-symref\". In that case we'll point\nfeature_list to the \"y\" in \"almost-symref\" and restart our search. But\nthat again means our offset won't be correct, as it won't account for\nthe bytes between the start of the string and that \"y\".\n\nSo instead, let's just record the beginning of the feature_list string\nin a separate pointer that we never touch. That offset we take in and\nreturn is meant to be using that point as a base, and now we'll do so\nconsistently.\n\nSince the bug can't be reproduced using the current version of\ngit-upload-pack, we'll instead hard-code an input which triggers the\nproblem. Before this patch it loops forever re-parsing the second symref\nentry. Now we check both that it finishes, and that it parses both\nentries correctly (a case we could not test at all before).\n\nWe don't need to worry about testing v2 here; it communicates the\ncapabilities in a completely different way, and doesn't use this code at\nall. There are tests earlier in t5512 that are meant to cover this (they\ndon't, but we'll address that in a future patch).\n\nReported-by: Jonas Haag \nSigned-off-by: Jeff King \n---\n connect.c | 5 +++--\n t/t5512-ls-remote.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex c0c8a38178..0dc739c4e5 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -597,6 +597,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \n const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n {\n+\tconst char *orig_start = feature_list;\n \tint len;\n \n \tif (!feature_list)\n@@ -616,7 +617,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\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\t\t*offset = found + len - orig_start;\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@@ -628,7 +629,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t\tif (lenp)\n \t\t\t\t\t*lenp = end;\n \t\t\t\tif (offset)\n-\t\t\t\t\t*offset = value + end - feature_list;\n+\t\t\t\t\t*offset = value + end - orig_start;\n \t\t\t\treturn value;\n \t\t\t}\n \t\t\t/*\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 20d063fb9a..cab67282df 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -15,6 +15,19 @@ generate_references () {\n \tdone\n }\n \n+test_expect_success 'set up fake upload-pack' '\n+\t# This can be used to simulate an upload-pack that just shows the\n+\t# contents of the \"input\" file (prepared with the test-tool pkt-line\n+\t# helper), and does not do any negotiation (since ls-remote does not\n+\t# need it).\n+\twrite_script cat-input <<-\\EOF\n+\t# send our initial advertisement/response\n+\tcat input\n+\t# soak up the flush packet from the client\n+\tcat\n+\tEOF\n+'\n+\n test_expect_success 'dies when no remote found' '\n \ttest_must_fail git ls-remote\n '\n@@ -360,4 +373,43 @@ test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \ttest_cmp expect actual.v2\n '\n \n+test_expect_success 'v0 clients can handle multiple symrefs' '\n+\t# Modern versions of Git will not return multiple symref capabilities\n+\t# for v0, so we have to hard-code the response. Note that we will\n+\t# always use both v0 and object-format=sha1 here, as the hard-coded\n+\t# response reflects a server that only supports those.\n+\toid=1234567890123456789012345678901234567890 &&\n+\tsymrefs=\"symref=refs/remotes/origin/HEAD:refs/remotes/origin/main\" &&\n+\tsymrefs=\"$symrefs symref=HEAD:refs/heads/main\" &&\n+\n+\t# Likewise we want to make sure our parser is not fooled by the string\n+\t# \"symref\" appearing as part of an earlier cap. But there is no way to\n+\t# do that via upload-pack, as arbitrary strings can appear only in a\n+\t# \"symref\" value itself (where we skip past the values as a whole)\n+\t# and \"agent\" (which always appears after \"symref\", so putting our\n+\t# parser in a confused state is less interesting).\n+\tcaps=\"some other caps including a-fake-symref-cap\" &&\n+\n+\ttest-tool pkt-line pack >input.q <<-EOF &&\n+\t$oid HEADQ$caps $symrefs\n+\t$oid refs/heads/main\n+\t$oid refs/remotes/origin/HEAD\n+\t$oid refs/remotes/origin/main\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n+\tcat >expect <<-EOF &&\n+\tref: refs/heads/main\tHEAD\n+\t$oid\tHEAD\n+\t$oid\trefs/heads/main\n+\tref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/HEAD\n+\t$oid\trefs/remotes/origin/main\n+\tEOF\n+\n+\tgit ls-remote --symref --upload-pack=./cat-input . >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475418","messageId":"20230414212419.GB639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 2/7] t5512: stop referring to \"v1\" protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:24:19Z","receivedAt":"2023-04-14T21:24:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There really isn't a \"v1\" Git protocol. It's just v0 with an extra probe\nwhich we used to test compatibility in preparation for v2. Any tests\nthat are looking for before/after behavior for v2 really care about\n\"v0\". Mentioning \"v1\" in these tests is just making things more\nconfusing, because we don't care about that probe; we're really testing\nv0. So let's say so.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex cab67282df..b868e20d7e 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -358,17 +358,17 @@ test_expect_success 'ls-remote --sort fails gracefully outside repository' '\n test_expect_success 'ls-remote patterns work with all protocol versions' '\n \tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n \t\trefs/heads/main refs/remotes/origin/main >expect &&\n-\tgit -c protocol.version=1 ls-remote . main >actual.v1 &&\n-\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=0 ls-remote . main >actual.v0 &&\n+\ttest_cmp expect actual.v0 &&\n \tgit -c protocol.version=2 ls-remote . main >actual.v2 &&\n \ttest_cmp expect actual.v2\n '\n \n test_expect_success 'ls-remote prefixes work with all protocol versions' '\n \tgit for-each-ref --format=\"%(objectname)\t%(refname)\" \\\n \t\trefs/heads/ refs/tags/ >expect &&\n-\tgit -c protocol.version=1 ls-remote --heads --tags . >actual.v1 &&\n-\ttest_cmp expect actual.v1 &&\n+\tgit -c protocol.version=0 ls-remote --heads --tags . >actual.v0 &&\n+\ttest_cmp expect actual.v0 &&\n \tgit -c protocol.version=2 ls-remote --heads --tags . >actual.v2 &&\n \ttest_cmp expect actual.v2\n '\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475419","messageId":"20230414212511.GC639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 3/7] v0 protocol: fix sha1/sha256 confusion for capabilities^{}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:25:11Z","receivedAt":"2023-04-14T21:25:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit eb398797cd (connect: advertized capability is not a ref,\n2016-09-09) added support for an upload-pack server responding with:\n\n 0000000000000000000000000000000000000000 capabilities^{}\n\nfollowed by a NUL and the actual capabilities. We correctly parse the\noid using the packet_reader's hash_algo field, but then we compare it to\nnull_oid(), which will instead use our current repo's default algorithm.\nIf we're defaulting to sha256 locally but the other side is sha1, they\nwon't match and we'll fail to parse the line (and thus die()).\n\nThis can cause a test failure when the suite is run with\nGIT_TEST_DEFAULT_HASH=sha256, and we even do so regularly via the\nlinux-sha256 CI job. But since the test requires JGit to run, it's\nusually just skipped, and nobody noticed the problem.\n\nThe reason the original patch used JGit is that Git itself does not ever\nproduce such a line via upload-pack; the feature was added to fix a\nreal-world problem when interacting with JGit. That was good for\nverifying that the incompatibility was fixed, but it's not a good\nregression test:\n\n - hardly anybody runs it, because you have to have jgit installed;\n hence this bug going unnoticed\n\n - we're depending on jgit's behavior for the test to do anything\n useful. In particular, this behavior is only relevant to the v0\n protocol, but these days we ask for the v2 protocol by default. So\n for modern jgit, this is probably testing nothing.\n\n - it's complicated and slow. We had to do some fifo trickery to handle\n races, and this one test makes up 40% of the runtime of the total\n script.\n\nInstead, let's just hard-code the response that's of interest to us.\nThat will test exactly what we want for every run, and reveals the bug\nwhen run in sha256 mode. And of course we'll fix the actual bug by using\nthe correct hash_algo struct.\n\nSigned-off-by: Jeff King \n---\n connect.c | 3 ++-\n t/t5512-ls-remote.sh | 51 +++++++++++++-------------------------------\n 2 files changed, 17 insertions(+), 37 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 0dc739c4e5..b7ba5f5727 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -263,7 +263,8 @@ static int process_dummy_ref(const struct packet_reader *reader)\n \t\treturn 0;\n \tname++;\n \n-\treturn oideq(null_oid(), &oid) && !strcmp(name, \"capabilities^{}\");\n+\treturn oideq(reader->hash_algo->null_oid, &oid) &&\n+\t\t!strcmp(name, \"capabilities^{}\");\n }\n \n static void check_no_capabilities(const char *line, int len)\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex b868e20d7e..6fb67ddce4 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -300,44 +300,23 @@ test_expect_success 'ls-remote --symref omits filtered-out matches' '\n \ttest_cmp expect actual\n '\n \n-test_lazy_prereq GIT_DAEMON '\n-\ttest_bool_env GIT_TEST_GIT_DAEMON true\n-'\n-\n-# This test spawns a daemon, so run it only if the user would be OK with\n-# testing with git-daemon.\n-test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '\n-\ttest_set_port JGIT_DAEMON_PORT &&\n-\tJGIT_DAEMON_PID= &&\n-\tgit init --bare empty.git &&\n-\t>empty.git/git-daemon-export-ok &&\n-\tmkfifo jgit_daemon_output &&\n-\t{\n-\t\tjgit daemon --port=\"$JGIT_DAEMON_PORT\" . >jgit_daemon_output &\n-\t\tJGIT_DAEMON_PID=$!\n-\t} &&\n-\ttest_when_finished kill \"$JGIT_DAEMON_PID\" &&\n-\t{\n-\t\tread line &&\n-\t\tcase $line in\n-\t\tExporting*)\n-\t\t\t;;\n-\t\t*)\n-\t\t\techo \"Expected: Exporting\" &&\n-\t\t\tfalse;;\n-\t\tesac &&\n-\t\tread line &&\n-\t\tcase $line in\n-\t\t\"Listening on\"*)\n-\t\t\t;;\n-\t\t*)\n-\t\t\techo \"Expected: Listening on\" &&\n-\t\t\tfalse;;\n-\t\tesac\n-\t} input.q <<-EOF &&\n+\t$oid capabilities^{}Qcaps-go-here\n+\t0000\n+\tEOF\n+\tq_to_nul input &&\n+\n \t# --exit-code asks the command to exit with 2 when no\n \t# matching refs are found.\n-\ttest_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git\n+\ttest_expect_code 2 git ls-remote --exit-code --upload-pack=./cat-input .\n '\n \n test_expect_success 'ls-remote works outside repository' '\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475420","messageId":"20230414212516.GE639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 5/7] t5512: allow any protocol version for filtered symref test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:25:16Z","receivedAt":"2023-04-14T21:25:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a test that checks that ls-remote, when asked only about HEAD,\nwill report the HEAD symref, and not others. This was marked to always\nrun with the v0 protocol by b2f73b70b2 (t5512: compensate for v0 only\nsending HEAD symrefs, 2019-02-25).\n\nBut in v0 this test is doing nothing! For v0, upload-pack only reports\nthe HEAD symref anyway, so we'd never have any other symref to report.\n\nFor v2, it is useful; we learn about all symrefs (and the test repo has\nmultiple), so this demonstrates that we correctly avoid showing them.\n\nWe could perhaps mark this to test explicitly with v2, but since that is\nthe default these days, it's sufficient to just run ls-remote without\nany protocol specification. It still passes if somebody does an explicit\nGIT_TEST_PROTOCOL_VERSION=0; it's just testing less.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex a34cab12ae..fbf23d12a6 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -271,9 +271,7 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \tref: refs/heads/main\tHEAD\n \t$rev\tHEAD\n \tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . HEAD >actual &&\n+\tgit ls-remote --symref . HEAD >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475421","messageId":"20230414212513.GD639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 4/7] t5512: add v2 support for \"ls-remote --symref\" test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:25:13Z","receivedAt":"2023-04-14T21:26:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit b2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n2019-02-25) configured this test to always run with protocol v0, since\nthe output is different for v2.\n\nBut that means we are not getting any test coverage of the feature with\nv2 at all. We could obviously switch to using and expecting v2, but then\nthat leaves v0 behind (and while we don't use it by default, it's still\nimportant for testing interoperability with older servers). Likewise, we\ncould switch the expected output based on $GIT_TEST_PROTOCOL_VERSION,\nbut hardly anybody runs the tests for v0 these days.\n\nInstead, let's explicitly run it for both protocol versions to make sure\nthey're well behaved. This matches other similar tests added later in\n6a139cdd74 (ls-remote: pass heads/tags prefixes to transport,\n2018-10-31), etc.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex 6fb67ddce4..a34cab12ae 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -244,22 +244,25 @@ test_expect_success 'protocol v2 supports hiderefs' '\n \n test_expect_success 'ls-remote --symref' '\n \tgit fetch origin &&\n-\techo \"ref: refs/heads/main\tHEAD\" >expect &&\n+\techo \"ref: refs/heads/main\tHEAD\" >expect.v2 &&\n \tgenerate_references \\\n \t\tHEAD \\\n-\t\trefs/heads/main >>expect &&\n+\t\trefs/heads/main >>expect.v2 &&\n+\techo \"ref: refs/remotes/origin/main\trefs/remotes/origin/HEAD\" >>expect.v2 &&\n \toid=$(git rev-parse HEAD) &&\n-\techo \"$oid\trefs/remotes/origin/HEAD\" >>expect &&\n+\techo \"$oid\trefs/remotes/origin/HEAD\" >>expect.v2 &&\n \tgenerate_references \\\n \t\trefs/remotes/origin/main \\\n \t\trefs/tags/mark \\\n \t\trefs/tags/mark1.1 \\\n \t\trefs/tags/mark1.10 \\\n-\t\trefs/tags/mark1.2 >>expect &&\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref >actual &&\n-\ttest_cmp expect actual\n+\t\trefs/tags/mark1.2 >>expect.v2 &&\n+\t# v0 does not show non-HEAD symrefs\n+\tgrep -v \"ref: refs/remotes\" expect.v0 &&\n+\tgit -c protocol.version=0 ls-remote --symref >actual.v0 &&\n+\ttest_cmp expect.v0 actual.v0 &&\n+\tgit -c protocol.version=2 ls-remote --symref >actual.v2 &&\n+\ttest_cmp expect.v2 actual.v2\n '\n \n test_expect_success 'ls-remote with filtered symref (refname)' '\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475422","messageId":"20230414212518.GF639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 6/7] t5512: test \"ls-remote --heads --symref\" filtering with v0 and v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:25:18Z","receivedAt":"2023-04-14T21:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two overlapping tests for checking the behavior of \"ls-remote\n--symref\" when filtering output. The first test checks that using\n\"--heads\" will omit the symref for HEAD (since we don't print anything\nabout HEAD at all), but still prints other symrefs.\n\nThis has been marked as expecting failure since it was added in\n99c08d4eb2 (ls-remote: add support for showing symrefs, 2016-01-19).\nThat's because back then, we only had the v0 protocol, and it only\nreported on the HEAD symref, not others. But these days we have v2,\nwhich does exactly what the test wants. It would even have started\nunexpectedly passing when we switched to v2 by default, except that\nb2f73b70b2 (t5512: compensate for v0 only sending HEAD symrefs,\n2019-02-25) over-zealously marked it to run only in v0 mode.\n\nSo let's run it with both protocol versions, and adjust the expected\noutput for each. It passes in v2 without modification. In v0 mode, we'll\ndrop the extra symref, but this is still testing something useful: it\nensures that we do omit HEAD.\n\nThe test after this checks \"--heads\" again, this time using the expected\nv0 output. That's now redundant. It also checks that limiting with a\npattern like \"refs/heads/*\" works similarly, but that's redundant with a\ntest earlier in the script which limits by HEAD (again, back then the\n\"HEAD\" test was less interesting because there were no other symrefs to\nomit, but in a modern v2 world, there are). So we can just delete that\nsecond test entirely.\n\nSigned-off-by: Jeff King \n---\n t/t5512-ls-remote.sh | 26 +++++++-------------------\n 1 file changed, 7 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh\nindex fbf23d12a6..151c76eb09 100755\n--- a/t/t5512-ls-remote.sh\n+++ b/t/t5512-ls-remote.sh\n@@ -275,30 +275,18 @@ test_expect_success 'ls-remote with filtered symref (refname)' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'ls-remote with filtered symref (--heads)' '\n+test_expect_success 'ls-remote with filtered symref (--heads)' '\n \tgit symbolic-ref refs/heads/foo refs/tags/mark &&\n-\tcat >expect <<-EOF &&\n+\tcat >expect.v2 <<-EOF &&\n \tref: refs/tags/mark\trefs/heads/foo\n \t$rev\trefs/heads/foo\n \t$rev\trefs/heads/main\n \tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'ls-remote --symref omits filtered-out matches' '\n-\tcat >expect <<-EOF &&\n-\t$rev\trefs/heads/foo\n-\t$rev\trefs/heads/main\n-\tEOF\n-\t# Protocol v2 supports sending symrefs for refs other than HEAD, so use\n-\t# protocol v0 here.\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref --heads . >actual &&\n-\ttest_cmp expect actual &&\n-\tGIT_TEST_PROTOCOL_VERSION=0 git ls-remote --symref . \"refs/heads/*\" >actual &&\n-\ttest_cmp expect actual\n+\tgrep -v \"^ref: refs/tags/\" expect.v0 &&\n+\tgit -c protocol.version=0 ls-remote --symref --heads . >actual.v0 &&\n+\ttest_cmp expect.v0 actual.v0 &&\n+\tgit -c protocol.version=2 ls-remote --symref --heads . >actual.v2 &&\n+\ttest_cmp expect.v2 actual.v2\n '\n \n test_expect_success 'indicate no refs in v0 standards-compliant empty remote' '\n-- \n2.40.0.515.gdfb9e78b42\n\n"},{"id":"475423","messageId":"20230414212520.GG639756@coredump.intra.peff.net","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"[PATCH v3 7/7] v0 protocol: use size_t for capability length/offset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-14T21:25:20Z","receivedAt":"2023-04-14T21:26:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing server capabilities, we use \"int\" to store lengths and\noffsets. At first glance this seems like a spot where our parser may be\nconfused by integer overflow if somebody sent us a malicious response.\n\nIn practice these strings are all bounded by the 64k limit of a\npkt-line, so using \"int\" is OK. However, it makes the code simpler to\naudit if they just use size_t everywhere. Note that because we take\nthese parameters as pointers, this also forces many callers to update\ntheir declared types.\n\nSigned-off-by: Jeff King \n---\n builtin/receive-pack.c | 2 +-\n connect.c | 22 +++++++++++-----------\n connect.h | 4 ++--\n fetch-pack.c | 4 ++--\n send-pack.c | 2 +-\n transport.c | 2 +-\n upload-pack.c | 2 +-\n 7 files changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9109552533..3b495ecc84 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -2093,7 +2093,7 @@ static struct command *read_head_info(struct packet_reader *reader,\n \t\t\tconst char *feature_list = reader->line + linelen + 1;\n \t\t\tconst char *hash = NULL;\n \t\t\tconst char *client_sid;\n-\t\t\tint len = 0;\n+\t\t\tsize_t len = 0;\n \t\t\tif (parse_feature_request(feature_list, \"report-status\"))\n \t\t\t\treport_status = 1;\n \t\t\tif (parse_feature_request(feature_list, \"report-status-v2\"))\ndiff --git a/connect.c b/connect.c\nindex b7ba5f5727..c54adc652f 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -22,7 +22,7 @@\n \n static char *server_capabilities_v1;\n static struct strvec server_capabilities_v2 = STRVEC_INIT;\n-static const char *next_server_feature_value(const char *feature, int *len, int *offset);\n+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset);\n \n static int check_ref(const char *name, unsigned int flags)\n {\n@@ -205,10 +205,10 @@ static void parse_one_symref_info(struct string_list *symref, const char *val, i\n static void annotate_refs_with_symref_info(struct ref *ref)\n {\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n-\tint offset = 0;\n+\tsize_t offset = 0;\n \n \twhile (1) {\n-\t\tint len;\n+\t\tsize_t len;\n \t\tconst char *val;\n \n \t\tval = next_server_feature_value(\"symref\", &len, &offset);\n@@ -231,7 +231,7 @@ static void annotate_refs_with_symref_info(struct ref *ref)\n static void process_capabilities(struct packet_reader *reader, int *linelen)\n {\n \tconst char *feat_val;\n-\tint feat_len;\n+\tsize_t feat_len;\n \tconst char *line = reader->line;\n \tint nul_location = strlen(line);\n \tif (nul_location == *linelen)\n@@ -596,10 +596,10 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \treturn list;\n }\n \n-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset)\n+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset)\n {\n \tconst char *orig_start = feature_list;\n-\tint len;\n+\tsize_t len;\n \n \tif (!feature_list)\n \t\treturn NULL;\n@@ -623,7 +623,7 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \t\t\t}\n \t\t\t/* feature with a value (e.g., \"agent=git/1.2.3\") */\n \t\t\telse if (*value == '=') {\n-\t\t\t\tint end;\n+\t\t\t\tsize_t end;\n \n \t\t\t\tvalue++;\n \t\t\t\tend = strcspn(value, \" \\t\\n\");\n@@ -645,8 +645,8 @@ const char *parse_feature_value(const char *feature_list, const char *feature, i\n \n int server_supports_hash(const char *desired, int *feature_supported)\n {\n-\tint offset = 0;\n-\tint len;\n+\tsize_t offset = 0;\n+\tsize_t len;\n \tconst char *hash;\n \n \thash = next_server_feature_value(\"object-format\", &len, &offset);\n@@ -670,12 +670,12 @@ int parse_feature_request(const char *feature_list, const char *feature)\n \treturn !!parse_feature_value(feature_list, feature, NULL, NULL);\n }\n \n-static const char *next_server_feature_value(const char *feature, int *len, int *offset)\n+static const char *next_server_feature_value(const char *feature, size_t *len, size_t *offset)\n {\n \treturn parse_feature_value(server_capabilities_v1, feature, len, offset);\n }\n \n-const char *server_feature_value(const char *feature, int *len)\n+const char *server_feature_value(const char *feature, size_t *len)\n {\n \treturn parse_feature_value(server_capabilities_v1, feature, len, NULL);\n }\ndiff --git a/connect.h b/connect.h\nindex f41a0b4c1f..1645126c17 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -12,14 +12,14 @@ int finish_connect(struct child_process *conn);\n int git_connection_is_socket(struct child_process *conn);\n int server_supports(const char *feature);\n int parse_feature_request(const char *features, const char *feature);\n-const char *server_feature_value(const char *feature, int *len_ret);\n+const char *server_feature_value(const char *feature, size_t *len_ret);\n int url_is_local_not_ssh(const char *url);\n \n struct packet_reader;\n enum protocol_version discover_version(struct packet_reader *reader);\n \n int server_supports_hash(const char *desired, int *feature_supported);\n-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset);\n+const char *parse_feature_value(const char *feature_list, const char *feature, size_t *lenp, size_t *offset);\n int server_supports_v2(const char *c);\n void ensure_server_supports_v2(const char *c);\n int server_feature_v2(const char *c, const char **v);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 368f2ed25a..97a44ed582 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1099,7 +1099,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tstruct object_id oid;\n \tconst char *agent_feature;\n-\tint agent_len;\n+\tsize_t agent_len;\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n@@ -1117,7 +1117,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tagent_supported = 1;\n \t\tif (agent_len)\n \t\t\tprint_verbose(args, _(\"Server version is %.*s\"),\n-\t\t\t\t agent_len, agent_feature);\n+\t\t\t\t (int)agent_len, agent_feature);\n \t}\n \n \tif (!server_supports(\"session-id\"))\ndiff --git a/send-pack.c b/send-pack.c\nindex f81bbb28d7..97344b629e 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -538,7 +538,7 @@ int send_pack(struct send_pack_args *args,\n \t\tdie(_(\"the receiving end does not support this repository's hash algorithm\"));\n \n \tif (args->push_cert != SEND_PACK_PUSH_CERT_NEVER) {\n-\t\tint len;\n+\t\tsize_t len;\n \t\tpush_cert_nonce = server_feature_value(\"push-cert\", &len);\n \t\tif (push_cert_nonce) {\n \t\t\treject_invalid_nonce(push_cert_nonce, len);\ndiff --git a/transport.c b/transport.c\nindex 89a220425e..6223dc3de2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -317,7 +317,7 @@ static struct ref *handshake(struct transport *transport, int for_push,\n \tstruct git_transport_data *data = transport->data;\n \tstruct ref *refs = NULL;\n \tstruct packet_reader reader;\n-\tint sid_len;\n+\tsize_t sid_len;\n \tconst char *server_sid;\n \n \tconnect_setup(transport, for_push);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex e23f16dfdd..565e46058f 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1067,7 +1067,7 @@ static void receive_needs(struct upload_pack_data *data,\n \t\tconst char *features;\n \t\tstruct object_id oid_buf;\n \t\tconst char *arg;\n-\t\tint feature_len;\n+\t\tsize_t feature_len;\n \n \t\treset_timeout(data->timeout);\n \t\tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n-- \n2.40.0.515.gdfb9e78b42\n"},{"id":"475525","messageId":"xmqqttxeo5lx.fsf@gitster.g","threadId":"59579","inReplyTo":"20230414212404.GA639653@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/7] v0 multiple-symref infinite loop fix and test cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-17T16:06:50Z","receivedAt":"2023-04-17T16:06:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 12, 2023 at 05:04:23AM -0400, Jeff King wrote:\n>\n>> I'll squash that in and update the commit message before I do the next\n>> re-roll (but will still hold off a bit to get further comments).\n>\n> Nobody said anything, so I assume the rest of the series is perfect. ;)\n>\n> Junio, I see that you picked up this fix as a \"squash\", along with my\n> other \"v2\" update. Here's a v3 that does the actual squash along with a\n> commit message update. That ties up all loose ends from my perspective,\n> but of course if anybody has review comments, please send them.\n\nThese look good. Queued.\n\nThanks.\n"}]}