{"thread":{"id":"49814","subject":"[PATCH] remote-curl: die on server-side errors","startedAt":"2018-11-12T22:45:01Z","lastAt":"2019-02-06T22:19:56Z","messageCount":41,"participants":["steadmon@google.com","Stefan Beller","Junio C Hamano","Jeff King","Josh Steadmon","Masaya Suzuki"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"363092","messageId":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","threadId":"49814","inReplyTo":null,"subject":"[PATCH] remote-curl: die on server-side errors","fromName":"","fromEmail":"steadmon@google.com","sentAt":"2018-11-12T22:44:56Z","receivedAt":"2018-11-12T22:45:01Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"When a smart HTTP server sends an error message via pkt-line,\nremote-curl will fail to detect the error (which usually results in\nincorrectly falling back to dumb-HTTP mode).\n\nThis patch adds a check in discover_refs() for server-side error\nmessages, as well as a test case for this issue.\n\nSigned-off-by: Josh Steadmon \n---\n remote-curl.c | 4 +++-\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-smart-http.sh | 3 +++\n t/t5551-http-fetch-smart.sh | 5 +++++\n 5 files changed, 16 insertions(+), 1 deletion(-)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..bb3a86505e 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t} else if (maybe_smart &&\n \t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n \t\tlast->proto_git = 1;\n-\t}\n+\t} else if (maybe_smart && last->len > 5 &&\n+\t\t starts_with(last->buf + 4, \"ERR \"))\n+\t\tdie(_(\"remote error: %s\"), last->buf + 8);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a8729f8232..4e946623bb 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -131,6 +131,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-sed.sh\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..6de2bc775c 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n Options ExecCGI\n \ndiff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh\nnew file mode 100644\nindex 0000000000..0a1af318f7\n--- /dev/null\n+++ b/t/lib-httpd/error-smart-http.sh\n@@ -0,0 +1,3 @@\n+printf \"Content-Type: text/%s\\n\" \"html\"\n+echo\n+printf \"%s\" \"0019ERR server-side error\"\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..ba83e567e5 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n \t! grep \"=> Send data\" err\n '\n \n+test_expect_success 'server-side error detected' '\n+\ttest_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&\n+\tgrep \"server-side error\" actual\n+'\n+\n stop_httpd\n test_done\n-- \n2.19.1.930.g4563a0d9d0-goog\n\n"},{"id":"363096","messageId":"CAGZ79kbsELU=+91WzeUu6w8OUpQiP4FQO4LBvzCXJvBEe_qCdw@mail.gmail.com","threadId":"49814","inReplyTo":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-12T22:55:53Z","receivedAt":"2018-11-12T22:56:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 12, 2018 at 2:45 PM wrote:\n>\n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n>\n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n>\n> Signed-off-by: Josh Steadmon \n\nReviewed-by: Stefan Beller \n"},{"id":"363125","messageId":"xmqqbm6ty9ib.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T02:52:12Z","receivedAt":"2018-11-13T02:52:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"steadmon@google.com writes:\n\n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n>\n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> remote-curl.c | 4 +++-\n> t/lib-httpd.sh | 1 +\n> t/lib-httpd/apache.conf | 4 ++++\n> t/lib-httpd/error-smart-http.sh | 3 +++\n> t/t5551-http-fetch-smart.sh | 5 +++++\n> 5 files changed, 16 insertions(+), 1 deletion(-)\n> create mode 100644 t/lib-httpd/error-smart-http.sh\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..bb3a86505e 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t} else if (maybe_smart &&\n> \t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> \t\tlast->proto_git = 1;\n> -\t}\n> +\t} else if (maybe_smart && last->len > 5 &&\n> +\t\t starts_with(last->buf + 4, \"ERR \"))\n> +\t\tdie(_(\"remote error: %s\"), last->buf + 8);\n\nTwo observations and a half.\n\n - All of these if/else if/ blocks (currently 2, now you added the\n third one) are to special case the \"maybe_smart\" case. Perhaps\n the whole thing should be restrucutred to\n\n\tif (maybe_smart) {\n\t\tif ...\n\t\telse if ...\n\t\telse if ...\n\t}\n\n - All conditions skip the first four bytes in last->buf and does\n starts_with (the first branch is \"starts_with(\"#\") in disguise).\n Can we do something to the hardcoded magic number 4, which looks\n somewhat irritating?\n\n - This is less of the problem with the suggested change in the\n first bullet item above, but the current code structure makes\n readers wonder if maybe_start that starts as 1 is turned off\n somewhere in the if/else if/ cascade when we find out that we are\n not dealing with smart HTTP after all. That however is not what\n is happening. The \"maybe\" variable is primarily there to avoid\n repeating the logic that determined its initial value in the\n early part of the function. The last->proto_git field is used to\n record if we are using smart HTTP or not.\n\nOtherwise the change looks sensible.\n\n\n> diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\n> index a8729f8232..4e946623bb 100644\n> --- a/t/lib-httpd.sh\n> +++ b/t/lib-httpd.sh\n> @@ -131,6 +131,7 @@ prepare_httpd() {\n> \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n> \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n> \tinstall_script broken-smart-http.sh\n> +\tinstall_script error-smart-http.sh\n> \tinstall_script error.sh\n> \tinstall_script apply-one-time-sed.sh\n> \n> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\n> index 581c010d8f..6de2bc775c 100644\n> --- a/t/lib-httpd/apache.conf\n> +++ b/t/lib-httpd/apache.conf\n> @@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/\n> \n> ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n> ScriptAlias /broken_smart/ broken-smart-http.sh/\n> +ScriptAlias /error_smart/ error-smart-http.sh/\n> ScriptAlias /error/ error.sh/\n> ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n> \n> @@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n> \n> \tOptions ExecCGI\n> \n> +\n> +\tOptions ExecCGI\n> +\n> \n> Options ExecCGI\n> \n> diff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh\n> new file mode 100644\n> index 0000000000..0a1af318f7\n> --- /dev/null\n> +++ b/t/lib-httpd/error-smart-http.sh\n> @@ -0,0 +1,3 @@\n> +printf \"Content-Type: text/%s\\n\" \"html\"\n> +echo\n> +printf \"%s\" \"0019ERR server-side error\"\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index 8630b0cc39..ba83e567e5 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n> \t! grep \"=> Send data\" err\n> '\n> \n> +test_expect_success 'server-side error detected' '\n> +\ttest_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&\n> +\tgrep \"server-side error\" actual\n> +'\n> +\n> stop_httpd\n> test_done\n"},{"id":"363126","messageId":"xmqq7ehhy91z.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T03:02:00Z","receivedAt":"2018-11-13T03:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"steadmon@google.com writes:\n\n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n>\n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n\nForgot to mention one procedural comment.\n\nAs you can see in the To: line of this reply, your MUA is placing\nonly the e-mail address without name on your From: line.\n\nPreferrably I'd like to see the same string as your sign-off on the\n\"From:\" line in your messages for a bit of human touch ;-) Can you\ntweak your MUA to make that happen?\n\nThe second preference is to add an in-body header (i.e. as the first\nline of the body of the message) so that the body of the message\nstarts like this:\n\n From: Josh Steadmon \n\n When a smart HTTP server sends an error message via pkt-line,\n remote-curl will fail to detect the error (which usually results in\n incorrectly falling back to dumb-HTTP mode).\n\n This patch adds a check in discover_refs() for server-side error\n messages, as well as a test case for this issue.\n\n Signed-off-by: Josh Steadmon \n ---\n remote-curl.c | 4 +++-\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n\nEither way would make sure that the resulting patch's author line\nwill be attributed correctly to the same identity as who is signing\nit off first as the author.\n\nThanks.\n"},{"id":"363153","messageId":"20181113142624.GA17128@sigill.intra.peff.net","threadId":"49814","inReplyTo":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-13T14:26:24Z","receivedAt":"2018-11-13T14:26:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2018 at 02:44:56PM -0800, steadmon@google.com wrote:\n\n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n> \n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n\nAside from the reformatting of the conditional that Junio mentioned,\nthis seems pretty good to me. But while looking at that, I found a few\nthings, some old and some new. :)\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..bb3a86505e 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t} else if (maybe_smart &&\n> \t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> \t\tlast->proto_git = 1;\n> -\t}\n> +\t} else if (maybe_smart && last->len > 5 &&\n> +\t\t starts_with(last->buf + 4, \"ERR \"))\n> +\t\tdie(_(\"remote error: %s\"), last->buf + 8);\n\nThe magic \"4\" here and in the existing \"version 2\" check is because we\nare expecting pkt-lines. The original conditional always calls\npacked_read_line_buf() and will complain if we didn't actually get a\npkt-line.\n\nShould we confirm that we got a real packet-line? Or at least that those\nfirst four are even plausible hex chars?\n\nI admit that it's pretty unlikely that the server is going to fool us\nhere. It would need something like \"foo ERRORS ARE FUN!\". And even then\nwe'd report an error (whereas the correct behavior is falling back to\ndumb http, but we know that won't work anyway because that's not a valid\nref advertisement). So I doubt this is really a bug per se, but it might\nmake it easier to understand what's going on if we actually parsed the\npacket.\n\nSimilarly, we seem eager to accept \"version 2\" even if we are only\nexpecting v0. I know you have another series working in that direction,\nbut I don't think it touches this \"proto_git\". I guess accepting\n\"version 2\" as \"we're speaking git protocol\" and then barfing later with\n\"wait, I didn't mean to speak v2\" is probably OK.\n\n-Peff\n"},{"id":"363154","messageId":"xmqq4lcluk1u.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"9a89e54e79593f6455b52e01d802695362f4ec21.1542062657.git.steadmon@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-13T14:30:21Z","receivedAt":"2018-11-13T14:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"steadmon@google.com writes:\n\n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n\nOK, that is a valid thing to worry about.\n\n>\n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n\nThis makes me wonder if discoer_refs() is the only place where we\nought to be checking for ERR packets but we are not. Are there\nother places that we also need a similar fix?\n"},{"id":"363224","messageId":"20181113221522.GD126896@google.com","threadId":"49814","inReplyTo":"xmqq7ehhy91z.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-13T22:15:22Z","receivedAt":"2018-11-13T22:15:33Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.13 12:02, Junio C Hamano wrote:\n> steadmon@google.com writes:\n> \n> > When a smart HTTP server sends an error message via pkt-line,\n> > remote-curl will fail to detect the error (which usually results in\n> > incorrectly falling back to dumb-HTTP mode).\n> >\n> > This patch adds a check in discover_refs() for server-side error\n> > messages, as well as a test case for this issue.\n> >\n> > Signed-off-by: Josh Steadmon \n> > ---\n> \n> Forgot to mention one procedural comment.\n> \n> As you can see in the To: line of this reply, your MUA is placing\n> only the e-mail address without name on your From: line.\n> \n> Preferrably I'd like to see the same string as your sign-off on the\n> \"From:\" line in your messages for a bit of human touch ;-) Can you\n> tweak your MUA to make that happen?\n> \n> The second preference is to add an in-body header (i.e. as the first\n> line of the body of the message) so that the body of the message\n> starts like this:\n> \n> From: Josh Steadmon \n> \n> When a smart HTTP server sends an error message via pkt-line,\n> remote-curl will fail to detect the error (which usually results in\n> incorrectly falling back to dumb-HTTP mode).\n> \n> This patch adds a check in discover_refs() for server-side error\n> messages, as well as a test case for this issue.\n> \n> Signed-off-by: Josh Steadmon \n> ---\n> remote-curl.c | 4 +++-\n> t/lib-httpd.sh | 1 +\n> t/lib-httpd/apache.conf | 4 ++++\n> \n> Either way would make sure that the resulting patch's author line\n> will be attributed correctly to the same identity as who is signing\n> it off first as the author.\n> \n> Thanks.\n\nThis should be fixed for future patch submissions. Thanks for the\nheads-up.\n"},{"id":"363225","messageId":"20181113222540.GE126896@google.com","threadId":"49814","inReplyTo":"20181113142624.GA17128@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-13T22:25:40Z","receivedAt":"2018-11-13T22:25:50Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.13 09:26, Jeff King wrote:\n> On Mon, Nov 12, 2018 at 02:44:56PM -0800, steadmon@google.com wrote:\n> \n> > When a smart HTTP server sends an error message via pkt-line,\n> > remote-curl will fail to detect the error (which usually results in\n> > incorrectly falling back to dumb-HTTP mode).\n> > \n> > This patch adds a check in discover_refs() for server-side error\n> > messages, as well as a test case for this issue.\n> \n> Aside from the reformatting of the conditional that Junio mentioned,\n> this seems pretty good to me. But while looking at that, I found a few\n> things, some old and some new. :)\n> \n> > diff --git a/remote-curl.c b/remote-curl.c\n> > index 762a55a75f..bb3a86505e 100644\n> > --- a/remote-curl.c\n> > +++ b/remote-curl.c\n> > @@ -436,7 +436,9 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> > \t} else if (maybe_smart &&\n> > \t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> > \t\tlast->proto_git = 1;\n> > -\t}\n> > +\t} else if (maybe_smart && last->len > 5 &&\n> > +\t\t starts_with(last->buf + 4, \"ERR \"))\n> > +\t\tdie(_(\"remote error: %s\"), last->buf + 8);\n> \n> The magic \"4\" here and in the existing \"version 2\" check is because we\n> are expecting pkt-lines. The original conditional always calls\n> packed_read_line_buf() and will complain if we didn't actually get a\n> pkt-line.\n> \n> Should we confirm that we got a real packet-line? Or at least that those\n> first four are even plausible hex chars?\n> \n> I admit that it's pretty unlikely that the server is going to fool us\n> here. It would need something like \"foo ERRORS ARE FUN!\". And even then\n> we'd report an error (whereas the correct behavior is falling back to\n> dumb http, but we know that won't work anyway because that's not a valid\n> ref advertisement). So I doubt this is really a bug per se, but it might\n> make it easier to understand what's going on if we actually parsed the\n> packet.\n\nUnfortunately we can't just directly parse the data in last->buf,\nbecause other parts of the code are expecting to see the raw pkt-line\ndata there. I tried adding a duplicate pointer and length variable for\nthis data and parsing that through packet_read_line_buf(), but even\nwithout using the results this apparently has side-effects that break\nall of t5550 (and probably other tests as well). It also fails if I\ncompletely duplicate last->buf into a new char* and call\npacket_readline_buf() on that, so there's clearly some interaction in\nthe pkt-line guts that I'm not properly accounting for.\n\n> Similarly, we seem eager to accept \"version 2\" even if we are only\n> expecting v0. I know you have another series working in that direction,\n> but I don't think it touches this \"proto_git\". I guess accepting\n> \"version 2\" as \"we're speaking git protocol\" and then barfing later with\n> \"wait, I didn't mean to speak v2\" is probably OK.\n> \n> -Peff\n"},{"id":"363227","messageId":"20181113222841.GF126896@google.com","threadId":"49814","inReplyTo":"xmqq4lcluk1u.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-13T22:28:41Z","receivedAt":"2018-11-13T22:28:51Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.13 23:30, Junio C Hamano wrote:\n> steadmon@google.com writes:\n> \n> > When a smart HTTP server sends an error message via pkt-line,\n> > remote-curl will fail to detect the error (which usually results in\n> > incorrectly falling back to dumb-HTTP mode).\n> \n> OK, that is a valid thing to worry about.\n> \n> >\n> > This patch adds a check in discover_refs() for server-side error\n> > messages, as well as a test case for this issue.\n> \n> This makes me wonder if discoer_refs() is the only place where we\n> ought to be checking for ERR packets but we are not. Are there\n> other places that we also need a similar fix?\n\nQuite possibly; I'll review the other client code to see if there are\nsimilar issues before sending v2.\n"},{"id":"363276","messageId":"20181114004915.GA5059@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181113222540.GE126896@google.com","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-14T00:49:15Z","receivedAt":"2018-11-14T00:49:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2018 at 02:25:40PM -0800, Josh Steadmon wrote:\n\n> > The magic \"4\" here and in the existing \"version 2\" check is because we\n> > are expecting pkt-lines. The original conditional always calls\n> > packed_read_line_buf() and will complain if we didn't actually get a\n> > pkt-line.\n> > \n> > Should we confirm that we got a real packet-line? Or at least that those\n> > first four are even plausible hex chars?\n> > \n> > I admit that it's pretty unlikely that the server is going to fool us\n> > here. It would need something like \"foo ERRORS ARE FUN!\". And even then\n> > we'd report an error (whereas the correct behavior is falling back to\n> > dumb http, but we know that won't work anyway because that's not a valid\n> > ref advertisement). So I doubt this is really a bug per se, but it might\n> > make it easier to understand what's going on if we actually parsed the\n> > packet.\n> \n> Unfortunately we can't just directly parse the data in last->buf,\n> because other parts of the code are expecting to see the raw pkt-line\n> data there. I tried adding a duplicate pointer and length variable for\n> this data and parsing that through packet_read_line_buf(), but even\n> without using the results this apparently has side-effects that break\n> all of t5550 (and probably other tests as well). It also fails if I\n> completely duplicate last->buf into a new char* and call\n> packet_readline_buf() on that, so there's clearly some interaction in\n> the pkt-line guts that I'm not properly accounting for.\n\nYes, the packet_read_line_buf() interface will both advance the buf\npointer and decrement the length. So if we want to \"peek\", we have to\ndo so with a copy (there's a peek function if you use the packet_reader\ninterface, but that might be overkill here).\n\nYou can rewrite it like this, which is a pretty faithful conversion and\npasses the tests (but see below).\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..66c57c9d62 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -330,9 +330,78 @@ static int get_protocol_http_header(enum protocol_version version,\n \treturn 0;\n }\n \n+static void check_smart_http(struct discovery *d, const char *service,\n+\t\t\t struct strbuf *type)\n+{\n+\tchar *src_buf;\n+\tsize_t src_len;\n+\tchar pkt[LARGE_PACKET_MAX];\n+\tint pkt_len;\n+\tenum packet_read_status r;\n+\n+\t/*\n+\t * We speculatively try to read a packet, which means we must preserve\n+\t * the original buf/len pair in some cases.\n+\t */\n+\tsrc_buf = d->buf;\n+\tsrc_len = d->len;\n+\tr = packet_read_with_status(-1, &src_buf, &src_len,\n+\t\t\t\t pkt, sizeof(pkt), &pkt_len,\n+\t\t\t\t PACKET_READ_GENTLE |\n+\t\t\t\t PACKET_READ_CHOMP_NEWLINE);\n+\n+\t/*\n+\t * This could probably just be handled as \"not smart\" like all the\n+\t * other pkt-line error cases, but traditionally we've complained\n+\t * about it (technically we used to do so only when we got the right\n+\t * content-type, but it probably doesn't matter).\n+\t */\n+\tif (r == PACKET_READ_FLUSH)\n+\t\tdie(\"invalid server response; expected service, got flush packet\");\n+\tif (r != PACKET_READ_NORMAL)\n+\t\treturn; /* not smart */\n+\n+\tif (pkt[0] == '#') {\n+\t\t/* v0 smart http */\n+\t\tstruct strbuf exp = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n+\t\tif (strbuf_cmp(&exp, type)) {\n+\t\t\tstrbuf_release(&exp);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tstrbuf_reset(&exp);\n+\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n+\t\tif (strcmp(pkt, exp.buf))\n+\t\t\tdie(\"invalid server response; got '%s'\", pkt);\n+\n+\t\tstrbuf_release(&exp);\n+\n+\t\t/*\n+\t\t * The header can include additional metadata lines, up\n+\t\t * until a packet flush marker. Ignore these now, but\n+\t\t * in the future we might start to scan them.\n+\t\t */\n+\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n+\t\t\t;\n+\n+\t\td->buf = src_buf;\n+\t\td->len = src_len;\n+\t\td->proto_git = 1;\n+\n+\t} else if (starts_with(pkt, \"version 2\")) {\n+\t\t/*\n+\t\t * v2 smart http; do not consume version packet, which will\n+\t\t * be handled elsewhere. Should we be checking the content-type\n+\t\t * in this code-path, too?\n+\t\t */\n+\t\td->proto_git = 1;\n+\t}\n+}\n+\n static struct discovery *discover_refs(const char *service, int for_push)\n {\n-\tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n@@ -405,38 +474,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\tif (maybe_smart &&\n-\t (5 <= last->len && last->buf[4] == '#') &&\n-\t !strbuf_cmp(&exp, &type)) {\n-\t\tchar *line;\n-\n-\t\t/*\n-\t\t * smart HTTP response; validate that the service\n-\t\t * pkt-line matches our request.\n-\t\t */\n-\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n-\t\tif (!line)\n-\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n-\n-\t\tstrbuf_reset(&exp);\n-\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", line);\n-\t\tstrbuf_release(&exp);\n-\n-\t\t/* The header can include additional metadata lines, up\n-\t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n-\t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n-\t\t\t;\n-\n-\t\tlast->proto_git = 1;\n-\t} else if (maybe_smart &&\n-\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n-\t\tlast->proto_git = 1;\n-\t}\n+\tif (maybe_smart)\n+\t\tcheck_smart_http(last, service, &type);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\n@@ -444,7 +483,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tlast->refs = parse_info_refs(last);\n \n \tstrbuf_release(&refs_url);\n-\tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n\nSo the few tricky things are:\n\n - the ordering with respect to checking the packet and the\n content-type is a little different here. Should v2 protocol be\n expecting that content-type, too? If so, then I think it would make\n sense to check that first, before even considering if there's a\n packet to read (and the current code is overly-permissive in that\n case).\n\n - there is no way to speculatively read a packet and not die() if the\n buffer doesn't have pkt-lines in it. So we'd additionally need the\n patch below (which annoyingly has to touch a bunch of switch\n statements to keep the compiler happy). If we _can_ check the\n content-type first, then all of that could go away (i.e., once we\n see the right content-type, we're confident enough in expecting\n packets to die() in the existing packet code-paths).\n\ndiff --git a/connect.c b/connect.c\nindex 24281b6082..1caafc3ce7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -125,6 +125,8 @@ enum protocol_version discover_version(struct packet_reader *reader)\n \tswitch (packet_reader_peek(reader)) {\n \tcase PACKET_READ_EOF:\n \t\tdie_initial_contact(0);\n+\tcase PACKET_READ_FORMAT_ERROR:\n+\t\tBUG(\"format error from non-gentle packet read\");\n \tcase PACKET_READ_FLUSH:\n \tcase PACKET_READ_DELIM:\n \t\tversion = protocol_v0;\n@@ -304,6 +306,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\tswitch (packet_reader_read(reader)) {\n \t\tcase PACKET_READ_EOF:\n \t\t\tdie_initial_contact(1);\n+\t\tcase PACKET_READ_FORMAT_ERROR:\n+\t\t\tBUG(\"format error from non-gentle packet read\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tlen = reader->pktlen;\n \t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 04d10bbd03..2daacc6188 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -293,7 +293,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,\n \n \t/* And complain if we didn't get enough bytes to satisfy the read. */\n \tif (ret != size) {\n-\t\tif (options & PACKET_READ_GENTLE_ON_EOF)\n+\t\tif ((options & PACKET_READ_GENTLE_ON_EOF) ||\n+\t\t (options & PACKET_READ_GENTLE))\n \t\t\treturn -1;\n \n \t\tdie(_(\"the remote end hung up unexpectedly\"));\n@@ -324,6 +325,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \tlen = packet_length(linelen);\n \n \tif (len < 0) {\n+\t\tif (options & PACKET_READ_GENTLE) {\n+\t\t\t*pktlen = -1;\n+\t\t\treturn PACKET_READ_FORMAT_ERROR;\n+\t\t}\n \t\tdie(_(\"protocol error: bad line length character: %.4s\"), linelen);\n \t} else if (!len) {\n \t\tpacket_trace(\"0000\", 4, 0);\n@@ -334,12 +339,21 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t*pktlen = 0;\n \t\treturn PACKET_READ_DELIM;\n \t} else if (len < 4) {\n+\t\tif (options & PACKET_READ_GENTLE) {\n+\t\t\t*pktlen = -1;\n+\t\t\treturn PACKET_READ_FORMAT_ERROR;\n+\t\t}\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n \t}\n \n \tlen -= 4;\n-\tif ((unsigned)len >= size)\n+\tif ((unsigned)len >= size) {\n+\t\tif (options & PACKET_READ_GENTLE) {\n+\t\t\t*pktlen = -1;\n+\t\t\treturn PACKET_READ_FORMAT_ERROR;\n+\t\t}\n \t\tdie(_(\"protocol error: bad line length %d\"), len);\n+\t}\n \n \tif (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {\n \t\t*pktlen = -1;\ndiff --git a/pkt-line.h b/pkt-line.h\nindex 5b28d43472..7580f83cdc 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -44,7 +44,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n * If src_buffer (or *src_buffer) is NULL, then data is read from the\n * descriptor \"fd\".\n *\n- * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any\n+ * If options does not contain PACKET_READ_GENTLE, we will die under any\n * of the following conditions:\n *\n * 1. Read error from descriptor.\n@@ -56,6 +56,8 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n * 4. Truncated output from the remote (e.g., we expected a packet but got\n * EOF, or we got a partial packet followed by EOF).\n *\n+ * If options does contain PACKET_READ_GENTLE, we'll instead return -1.\n+ *\n * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on\n * condition 4 (truncated input), but instead return -1. However, we will still\n * die for the other 3 conditions.\n@@ -65,6 +67,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);\n */\n #define PACKET_READ_GENTLE_ON_EOF (1u<<0)\n #define PACKET_READ_CHOMP_NEWLINE (1u<<1)\n+#define PACKET_READ_GENTLE (1u<<2)\n int packet_read(int fd, char **src_buffer, size_t *src_len, char\n \t\t*buffer, unsigned size, int options);\n \n@@ -79,6 +82,7 @@ enum packet_read_status {\n \tPACKET_READ_NORMAL,\n \tPACKET_READ_FLUSH,\n \tPACKET_READ_DELIM,\n+\tPACKET_READ_FORMAT_ERROR,\n };\n enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\t\t\t\t\tsize_t *src_len, char *buffer,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..66c57c9d62 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1214,6 +1252,8 @@ static size_t proxy_in(char *buffer, size_t eltsize,\n \t\tswitch (packet_reader_read(&p->reader)) {\n \t\tcase PACKET_READ_EOF:\n \t\t\tdie(\"unexpected EOF when reading from parent process\");\n+\t\tcase PACKET_READ_FORMAT_ERROR:\n+\t\t\tBUG(\"format error from non-gentle packet read\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tpacket_buf_write_len(&p->request_buffer, p->reader.line,\n \t\t\t\t\t p->reader.pktlen);\ndiff --git a/serve.c b/serve.c\nindex bda085f09c..e88936262b 100644\n--- a/serve.c\n+++ b/serve.c\n@@ -181,6 +181,8 @@ static int process_request(void)\n \t\tswitch (packet_reader_peek(&reader)) {\n \t\tcase PACKET_READ_EOF:\n \t\t\tBUG(\"Should have already died when seeing EOF\");\n+\t\tcase PACKET_READ_FORMAT_ERROR:\n+\t\t\tBUG(\"format error from non-gentle packet read\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\t/* collect request; a sequence of keys and values */\n \t\t\tif (is_command(reader.line, &command) ||\ndiff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c\nindex 282d536384..1522176a2f 100644\n--- a/t/helper/test-pkt-line.c\n+++ b/t/helper/test-pkt-line.c\n@@ -37,6 +37,8 @@ static void unpack(void)\n \t\tswitch (reader.status) {\n \t\tcase PACKET_READ_EOF:\n \t\t\tbreak;\n+\t\tcase PACKET_READ_FORMAT_ERROR:\n+\t\t\tBUG(\"format error from non-gentle packet read\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tprintf(\"%s\\n\", reader.line);\n \t\t\tbreak;\n@@ -64,6 +66,8 @@ static void unpack_sideband(void)\n \t\tswitch (reader.status) {\n \t\tcase PACKET_READ_EOF:\n \t\t\tbreak;\n+\t\tcase PACKET_READ_FORMAT_ERROR:\n+\t\t\tBUG(\"format error from non-gentle packet read\");\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tband = reader.line[0] & 0xff;\n \t\t\tif (band < 1 || band > 2)\n\n"},{"id":"363323","messageId":"20181114070052.GA19904@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181114004915.GA5059@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-14T07:00:52Z","receivedAt":"2018-11-14T07:00:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote:\n\n> Yes, the packet_read_line_buf() interface will both advance the buf\n> pointer and decrement the length. So if we want to \"peek\", we have to\n> do so with a copy (there's a peek function if you use the packet_reader\n> interface, but that might be overkill here).\n> \n> You can rewrite it like this, which is a pretty faithful conversion and\n> passes the tests (but see below).\n> [...]\n\nHere's a version which is less faithful, but I think does sensible\nthings in all cases, and is much easier to follow. I get a little\nnervous just because it tightens some cases, and one never knows if\nother implementations might be relying on the looseness. E.g.:\n\n - in the current code we'd still drop back to dumb http if the server\n tells us \"application/x-git-upload-pack\" but the initial pktline\n doesn't start with \"#\" (even though if it _does_ have \"#\" at\n position 5 but isn't a valid pktline, we'd complain!)\n\n - right now the v2 protocol does not require the server to say\n \"application/x-git-upload-pack\" for the content-type\n\nThis patch tightens both of those (I also made a few stylistic tweaks,\nand added the ERR condition to show where it would go). I dunno. Part of\nme sees this as a nice cleanup, but maybe it is better to just leave it\nalone. A lot of these behaviors are just how it happens to work now, and\nnot part of the spec, but we don't know what might be relying on them.\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..1adb96311b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -330,9 +330,61 @@ static int get_protocol_http_header(enum protocol_version version,\n \treturn 0;\n }\n \n+static void check_smart_http(struct discovery *d, const char *service,\n+\t\t\t struct strbuf *type)\n+{\n+\tchar *src_buf;\n+\tsize_t src_len;\n+\tchar *line;\n+\tconst char *p;\n+\n+\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n+\t !skip_prefix(p, service, &p) ||\n+\t strcmp(p, \"-advertisement\"))\n+\t\treturn;\n+\n+\t/*\n+\t * We speculatively try to read a packet, which means we must preserve\n+\t * the original buf/len pair in some cases.\n+\t */\n+\tsrc_buf = d->buf;\n+\tsrc_len = d->len;\n+\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n+\tif (!line)\n+\t\tdie(\"invalid server response; expected service, got flush packet\");\n+\n+\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n+\t\t/*\n+\t\t * The header can include additional metadata lines, up\n+\t\t * until a packet flush marker. Ignore these now, but\n+\t\t * in the future we might start to scan them.\n+\t\t */\n+\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n+\t\t\t;\n+\n+\t\t/*\n+\t\t * v0 smart http; callers expect us to soak up the\n+\t\t * service and header packets\n+\t\t */\n+\t\td->buf = src_buf;\n+\t\td->len = src_len;\n+\t\td->proto_git = 1;\n+\n+\t} else if (starts_with(line, \"version 2\")) { /* should be strcmp()? */\n+\t\t/*\n+\t\t * v2 smart http; do not consume version packet, which will\n+\t\t * be handled elsewhere.\n+\t\t */\n+\t\td->proto_git = 1;\n+\t} else if (skip_prefix(line, \"ERR \", &p)) {\n+\t\tdie(_(\"remote error: %s\"), p);\n+\t} else {\n+\t\tdie(\"invalid server response; got '%s'\", line);\n+\t}\n+}\n+\n static struct discovery *discover_refs(const char *service, int for_push)\n {\n-\tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n@@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\tif (maybe_smart &&\n-\t (5 <= last->len && last->buf[4] == '#') &&\n-\t !strbuf_cmp(&exp, &type)) {\n-\t\tchar *line;\n-\n-\t\t/*\n-\t\t * smart HTTP response; validate that the service\n-\t\t * pkt-line matches our request.\n-\t\t */\n-\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n-\t\tif (!line)\n-\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n-\n-\t\tstrbuf_reset(&exp);\n-\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", line);\n-\t\tstrbuf_release(&exp);\n-\n-\t\t/* The header can include additional metadata lines, up\n-\t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n-\t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n-\t\t\t;\n-\n-\t\tlast->proto_git = 1;\n-\t} else if (maybe_smart &&\n-\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n-\t\tlast->proto_git = 1;\n-\t}\n+\tif (maybe_smart)\n+\t\tcheck_smart_http(last, service, &type);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\n@@ -444,7 +466,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tlast->refs = parse_info_refs(last);\n \n \tstrbuf_release(&refs_url);\n-\tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n"},{"id":"363453","messageId":"20181115215152.GK126896@google.com","threadId":"49814","inReplyTo":"20181114070052.GA19904@sigill.intra.peff.net","subject":"Re: [PATCH] remote-curl: die on server-side errors","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-15T21:51:52Z","receivedAt":"2018-11-15T21:52:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.14 02:00, Jeff King wrote:\n> On Tue, Nov 13, 2018 at 07:49:15PM -0500, Jeff King wrote:\n> \n> > Yes, the packet_read_line_buf() interface will both advance the buf\n> > pointer and decrement the length. So if we want to \"peek\", we have to\n> > do so with a copy (there's a peek function if you use the packet_reader\n> > interface, but that might be overkill here).\n> > \n> > You can rewrite it like this, which is a pretty faithful conversion and\n> > passes the tests (but see below).\n> > [...]\n> \n> Here's a version which is less faithful, but I think does sensible\n> things in all cases, and is much easier to follow. I get a little\n> nervous just because it tightens some cases, and one never knows if\n> other implementations might be relying on the looseness. E.g.:\n> \n> - in the current code we'd still drop back to dumb http if the server\n> tells us \"application/x-git-upload-pack\" but the initial pktline\n> doesn't start with \"#\" (even though if it _does_ have \"#\" at\n> position 5 but isn't a valid pktline, we'd complain!)\n> \n> - right now the v2 protocol does not require the server to say\n> \"application/x-git-upload-pack\" for the content-type\n> \n> This patch tightens both of those (I also made a few stylistic tweaks,\n> and added the ERR condition to show where it would go). I dunno. Part of\n> me sees this as a nice cleanup, but maybe it is better to just leave it\n> alone. A lot of these behaviors are just how it happens to work now, and\n> not part of the spec, but we don't know what might be relying on them.\n\nAt least according to the protocol-v2 and http-protocol docs, the\nstricter behavior seems correct:\n\nFor the first point above, dumb servers should never use an\n\"application/x-git-*\" content type (http-protocol.txt line 163-167).\n\nFor the second point, the docs require v2 servers to use\n\"application/x-git-*\" content types. protocol-v2.txt lines 63-65 state\nthat v2 clients should make a smart http request, while\nhttp-protocol.txt lines 247-252 state that a smart server's response\ntype must be \"application/x-git-*\".\n\nOf course we don't know if other implementations follow the spec, but\nISTM that this patch at least doesn't contradict how we've promised the\nprotocols should work.\n\nIf no one has any objections, I'll include the diff below in v2. Thanks\nfor the help Jeff!\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..1adb96311b 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -330,9 +330,61 @@ static int get_protocol_http_header(enum protocol_version version,\n> \treturn 0;\n> }\n> \n> +static void check_smart_http(struct discovery *d, const char *service,\n> +\t\t\t struct strbuf *type)\n> +{\n> +\tchar *src_buf;\n> +\tsize_t src_len;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n> +\t !skip_prefix(p, service, &p) ||\n> +\t strcmp(p, \"-advertisement\"))\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * We speculatively try to read a packet, which means we must preserve\n> +\t * the original buf/len pair in some cases.\n> +\t */\n> +\tsrc_buf = d->buf;\n> +\tsrc_len = d->len;\n> +\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n> +\tif (!line)\n> +\t\tdie(\"invalid server response; expected service, got flush packet\");\n> +\n> +\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n> +\t\t/*\n> +\t\t * The header can include additional metadata lines, up\n> +\t\t * until a packet flush marker. Ignore these now, but\n> +\t\t * in the future we might start to scan them.\n> +\t\t */\n> +\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n> +\t\t\t;\n> +\n> +\t\t/*\n> +\t\t * v0 smart http; callers expect us to soak up the\n> +\t\t * service and header packets\n> +\t\t */\n> +\t\td->buf = src_buf;\n> +\t\td->len = src_len;\n> +\t\td->proto_git = 1;\n> +\n> +\t} else if (starts_with(line, \"version 2\")) { /* should be strcmp()? */\n> +\t\t/*\n> +\t\t * v2 smart http; do not consume version packet, which will\n> +\t\t * be handled elsewhere.\n> +\t\t */\n> +\t\td->proto_git = 1;\n> +\t} else if (skip_prefix(line, \"ERR \", &p)) {\n> +\t\tdie(_(\"remote error: %s\"), p);\n> +\t} else {\n> +\t\tdie(\"invalid server response; got '%s'\", line);\n> +\t}\n> +}\n> +\n> static struct discovery *discover_refs(const char *service, int for_push)\n> {\n> -\tstruct strbuf exp = STRBUF_INIT;\n> \tstruct strbuf type = STRBUF_INIT;\n> \tstruct strbuf charset = STRBUF_INIT;\n> \tstruct strbuf buffer = STRBUF_INIT;\n> @@ -405,38 +457,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n> \tlast->buf = last->buf_alloc;\n> \n> -\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> -\tif (maybe_smart &&\n> -\t (5 <= last->len && last->buf[4] == '#') &&\n> -\t !strbuf_cmp(&exp, &type)) {\n> -\t\tchar *line;\n> -\n> -\t\t/*\n> -\t\t * smart HTTP response; validate that the service\n> -\t\t * pkt-line matches our request.\n> -\t\t */\n> -\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n> -\t\tif (!line)\n> -\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n> -\n> -\t\tstrbuf_reset(&exp);\n> -\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n> -\t\tif (strcmp(line, exp.buf))\n> -\t\t\tdie(\"invalid server response; got '%s'\", line);\n> -\t\tstrbuf_release(&exp);\n> -\n> -\t\t/* The header can include additional metadata lines, up\n> -\t\t * until a packet flush marker. Ignore these now, but\n> -\t\t * in the future we might start to scan them.\n> -\t\t */\n> -\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n> -\t\t\t;\n> -\n> -\t\tlast->proto_git = 1;\n> -\t} else if (maybe_smart &&\n> -\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> -\t\tlast->proto_git = 1;\n> -\t}\n> +\tif (maybe_smart)\n> +\t\tcheck_smart_http(last, service, &type);\n> \n> \tif (last->proto_git)\n> \t\tlast->refs = parse_git_refs(last, for_push);\n> @@ -444,7 +466,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t\tlast->refs = parse_info_refs(last);\n> \n> \tstrbuf_release(&refs_url);\n> -\tstrbuf_release(&exp);\n> \tstrbuf_release(&type);\n> \tstrbuf_release(&charset);\n> \tstrbuf_release(&effective_url);\n"},{"id":"363483","messageId":"20181116084427.GA31493@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181115215152.GK126896@google.com","subject":"[PATCH 0/3] remote-curl smart-http discovery cleanup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-16T08:44:27Z","receivedAt":"2018-11-16T08:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2018 at 01:51:52PM -0800, Josh Steadmon wrote:\n\n> > This patch tightens both of those (I also made a few stylistic tweaks,\n> > and added the ERR condition to show where it would go). I dunno. Part of\n> > me sees this as a nice cleanup, but maybe it is better to just leave it\n> > alone. A lot of these behaviors are just how it happens to work now, and\n> > not part of the spec, but we don't know what might be relying on them.\n> \n> At least according to the protocol-v2 and http-protocol docs, the\n> stricter behavior seems correct:\n> \n> For the first point above, dumb servers should never use an\n> \"application/x-git-*\" content type (http-protocol.txt line 163-167).\n> \n> For the second point, the docs require v2 servers to use\n> \"application/x-git-*\" content types. protocol-v2.txt lines 63-65 state\n> that v2 clients should make a smart http request, while\n> http-protocol.txt lines 247-252 state that a smart server's response\n> type must be \"application/x-git-*\".\n\nThanks for digging into the spec. I agree that it's pretty clear that\nthe appropriate content-type is expected.\n\n> Of course we don't know if other implementations follow the spec, but\n> ISTM that this patch at least doesn't contradict how we've promised the\n> protocols should work.\n\nThese seem like pretty unlikely ways for a buggy server to behave, so I\nthink it's a reasonable risk. I also checked GitHub's implementation\n(which recently learned to speak v2) and made sure that it works. :)\nI didn't check JGit, but given the provenance, I assume it's fine.\n\nAmusingly, this does break the test you just added, because it tries to\nissue an ERR after claiming \"text/html\" (and after my patch, we\ncorrectly fall back to dumb-http).\n\n> If no one has any objections, I'll include the diff below in v2. Thanks\n> for the help Jeff!\n\nI think it makes sense to do the refactoring first as a separate step.\nAnd of course it needs a commit message. So how about this series (your\noriginal is rebased on top)?\n\n [1/3]: remote-curl: refactor smart-http discovery\n [2/3]: remote-curl: tighten \"version 2\" check for smart-http\n [3/3]: remote-curl: die on server-side errors\n\n remote-curl.c | 96 +++++++++++++++++++++------------\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++\n t/lib-httpd/error-smart-http.sh | 3 ++\n t/t5551-http-fetch-smart.sh | 5 ++\n 5 files changed, 75 insertions(+), 34 deletions(-)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\n-Peff\n"},{"id":"363484","messageId":"20181116084725.GA31603@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181116084427.GA31493@sigill.intra.peff.net","subject":"[PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-16T08:47:25Z","receivedAt":"2018-11-16T08:47:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After making initial contact with an http server, we have to decide if\nthe server supports smart-http, and if so, which version. Our rules are\na bit inconsistent:\n\n 1. For v0, we require that the content-type indicates a smart-http\n response. We also require the response to look vaguely like a\n pkt-line starting with \"#\". If one of those does not match, we fall\n back to dumb-http.\n\n But according to our http protocol spec[1]:\n\n Dumb servers MUST NOT return a return type starting with\n `application/x-git-`.\n\n If we see the expected content-type, we should consider it\n smart-http. At that point we can parse the pkt-line for real, and\n complain if it is not syntactically valid.\n\n 2. For v2, we do not actually check the content-type. Our v2 protocol\n spec says[2]:\n\n When using the http:// or https:// transport a client makes a\n \"smart\" info/refs request as described in `http-protocol.txt`[...]\n\n and the http spec is clear that for a smart-http[3]:\n\n The Content-Type MUST be `application/x-$servicename-advertisement`.\n\n So it is required according to the spec.\n\nThese inconsistencies were easy to miss because of the way the original\ncode was written as an inline conditional. Let's pull it out into its\nown function for readability, and improve a few things:\n\n - we now predicate the smart/dumb decision entirely on the presence of\n the correct content-type\n\n - we do a real pkt-line parse before deciding how to proceed (and die\n if it isn't valid)\n\n - use skip_prefix() for comparing service strings, instead of\n constructing expected output in a strbuf; this avoids dealing with\n memory cleanup\n\nNote that this _is_ tightening what the client will allow. It's all\naccording to the spec, but it's possible that other implementations\nmight violate these. However, violating these particular rules seems\nlike an odd choice for a server to make.\n\n[1] Documentation/technical/http-protocol.txt, l. 166-167\n[2] Documentation/technical/protocol-v2.txt, l. 63-64\n[3] Documentation/technical/http-protocol.txt, l. 247\n\nHelped-by: Josh Steadmon \nSigned-off-by: Jeff King \n---\n remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 59 insertions(+), 34 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 762a55a75f..dd9bc41aa1 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version,\n \treturn 0;\n }\n \n+static void check_smart_http(struct discovery *d, const char *service,\n+\t\t\t struct strbuf *type)\n+{\n+\tchar *src_buf;\n+\tsize_t src_len;\n+\tchar *line;\n+\tconst char *p;\n+\n+\t/*\n+\t * If we don't see x-$service-advertisement, then it's not smart-http.\n+\t * But once we do, we commit to it and assume any other protocol\n+\t * violations are hard errors.\n+\t */\n+\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n+\t !skip_prefix(p, service, &p) ||\n+\t strcmp(p, \"-advertisement\"))\n+\t\treturn;\n+\n+\t/*\n+\t * \"Peek\" at the first packet by using a separate buf/len pair; some\n+\t * cases below require us leaving the originals intact.\n+\t */\n+\tsrc_buf = d->buf;\n+\tsrc_len = d->len;\n+\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n+\tif (!line)\n+\t\tdie(\"invalid server response; expected service, got flush packet\");\n+\n+\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n+\t\t/*\n+\t\t * The header can include additional metadata lines, up\n+\t\t * until a packet flush marker. Ignore these now, but\n+\t\t * in the future we might start to scan them.\n+\t\t */\n+\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n+\t\t\t;\n+\n+\t\t/*\n+\t\t * v0 smart http; callers expect us to soak up the\n+\t\t * service and header packets\n+\t\t */\n+\t\td->buf = src_buf;\n+\t\td->len = src_len;\n+\t\td->proto_git = 1;\n+\n+\t} else if (starts_with(line, \"version 2\")) {\n+\t\t/*\n+\t\t * v2 smart http; do not consume version packet, which will\n+\t\t * be handled elsewhere.\n+\t\t */\n+\t\td->proto_git = 1;\n+\n+\t} else {\n+\t\tdie(\"invalid server response; got '%s'\", line);\n+\t}\n+}\n+\n static struct discovery *discover_refs(const char *service, int for_push)\n {\n-\tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n@@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\tif (maybe_smart &&\n-\t (5 <= last->len && last->buf[4] == '#') &&\n-\t !strbuf_cmp(&exp, &type)) {\n-\t\tchar *line;\n-\n-\t\t/*\n-\t\t * smart HTTP response; validate that the service\n-\t\t * pkt-line matches our request.\n-\t\t */\n-\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n-\t\tif (!line)\n-\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n-\n-\t\tstrbuf_reset(&exp);\n-\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", line);\n-\t\tstrbuf_release(&exp);\n-\n-\t\t/* The header can include additional metadata lines, up\n-\t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n-\t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n-\t\t\t;\n-\n-\t\tlast->proto_git = 1;\n-\t} else if (maybe_smart &&\n-\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n-\t\tlast->proto_git = 1;\n-\t}\n+\tif (maybe_smart)\n+\t\tcheck_smart_http(last, service, &type);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\n@@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tlast->refs = parse_info_refs(last);\n \n \tstrbuf_release(&refs_url);\n-\tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n-- \n2.19.1.1636.gc7a073d580\n\n"},{"id":"363485","messageId":"20181116084838.GB31603@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181116084427.GA31493@sigill.intra.peff.net","subject":"[PATCH 2/3] remote-curl: tighten \"version 2\" check for smart-http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-16T08:48:38Z","receivedAt":"2018-11-16T08:48:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a v2 smart-http conversation, the server should reply to our initial\nrequest with a pkt-line saying \"version 2\" (this is the start of the\n\"capabilities advertisement\"). We check for the string using\nstarts_with(), but that's overly permissive (it would match \"version\n20\", for example).\n\nLet's tighten this check to use strcmp(). Note that we don't need to\nworry about a trailing newline here, because the ptk-line code will have\nchomped it for us already.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd9bc41aa1..3c9c4a07c3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,\n \t\td->len = src_len;\n \t\td->proto_git = 1;\n \n-\t} else if (starts_with(line, \"version 2\")) {\n+\t} else if (!strcmp(line, \"version 2\")) {\n \t\t/*\n \t\t * v2 smart http; do not consume version packet, which will\n \t\t * be handled elsewhere.\n-- \n2.19.1.1636.gc7a073d580\n\n"},{"id":"363486","messageId":"20181116084951.GC31603@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181116084427.GA31493@sigill.intra.peff.net","subject":"[PATCH 3/3] remote-curl: die on server-side errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-16T08:49:51Z","receivedAt":"2018-11-16T08:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Josh Steadmon \n\nWhen a smart HTTP server sends an error message via pkt-line,\nremote-curl will fail to detect the error (which usually results in\nincorrectly falling back to dumb-HTTP mode).\n\nThis patch adds a check in check_smart_http() for server-side error\nmessages, as well as a test case for this issue.\n\nSigned-off-by: Josh Steadmon \nSigned-off-by: Jeff King \n---\n remote-curl.c | 3 +++\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-smart-http.sh | 3 +++\n t/t5551-http-fetch-smart.sh | 5 +++++\n 5 files changed, 16 insertions(+)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 3c9c4a07c3..b1309f2bdc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -382,6 +382,9 @@ static void check_smart_http(struct discovery *d, const char *service,\n \t\t */\n \t\td->proto_git = 1;\n \n+\t} else if (skip_prefix(line, \"ERR \", &p)) {\n+\t\tdie(_(\"remote error: %s\"), p);\n+\n \t} else {\n \t\tdie(\"invalid server response; got '%s'\", line);\n \t}\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a8729f8232..4e946623bb 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -131,6 +131,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-sed.sh\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..6de2bc775c 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n Options ExecCGI\n \ndiff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh\nnew file mode 100644\nindex 0000000000..e65d447fc4\n--- /dev/null\n+++ b/t/lib-httpd/error-smart-http.sh\n@@ -0,0 +1,3 @@\n+echo \"Content-Type: application/x-git-upload-pack-advertisement\"\n+echo\n+printf \"%s\" \"0019ERR server-side error\"\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..ba83e567e5 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n \t! grep \"=> Send data\" err\n '\n \n+test_expect_success 'server-side error detected' '\n+\ttest_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&\n+\tgrep \"server-side error\" actual\n+'\n+\n stop_httpd\n test_done\n-- \n2.19.1.1636.gc7a073d580\n"},{"id":"363531","messageId":"20181116200421.GB9703@google.com","threadId":"49814","inReplyTo":"20181116084427.GA31493@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] remote-curl smart-http discovery cleanup","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-16T20:04:21Z","receivedAt":"2018-11-16T20:04:30Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.16 03:44, Jeff King wrote:\n[...]\n> Amusingly, this does break the test you just added, because it tries to\n> issue an ERR after claiming \"text/html\" (and after my patch, we\n> correctly fall back to dumb-http).\n\nHeh yeah, I copied the script from a dumb-http test without reading the\nspec.\n"},{"id":"363532","messageId":"20181116202734.GC9703@google.com","threadId":"49814","inReplyTo":"20181116084725.GA31603@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-16T20:27:34Z","receivedAt":"2018-11-16T20:27:44Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.16 03:47, Jeff King wrote:\n> After making initial contact with an http server, we have to decide if\n> the server supports smart-http, and if so, which version. Our rules are\n> a bit inconsistent:\n> \n> 1. For v0, we require that the content-type indicates a smart-http\n> response. We also require the response to look vaguely like a\n> pkt-line starting with \"#\". If one of those does not match, we fall\n> back to dumb-http.\n> \n> But according to our http protocol spec[1]:\n> \n> Dumb servers MUST NOT return a return type starting with\n> `application/x-git-`.\n> \n> If we see the expected content-type, we should consider it\n> smart-http. At that point we can parse the pkt-line for real, and\n> complain if it is not syntactically valid.\n> \n> 2. For v2, we do not actually check the content-type. Our v2 protocol\n> spec says[2]:\n> \n> When using the http:// or https:// transport a client makes a\n> \"smart\" info/refs request as described in `http-protocol.txt`[...]\n> \n> and the http spec is clear that for a smart-http[3]:\n> \n> The Content-Type MUST be `application/x-$servicename-advertisement`.\n> \n> So it is required according to the spec.\n> \n> These inconsistencies were easy to miss because of the way the original\n> code was written as an inline conditional. Let's pull it out into its\n> own function for readability, and improve a few things:\n> \n> - we now predicate the smart/dumb decision entirely on the presence of\n> the correct content-type\n> \n> - we do a real pkt-line parse before deciding how to proceed (and die\n> if it isn't valid)\n> \n> - use skip_prefix() for comparing service strings, instead of\n> constructing expected output in a strbuf; this avoids dealing with\n> memory cleanup\n> \n> Note that this _is_ tightening what the client will allow. It's all\n> according to the spec, but it's possible that other implementations\n> might violate these. However, violating these particular rules seems\n> like an odd choice for a server to make.\n> \n> [1] Documentation/technical/http-protocol.txt, l. 166-167\n> [2] Documentation/technical/protocol-v2.txt, l. 63-64\n> [3] Documentation/technical/http-protocol.txt, l. 247\n> \n> Helped-by: Josh Steadmon \n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 59 insertions(+), 34 deletions(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index 762a55a75f..dd9bc41aa1 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version,\n> \treturn 0;\n> }\n> \n> +static void check_smart_http(struct discovery *d, const char *service,\n> +\t\t\t struct strbuf *type)\n> +{\n> +\tchar *src_buf;\n> +\tsize_t src_len;\n> +\tchar *line;\n> +\tconst char *p;\n> +\n> +\t/*\n> +\t * If we don't see x-$service-advertisement, then it's not smart-http.\n> +\t * But once we do, we commit to it and assume any other protocol\n> +\t * violations are hard errors.\n> +\t */\n> +\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n> +\t !skip_prefix(p, service, &p) ||\n> +\t strcmp(p, \"-advertisement\"))\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * \"Peek\" at the first packet by using a separate buf/len pair; some\n> +\t * cases below require us leaving the originals intact.\n> +\t */\n> +\tsrc_buf = d->buf;\n> +\tsrc_len = d->len;\n> +\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n> +\tif (!line)\n> +\t\tdie(\"invalid server response; expected service, got flush packet\");\n> +\n> +\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n> +\t\t/*\n> +\t\t * The header can include additional metadata lines, up\n> +\t\t * until a packet flush marker. Ignore these now, but\n> +\t\t * in the future we might start to scan them.\n> +\t\t */\n> +\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n> +\t\t\t;\n> +\n> +\t\t/*\n> +\t\t * v0 smart http; callers expect us to soak up the\n> +\t\t * service and header packets\n> +\t\t */\n> +\t\td->buf = src_buf;\n> +\t\td->len = src_len;\n> +\t\td->proto_git = 1;\n> +\n> +\t} else if (starts_with(line, \"version 2\")) {\n> +\t\t/*\n> +\t\t * v2 smart http; do not consume version packet, which will\n> +\t\t * be handled elsewhere.\n> +\t\t */\n> +\t\td->proto_git = 1;\n> +\n> +\t} else {\n> +\t\tdie(\"invalid server response; got '%s'\", line);\n> +\t}\n> +}\n> +\n> static struct discovery *discover_refs(const char *service, int for_push)\n> {\n> -\tstruct strbuf exp = STRBUF_INIT;\n> \tstruct strbuf type = STRBUF_INIT;\n> \tstruct strbuf charset = STRBUF_INIT;\n> \tstruct strbuf buffer = STRBUF_INIT;\n> @@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n> \tlast->buf = last->buf_alloc;\n> \n> -\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> -\tif (maybe_smart &&\n> -\t (5 <= last->len && last->buf[4] == '#') &&\n> -\t !strbuf_cmp(&exp, &type)) {\n> -\t\tchar *line;\n> -\n> -\t\t/*\n> -\t\t * smart HTTP response; validate that the service\n> -\t\t * pkt-line matches our request.\n> -\t\t */\n> -\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n> -\t\tif (!line)\n> -\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n> -\n> -\t\tstrbuf_reset(&exp);\n> -\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n> -\t\tif (strcmp(line, exp.buf))\n> -\t\t\tdie(\"invalid server response; got '%s'\", line);\n> -\t\tstrbuf_release(&exp);\n> -\n> -\t\t/* The header can include additional metadata lines, up\n> -\t\t * until a packet flush marker. Ignore these now, but\n> -\t\t * in the future we might start to scan them.\n> -\t\t */\n> -\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n> -\t\t\t;\n> -\n> -\t\tlast->proto_git = 1;\n> -\t} else if (maybe_smart &&\n> -\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> -\t\tlast->proto_git = 1;\n> -\t}\n> +\tif (maybe_smart)\n> +\t\tcheck_smart_http(last, service, &type);\n> \n> \tif (last->proto_git)\n> \t\tlast->refs = parse_git_refs(last, for_push);\n> @@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t\tlast->refs = parse_info_refs(last);\n> \n> \tstrbuf_release(&refs_url);\n> -\tstrbuf_release(&exp);\n> \tstrbuf_release(&type);\n> \tstrbuf_release(&charset);\n> \tstrbuf_release(&effective_url);\n> -- \n> 2.19.1.1636.gc7a073d580\n> \n\nLooks good to me.\n\nReviewed-by: Josh Steadmon \n"},{"id":"363533","messageId":"20181116202800.GD9703@google.com","threadId":"49814","inReplyTo":"20181116084838.GB31603@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] remote-curl: tighten \"version 2\" check for smart-http","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-11-16T20:28:00Z","receivedAt":"2018-11-16T20:28:08Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.11.16 03:48, Jeff King wrote:\n> In a v2 smart-http conversation, the server should reply to our initial\n> request with a pkt-line saying \"version 2\" (this is the start of the\n> \"capabilities advertisement\"). We check for the string using\n> starts_with(), but that's overly permissive (it would match \"version\n> 20\", for example).\n> \n> Let's tighten this check to use strcmp(). Note that we don't need to\n> worry about a trailing newline here, because the ptk-line code will have\n> chomped it for us already.\n> \n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index dd9bc41aa1..3c9c4a07c3 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,\n> \t\td->len = src_len;\n> \t\td->proto_git = 1;\n> \n> -\t} else if (starts_with(line, \"version 2\")) {\n> +\t} else if (!strcmp(line, \"version 2\")) {\n> \t\t/*\n> \t\t * v2 smart http; do not consume version packet, which will\n> \t\t * be handled elsewhere.\n> -- \n> 2.19.1.1636.gc7a073d580\n> \n\nLooks good to me.\n\nReviewed-by: Josh Steadmon \n"},{"id":"365139","messageId":"cover.1544572142.git.steadmon@google.com","threadId":"49814","inReplyTo":"20181116084427.GA31493@sigill.intra.peff.net","subject":"[PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-12T00:25:14Z","receivedAt":"2018-12-12T00:25:24Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"This is a reroll of js/smart-http-detect-remote-error that also includes\na fixed version of ms/proto-err-packet-anywhere [1].\n\nThe first patch clarifies the use of ERR messages in the pkt-line\nprotocol and unifies error handling in pkt-line.c\n\nThe second patch refactors smart-http discovery in remote-curl.c. Among\nother improvements, it makes more use of the pkt-line functions, which\nallows us to catch remote errors that were previously ignored.\n\nThe third patch makes the version check in remote-curl more strict.\n\nThe final patch adds a test to verify that the fix in patch #2 does\nactually catch remote HTTP errors.\n\n[1]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/\n\nJeff King (2):\n remote-curl: refactor smart-http discovery\n remote-curl: tighten \"version 2\" check for smart-http\n\nJosh Steadmon (1):\n lib-httpd, t5551: check server-side HTTP errors\n\nMasaya Suzuki (1):\n pack-protocol.txt: accept error packets in any context\n\n Documentation/technical/pack-protocol.txt | 20 ++---\n builtin/archive.c | 2 -\n connect.c | 3 -\n fetch-pack.c | 2 -\n pkt-line.c | 4 +\n remote-curl.c | 93 ++++++++++++++---------\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 +\n t/lib-httpd/error-smart-http.sh | 3 +\n t/t5551-http-fetch-smart.sh | 5 ++\n t/t5703-upload-pack-ref-in-want.sh | 4 +-\n 11 files changed, 89 insertions(+), 52 deletions(-)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"365140","messageId":"df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com","threadId":"49814","inReplyTo":"cover.1544572142.git.steadmon@google.com","subject":"[PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-12T00:25:15Z","receivedAt":"2018-12-12T00:25:26Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Masaya Suzuki \n\nIn the Git pack protocol definition, an error packet may appear only in\na certain context. However, servers can face a runtime error (e.g. I/O\nerror) at an arbitrary timing. This patch changes the protocol to allow\nan error packet to be sent instead of any packet.\n\nFollowing this protocol spec change, the error packet handling code is\nmoved to pkt-line.c.\n\nSigned-off-by: Masaya Suzuki \nSigned-off-by: Josh Steadmon \n---\n Documentation/technical/pack-protocol.txt | 20 +++++++++++---------\n builtin/archive.c | 2 --\n connect.c | 3 ---\n fetch-pack.c | 2 --\n pkt-line.c | 4 ++++\n t/t5703-upload-pack-ref-in-want.sh | 4 ++--\n 6 files changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex 6ac774d5f6..7a2375a55d 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -22,6 +22,16 @@ protocol-common.txt. When the grammar indicate `PKT-LINE(...)`, unless\n otherwise noted the usual pkt-line LF rules apply: the sender SHOULD\n include a LF, but the receiver MUST NOT complain if it is not present.\n \n+An error packet is a special pkt-line that contains an error string.\n+\n+----\n+ error-line = PKT-LINE(\"ERR\" SP explanation-text)\n+----\n+\n+Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n+be sent. Once this packet is sent by a client or a server, the data transfer\n+process defined in this protocol is terminated.\n+\n Transports\n ----------\n There are three transports over which the packfile protocol is\n@@ -89,13 +99,6 @@ process on the server side over the Git protocol is this:\n \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n nc -v example.com 9418\n \n-If the server refuses the request for some reasons, it could abort\n-gracefully with an error message.\n-\n-----\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n-----\n-\n \n SSH Transport\n -------------\n@@ -398,12 +401,11 @@ from the client).\n Then the server will start sending its packfile data.\n \n ----\n- server-response = *ack_multi ack / nak / error-line\n+ server-response = *ack_multi ack / nak\n ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n ack_status = \"continue\" / \"common\" / \"ready\"\n ack = PKT-LINE(\"ACK\" SP obj-id)\n nak = PKT-LINE(\"NAK\")\n- error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n A simple clone may look like this (with no 'have' lines):\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex d2455237ce..5d179bbd16 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -59,8 +59,6 @@ static int run_remote_archiver(int argc, const char **argv,\n \tif (strcmp(buf, \"ACK\")) {\n \t\tif (starts_with(buf, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n-\t\tif (starts_with(buf, \"ERR \"))\n-\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \ndiff --git a/connect.c b/connect.c\nindex 24281b6082..4813f005ab 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -296,7 +296,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \tstruct ref **orig_list = list;\n \tint len = 0;\n \tenum get_remote_heads_state state = EXPECTING_FIRST_REF;\n-\tconst char *arg;\n \n \t*list = NULL;\n \n@@ -306,8 +305,6 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \t\t\tdie_initial_contact(1);\n \t\tcase PACKET_READ_NORMAL:\n \t\t\tlen = reader->pktlen;\n-\t\t\tif (len > 4 && skip_prefix(reader->line, \"ERR \", &arg))\n-\t\t\t\tdie(_(\"remote error: %s\"), arg);\n \t\t\tbreak;\n \t\tcase PACKET_READ_FLUSH:\n \t\t\tstate = EXPECTING_DONE;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 9691046e64..e66cd7b71b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -178,8 +178,6 @@ static enum ack_type get_ack(int fd, struct object_id *result_oid)\n \t\t\treturn ACK;\n \t\t}\n \t}\n-\tif (skip_prefix(line, \"ERR \", &arg))\n-\t\tdie(_(\"remote error: %s\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n }\n \ndiff --git a/pkt-line.c b/pkt-line.c\nindex 04d10bbd03..ce9e42d10e 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n \t\treturn PACKET_READ_EOF;\n \t}\n \n+\tif (starts_with(buffer, \"ERR \")) {\n+\t\tdie(_(\"remote error: %s\"), buffer + 4);\n+\t}\n+\n \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n \t len && buffer[len-1] == '\\n')\n \t\tlen--;\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex 3f58f05cbb..d2a9d0c127 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -208,7 +208,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n \tcp -r \"$LOCAL_PRISTINE\" local &&\n \tinconsistency master 1234567890123456789012345678901234567890 &&\n \ttest_must_fail git -C local fetch 2>err &&\n-\tgrep \"ERR upload-pack: not our ref\" err\n+\tgrep \"fatal: remote error: upload-pack: not our ref\" err\n '\n \n test_expect_success 'server is initially ahead - ref in want' '\n@@ -254,7 +254,7 @@ test_expect_success 'server loses a ref - ref in want' '\n \techo \"s/master/raster/\" >\"$HTTPD_ROOT_PATH/one-time-sed\" &&\n \ttest_must_fail git -C local fetch 2>err &&\n \n-\tgrep \"ERR unknown ref refs/heads/raster\" err\n+\tgrep \"fatal: remote error: unknown ref refs/heads/raster\" err\n '\n \n stop_httpd\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"365141","messageId":"27da5b6e12f488b4503d0998377dc3299eaccca4.1544572142.git.steadmon@google.com","threadId":"49814","inReplyTo":"cover.1544572142.git.steadmon@google.com","subject":"[PATCH v3 2/4] remote-curl: refactor smart-http discovery","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-12T00:25:16Z","receivedAt":"2018-12-12T00:25:28Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Jeff King \n\nAfter making initial contact with an http server, we have to decide if\nthe server supports smart-http, and if so, which version. Our rules are\na bit inconsistent:\n\n 1. For v0, we require that the content-type indicates a smart-http\n response. We also require the response to look vaguely like a\n pkt-line starting with \"#\". If one of those does not match, we fall\n back to dumb-http.\n\n But according to our http protocol spec[1]:\n\n Dumb servers MUST NOT return a return type starting with\n `application/x-git-`.\n\n If we see the expected content-type, we should consider it\n smart-http. At that point we can parse the pkt-line for real, and\n complain if it is not syntactically valid.\n\n 2. For v2, we do not actually check the content-type. Our v2 protocol\n spec says[2]:\n\n When using the http:// or https:// transport a client makes a\n \"smart\" info/refs request as described in `http-protocol.txt`[...]\n\n and the http spec is clear that for a smart-http[3]:\n\n The Content-Type MUST be `application/x-$servicename-advertisement`.\n\n So it is required according to the spec.\n\nThese inconsistencies were easy to miss because of the way the original\ncode was written as an inline conditional. Let's pull it out into its\nown function for readability, and improve a few things:\n\n - we now predicate the smart/dumb decision entirely on the presence of\n the correct content-type\n\n - we do a real pkt-line parse before deciding how to proceed (and die\n if it isn't valid)\n\n - use skip_prefix() for comparing service strings, instead of\n constructing expected output in a strbuf; this avoids dealing with\n memory cleanup\n\nNote that this _is_ tightening what the client will allow. It's all\naccording to the spec, but it's possible that other implementations\nmight violate these. However, violating these particular rules seems\nlike an odd choice for a server to make.\n\n[1] Documentation/technical/http-protocol.txt, l. 166-167\n[2] Documentation/technical/protocol-v2.txt, l. 63-64\n[3] Documentation/technical/http-protocol.txt, l. 247\n\nHelped-by: Josh Steadmon \nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \nSigned-off-by: Josh Steadmon \n---\n remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 59 insertions(+), 34 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcdc..38f51dffb8 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -330,9 +330,65 @@ static int get_protocol_http_header(enum protocol_version version,\n \treturn 0;\n }\n \n+static void check_smart_http(struct discovery *d, const char *service,\n+\t\t\t struct strbuf *type)\n+{\n+\tchar *src_buf;\n+\tsize_t src_len;\n+\tchar *line;\n+\tconst char *p;\n+\n+\t/*\n+\t * If we don't see x-$service-advertisement, then it's not smart-http.\n+\t * But once we do, we commit to it and assume any other protocol\n+\t * violations are hard errors.\n+\t */\n+\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n+\t !skip_prefix(p, service, &p) ||\n+\t strcmp(p, \"-advertisement\"))\n+\t\treturn;\n+\n+\t/*\n+\t * \"Peek\" at the first packet by using a separate buf/len pair; some\n+\t * cases below require us leaving the originals intact.\n+\t */\n+\tsrc_buf = d->buf;\n+\tsrc_len = d->len;\n+\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n+\tif (!line)\n+\t\tdie(\"invalid server response; expected service, got flush packet\");\n+\n+\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n+\t\t/*\n+\t\t * The header can include additional metadata lines, up\n+\t\t * until a packet flush marker. Ignore these now, but\n+\t\t * in the future we might start to scan them.\n+\t\t */\n+\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n+\t\t\t;\n+\n+\t\t/*\n+\t\t * v0 smart http; callers expect us to soak up the\n+\t\t * service and header packets\n+\t\t */\n+\t\td->buf = src_buf;\n+\t\td->len = src_len;\n+\t\td->proto_git = 1;\n+\n+\t} else if (starts_with(line, \"version 2\")) {\n+\t\t/*\n+\t\t * v2 smart http; do not consume version packet, which will\n+\t\t * be handled elsewhere.\n+\t\t */\n+\t\td->proto_git = 1;\n+\n+\t} else {\n+\t\tdie(\"invalid server response; got '%s'\", line);\n+\t}\n+}\n+\n static struct discovery *discover_refs(const char *service, int for_push)\n {\n-\tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n@@ -405,38 +461,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\tif (maybe_smart &&\n-\t (5 <= last->len && last->buf[4] == '#') &&\n-\t !strbuf_cmp(&exp, &type)) {\n-\t\tchar *line;\n-\n-\t\t/*\n-\t\t * smart HTTP response; validate that the service\n-\t\t * pkt-line matches our request.\n-\t\t */\n-\t\tline = packet_read_line_buf(&last->buf, &last->len, NULL);\n-\t\tif (!line)\n-\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n-\n-\t\tstrbuf_reset(&exp);\n-\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", line);\n-\t\tstrbuf_release(&exp);\n-\n-\t\t/* The header can include additional metadata lines, up\n-\t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n-\t\t */\n-\t\twhile (packet_read_line_buf(&last->buf, &last->len, NULL))\n-\t\t\t;\n-\n-\t\tlast->proto_git = 1;\n-\t} else if (maybe_smart &&\n-\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n-\t\tlast->proto_git = 1;\n-\t}\n+\tif (maybe_smart)\n+\t\tcheck_smart_http(last, service, &type);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\n@@ -444,7 +470,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tlast->refs = parse_info_refs(last);\n \n \tstrbuf_release(&refs_url);\n-\tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"365142","messageId":"dc995b3d17cd62dfc19964dd8a95811fe928aa57.1544572142.git.steadmon@google.com","threadId":"49814","inReplyTo":"cover.1544572142.git.steadmon@google.com","subject":"[PATCH v3 3/4] remote-curl: tighten \"version 2\" check for smart-http","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-12T00:25:17Z","receivedAt":"2018-12-12T00:25:30Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"From: Jeff King \n\nIn a v2 smart-http conversation, the server should reply to our initial\nrequest with a pkt-line saying \"version 2\" (this is the start of the\n\"capabilities advertisement\"). We check for the string using\nstarts_with(), but that's overly permissive (it would match \"version\n20\", for example).\n\nLet's tighten this check to use strcmp(). Note that we don't need to\nworry about a trailing newline here, because the ptk-line code will have\nchomped it for us already.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \nSigned-off-by: Josh Steadmon \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 38f51dffb8..b77b173f33 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -375,7 +375,7 @@ static void check_smart_http(struct discovery *d, const char *service,\n \t\td->len = src_len;\n \t\td->proto_git = 1;\n \n-\t} else if (starts_with(line, \"version 2\")) {\n+\t} else if (!strcmp(line, \"version 2\")) {\n \t\t/*\n \t\t * v2 smart http; do not consume version packet, which will\n \t\t * be handled elsewhere.\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"365143","messageId":"22a1615030b562452a06be9d10a515eab96f4bc4.1544572142.git.steadmon@google.com","threadId":"49814","inReplyTo":"cover.1544572142.git.steadmon@google.com","subject":"[PATCH v3 4/4] lib-httpd, t5551: check server-side HTTP errors","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-12T00:25:18Z","receivedAt":"2018-12-12T00:25:32Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Add an HTTP path to the test server config that returns an ERR pkt-line\nunconditionally. Verify in t5551 that remote-curl properly detects this\nERR message and aborts.\n\nSigned-off-by: Josh Steadmon \nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-smart-http.sh | 3 +++\n t/t5551-http-fetch-smart.sh | 5 +++++\n 4 files changed, 13 insertions(+)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex a8729f8232..4e946623bb 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -131,6 +131,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-sed.sh\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..6de2bc775c 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -117,6 +117,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n@@ -125,6 +126,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n Options ExecCGI\n \ndiff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh\nnew file mode 100644\nindex 0000000000..e65d447fc4\n--- /dev/null\n+++ b/t/lib-httpd/error-smart-http.sh\n@@ -0,0 +1,3 @@\n+echo \"Content-Type: application/x-git-upload-pack-advertisement\"\n+echo\n+printf \"%s\" \"0019ERR server-side error\"\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..ba83e567e5 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n \t! grep \"=> Send data\" err\n '\n \n+test_expect_success 'server-side error detected' '\n+\ttest_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&\n+\tgrep \"server-side error\" actual\n+'\n+\n stop_httpd\n test_done\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"365163","messageId":"xmqqsgz3i18b.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"cover.1544572142.git.steadmon@google.com","subject":"Re: [PATCH v3 0/4] Unify pkt-line error handling and refactor smart-http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-12T08:43:48Z","receivedAt":"2018-12-12T08:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n> This is a reroll of js/smart-http-detect-remote-error that also includes\n> a fixed version of ms/proto-err-packet-anywhere [1].\n\nYay. Thanks for reducing a topic I have to worry about by 1 ;-).\n\n> The first patch clarifies the use of ERR messages in the pkt-line\n> protocol and unifies error handling in pkt-line.c\n>\n> The second patch refactors smart-http discovery in remote-curl.c. Among\n> other improvements, it makes more use of the pkt-line functions, which\n> allows us to catch remote errors that were previously ignored.\n>\n> The third patch makes the version check in remote-curl more strict.\n>\n> The final patch adds a test to verify that the fix in patch #2 does\n> actually catch remote HTTP errors.\n\nThanks. All look sensible. Will queue.\n\n>\n> [1]: https://public-inbox.org/git/20181127045301.103807-1-masayasuzuki@google.com/\n>\n> Jeff King (2):\n> remote-curl: refactor smart-http discovery\n> remote-curl: tighten \"version 2\" check for smart-http\n>\n> Josh Steadmon (1):\n> lib-httpd, t5551: check server-side HTTP errors\n>\n> Masaya Suzuki (1):\n> pack-protocol.txt: accept error packets in any context\n>\n> Documentation/technical/pack-protocol.txt | 20 ++---\n> builtin/archive.c | 2 -\n> connect.c | 3 -\n> fetch-pack.c | 2 -\n> pkt-line.c | 4 +\n> remote-curl.c | 93 ++++++++++++++---------\n> t/lib-httpd.sh | 1 +\n> t/lib-httpd/apache.conf | 4 +\n> t/lib-httpd/error-smart-http.sh | 3 +\n> t/t5551-http-fetch-smart.sh | 5 ++\n> t/t5703-upload-pack-ref-in-want.sh | 4 +-\n> 11 files changed, 89 insertions(+), 52 deletions(-)\n> create mode 100644 t/lib-httpd/error-smart-http.sh\n"},{"id":"365166","messageId":"20181212110206.GA30673@sigill.intra.peff.net","threadId":"49814","inReplyTo":"df7d3659ae5f11d163f1e992f3b9403be709ddb7.1544572142.git.steadmon@google.com","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-12T11:02:07Z","receivedAt":"2018-12-12T11:02:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 11, 2018 at 04:25:15PM -0800, Josh Steadmon wrote:\n\n> From: Masaya Suzuki \n> \n> In the Git pack protocol definition, an error packet may appear only in\n> a certain context. However, servers can face a runtime error (e.g. I/O\n> error) at an arbitrary timing. This patch changes the protocol to allow\n> an error packet to be sent instead of any packet.\n> \n> Following this protocol spec change, the error packet handling code is\n> moved to pkt-line.c.\n\nThis is a change in the spec with an accompanying change in the code,\nwhich raises the question: what do other implementations do with this\nchange (both older Git, and implementations like JGit, libgit2, etc)?\n\nI think the answer for older Git is \"hang up unceremoniously\", which is\nprobably OK given the semantics of the change. And I'd suspect most\nother implementations would do the same. I just wonder if anybody tested\nit with other implementations.\n\n> +An error packet is a special pkt-line that contains an error string.\n> +\n> +----\n> + error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> +----\n> +\n> +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n> +be sent. Once this packet is sent by a client or a server, the data transfer\n> +process defined in this protocol is terminated.\n\nThe packfile data is typically packetized, too, and contains arbitrary\ndata (that could have \"ERR\" in it). It looks like we don't specifically\nsay PKT-LINE() in that part of the protocol spec, though, so I think\nthis is OK.\n\nLikewise, in the implementation:\n\n> diff --git a/pkt-line.c b/pkt-line.c\n> index 04d10bbd03..ce9e42d10e 100644\n> --- a/pkt-line.c\n> +++ b/pkt-line.c\n> @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> \t\treturn PACKET_READ_EOF;\n> \t}\n> \n> +\tif (starts_with(buffer, \"ERR \")) {\n> +\t\tdie(_(\"remote error: %s\"), buffer + 4);\n> +\t}\n> +\n> \tif ((options & PACKET_READ_CHOMP_NEWLINE) &&\n> \t len && buffer[len-1] == '\\n')\n> \t\tlen--;\n\nThis ERR handling has been moved to a very low level. What happens if\nwe're passing arbitrary data via the packet_read() code? Could we\nerroneously trigger an error if a packfile happens to have the bytes\n\"ERR \" at a packet boundary?\n\nFor packfiles via upload-pack, I _think_ we're OK, because we only\npacketize it when a sideband is in use. In which case this would never\nmatch, because we'd have \"\\1\" in the first byte slot.\n\nBut are there are other cases we need to worry about? Just\nbrainstorming, I can think of:\n\n 1. We also pass packetized packfiles between git-remote-https and\n the stateless-rpc mode of fetch-pack/send-pack. And I don't think\n we use sidebands there.\n\n 2. The packet code is used for long-lived clean/smudge filters these\n days, which also pass arbitrary data.\n\nSo I think it's probably not a good idea to unconditionally have callers\nof packet_read_with_status() handle this. We'd need a flag like\nPACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n\n-Peff\n"},{"id":"365210","messageId":"CAJB1erXRqQW0yQyZutJAJKC7WbdVhBAYUMWM+8ZutxA-W-7S8w@mail.gmail.com","threadId":"49814","inReplyTo":"20181212110206.GA30673@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-13T01:17:01Z","receivedAt":"2018-12-13T01:17:17Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Wed, Dec 12, 2018 at 3:02 AM Jeff King wrote:\n>\n> On Tue, Dec 11, 2018 at 04:25:15PM -0800, Josh Steadmon wrote:\n>\n> > From: Masaya Suzuki \n> >\n> > In the Git pack protocol definition, an error packet may appear only in\n> > a certain context. However, servers can face a runtime error (e.g. I/O\n> > error) at an arbitrary timing. This patch changes the protocol to allow\n> > an error packet to be sent instead of any packet.\n> >\n> > Following this protocol spec change, the error packet handling code is\n> > moved to pkt-line.c.\n>\n> This is a change in the spec with an accompanying change in the code,\n> which raises the question: what do other implementations do with this\n> change (both older Git, and implementations like JGit, libgit2, etc)?\n\nJGit is similar to Git. It parses \"ERR \" in limited places. When it sees an ERR\npacket in an unexpected place, it'll fail somewhere in the parsing code.\n\nhttps://github.com/eclipse/jgit/blob/30c6c7542190c149e2aee792f992a312a5fc5793/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java#L145-L147\nhttps://github.com/eclipse/jgit/blob/f40b39345cd9b54473ee871bff401fe3d394ffe3/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java#L208\n\nI'm not familiar with libgit2 code, but it seems it handles this at a\nlower level. An error type packet is parsed out at a low level, and\nthe error handling is done by the callers of the packet parser.\n\nhttps://github.com/libgit2/libgit2/blob/bea65980c7a42e34edfafbdc40b199ba7b2a564e/src/transports/smart_pkt.c#L482-L483\n\nI cannot find an ERR packet handling in go-git. It seems to me that if\nan ERR packet appears it treats it as a parsing error.\n\nhttps://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/common.go#L60-L62\n\n>\n> I think the answer for older Git is \"hang up unceremoniously\", which is\n> probably OK given the semantics of the change. And I'd suspect most\n> other implementations would do the same. I just wonder if anybody tested\n> it with other implementations.\n\nI'm thinking aloud here. There would be two aspects of the protocol\ncompatibility: (1) new clients speak to old servers (2) old clients\nspeak to a new server that speaks the updated protocol.\n\nFor (1), I assume that in the Git pack protocol, a packet starting\nfrom \"ERR \" does not appear naturally except for a very special case\nthat the server doesn't support sideband, but using the updated\nprotocol. As you mentioned, at first it looks like this can mistakenly\nparse the pack file of git-receive-pack as an ERR packet, assuming\nthat git-receive-pack's pack file is packetized. Actually\ngit-receive-pack's pack file is not packetized in the Git pack\nprotocol (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1695).\nI recently wrote a Git protocol parser\n(https://github.com/google/gitprotocolio), and I confirmed that this\nis the case at least for the HTTP transport. git-upload-pack's pack\nfile is indeed packetized, but packetized with sideband. Except for\nthe case where sideband is not used, the packfiles wouldn't be\nconsidered as an ERR packet accidentally.\n\nFor (2), if the old clients see an unexpected ERR packet, they cannot\nparse it. They would handle this unparsable data as if the server is\nnot speaking Git protocol correctly. Even if the old clients just\nignore the packet, due to the nature of the ERR packet, the server\nwon't send further data. The client won't be able to proceed. Overall,\nthe clients anyway face an error, and the only difference would be\nwhether the clients can show an error nicely or not. The new clients\nwill show the errors nicely to users. Old clients will not.\n\n>\n> > +An error packet is a special pkt-line that contains an error string.\n> > +\n> > +----\n> > + error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> > +----\n> > +\n> > +Throughout the protocol, where `PKT-LINE(...)` is expected, an error packet MAY\n> > +be sent. Once this packet is sent by a client or a server, the data transfer\n> > +process defined in this protocol is terminated.\n>\n> The packfile data is typically packetized, too, and contains arbitrary\n> data (that could have \"ERR\" in it). It looks like we don't specifically\n> say PKT-LINE() in that part of the protocol spec, though, so I think\n> this is OK.\n\nAs I described above, as far as I can see, the packfile in\ngit-upload-pack is not packetized. The packfile in git-receive-pack is\npacketized but typically with sideband. At least at the Git pack\nprotocol level, this should be OK.\n\n>\n> Likewise, in the implementation:\n>\n> > diff --git a/pkt-line.c b/pkt-line.c\n> > index 04d10bbd03..ce9e42d10e 100644\n> > --- a/pkt-line.c\n> > +++ b/pkt-line.c\n> > @@ -346,6 +346,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,\n> > return PACKET_READ_EOF;\n> > }\n> >\n> > + if (starts_with(buffer, \"ERR \")) {\n> > + die(_(\"remote error: %s\"), buffer + 4);\n> > + }\n> > +\n> > if ((options & PACKET_READ_CHOMP_NEWLINE) &&\n> > len && buffer[len-1] == '\\n')\n> > len--;\n>\n> This ERR handling has been moved to a very low level. What happens if\n> we're passing arbitrary data via the packet_read() code? Could we\n> erroneously trigger an error if a packfile happens to have the bytes\n> \"ERR \" at a packet boundary?\n>\n> For packfiles via upload-pack, I _think_ we're OK, because we only\n> packetize it when a sideband is in use. In which case this would never\n> match, because we'd have \"\\1\" in the first byte slot.\n>\n> But are there are other cases we need to worry about? Just\n> brainstorming, I can think of:\n>\n> 1. We also pass packetized packfiles between git-remote-https and\n> the stateless-rpc mode of fetch-pack/send-pack. And I don't think\n> we use sidebands there.\n>\n> 2. The packet code is used for long-lived clean/smudge filters these\n> days, which also pass arbitrary data.\n>\n> So I think it's probably not a good idea to unconditionally have callers\n> of packet_read_with_status() handle this. We'd need a flag like\n> PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n\nThis is outside of the Git pack protocol so having a separate parsing\nmode makes sense to me.\n\n>\n> -Peff\n"},{"id":"365231","messageId":"20181213080422.GA12132@sigill.intra.peff.net","threadId":"49814","inReplyTo":"CAJB1erXRqQW0yQyZutJAJKC7WbdVhBAYUMWM+8ZutxA-W-7S8w@mail.gmail.com","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-13T08:04:22Z","receivedAt":"2018-12-13T08:04:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 12, 2018 at 05:17:01PM -0800, Masaya Suzuki wrote:\n\n> > This is a change in the spec with an accompanying change in the code,\n> > which raises the question: what do other implementations do with this\n> > change (both older Git, and implementations like JGit, libgit2, etc)?\n> \n> JGit is similar to Git. It parses \"ERR \" in limited places. When it sees an ERR\n> packet in an unexpected place, it'll fail somewhere in the parsing code.\n> \n> https://github.com/eclipse/jgit/blob/30c6c7542190c149e2aee792f992a312a5fc5793/org.eclipse.jgit/src/org/eclipse/jgit/transport/PacketLineIn.java#L145-L147\n> https://github.com/eclipse/jgit/blob/f40b39345cd9b54473ee871bff401fe3d394ffe3/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java#L208\n> \n> I'm not familiar with libgit2 code, but it seems it handles this at a\n> lower level. An error type packet is parsed out at a low level, and\n> the error handling is done by the callers of the packet parser.\n> \n> https://github.com/libgit2/libgit2/blob/bea65980c7a42e34edfafbdc40b199ba7b2a564e/src/transports/smart_pkt.c#L482-L483\n> \n> I cannot find an ERR packet handling in go-git. It seems to me that if\n> an ERR packet appears it treats it as a parsing error.\n> \n> https://github.com/src-d/go-git/blob/master/plumbing/protocol/packp/common.go#L60-L62\n\nThanks for digging into these. It does make sense that other\nimplementations would give a parsing error. Hopefully they also produce\na sensible error message (ideally printing the bogus pktline), but even\nif they don't we're probably no worse off than the status quo. With the\ncurrent scheme, the server can't give any message, and just has to hang\nup anyway.\n\n> > I think the answer for older Git is \"hang up unceremoniously\", which is\n> > probably OK given the semantics of the change. And I'd suspect most\n> > other implementations would do the same. I just wonder if anybody tested\n> > it with other implementations.\n> \n> I'm thinking aloud here. There would be two aspects of the protocol\n> compatibility: (1) new clients speak to old servers (2) old clients\n> speak to a new server that speaks the updated protocol.\n> \n> For (1), I assume that in the Git pack protocol, a packet starting\n> from \"ERR \" does not appear naturally except for a very special case\n> that the server doesn't support sideband, but using the updated\n> protocol. As you mentioned, at first it looks like this can mistakenly\n> parse the pack file of git-receive-pack as an ERR packet, assuming\n> that git-receive-pack's pack file is packetized. Actually\n> git-receive-pack's pack file is not packetized in the Git pack\n> protocol (https://github.com/git/git/blob/master/builtin/receive-pack.c#L1695).\n> I recently wrote a Git protocol parser\n> (https://github.com/google/gitprotocolio), and I confirmed that this\n> is the case at least for the HTTP transport. git-upload-pack's pack\n> file is indeed packetized, but packetized with sideband. Except for\n> the case where sideband is not used, the packfiles wouldn't be\n> considered as an ERR packet accidentally.\n\nRight, that matches my understanding.\n\n> For (2), if the old clients see an unexpected ERR packet, they cannot\n> parse it. They would handle this unparsable data as if the server is\n> not speaking Git protocol correctly. Even if the old clients just\n> ignore the packet, due to the nature of the ERR packet, the server\n> won't send further data. The client won't be able to proceed. Overall,\n> the clients anyway face an error, and the only difference would be\n> whether the clients can show an error nicely or not. The new clients\n> will show the errors nicely to users. Old clients will not.\n\nYeah, this was the case I was more concerned about, and I think it is\nprobably OK (by this rationale, and what I wrote above).\n\n> > So I think it's probably not a good idea to unconditionally have callers\n> > of packet_read_with_status() handle this. We'd need a flag like\n> > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n> \n> This is outside of the Git pack protocol so having a separate parsing\n> mode makes sense to me.\n\nYeah. Here's a sample script which works with current Git (the index\ncontains the uppercased content \"ERR FOO\"), but fails after this patch\n(Git thinks the filter reported an error and dies; it's not great that\nwe die in the packet-reading code at all for this case, but your patch\nis hardly the first call to die() in that function).\n\n-- >8 --\ngit init -q repo\ncd repo\n\necho '*.magic filter=magic' >.git/info/attributes\ngit config filter.magic.process $PWD/filter\n\n# toy filter to uppercase content\ncat >filter <<-\\EOF\n#!/usr/bin/perl\nsub read_pkt {\n my @r;\n while (1) {\n read(STDIN, my $len, 4);\n last if $len eq \"0000\";\n read(STDIN, my $buf, hex($len)-4);\n push @r, $buf;\n }\n return @r;\n}\nsub write_pkt {\n local $| = 1;\n while (@_) {\n my $buf = shift;\n printf \"%04x\", length($buf) + 4;\n print $buf;\n }\n print \"0000\";\n}\n\nread_pkt(); # handshake\nwrite_pkt(qw(git-filter-server version=2));\nread_pkt(); # capabilities\nwrite_pkt(qw(capability=clean));\n\nread_pkt(); # clean command\n@content = read_pkt();\n\nwrite_pkt(qw(status=success));\nwrite_pkt(map { uc } @content);\nwrite_pkt(); # final status\nEOF\nchmod +x filter\n\necho 'err foo' >foo.magic\ngit add foo.magic\ngit cat-file blob :foo.magic\n"},{"id":"365310","messageId":"20181213221826.GE37614@google.com","threadId":"49814","inReplyTo":"CAJB1erXRqQW0yQyZutJAJKC7WbdVhBAYUMWM+8ZutxA-W-7S8w@mail.gmail.com","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-13T22:18:26Z","receivedAt":"2018-12-13T22:18:35Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.12 17:17, Masaya Suzuki wrote:\n> On Wed, Dec 12, 2018 at 3:02 AM Jeff King wrote:\n> > This ERR handling has been moved to a very low level. What happens if\n> > we're passing arbitrary data via the packet_read() code? Could we\n> > erroneously trigger an error if a packfile happens to have the bytes\n> > \"ERR \" at a packet boundary?\n> >\n> > For packfiles via upload-pack, I _think_ we're OK, because we only\n> > packetize it when a sideband is in use. In which case this would never\n> > match, because we'd have \"\\1\" in the first byte slot.\n> >\n> > But are there are other cases we need to worry about? Just\n> > brainstorming, I can think of:\n> >\n> > 1. We also pass packetized packfiles between git-remote-https and\n> > the stateless-rpc mode of fetch-pack/send-pack. And I don't think\n> > we use sidebands there.\n> >\n> > 2. The packet code is used for long-lived clean/smudge filters these\n> > days, which also pass arbitrary data.\n> >\n> > So I think it's probably not a good idea to unconditionally have callers\n> > of packet_read_with_status() handle this. We'd need a flag like\n> > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n> \n> This is outside of the Git pack protocol so having a separate parsing\n> mode makes sense to me.\n\nThis sounds like it could be a significant refactoring. Should we go\nback to V2 of this series, and then work on the new parsing mode\nseparately?\n"},{"id":"365498","messageId":"20181217213310.GA14251@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181213221826.GE37614@google.com","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-17T21:33:10Z","receivedAt":"2018-12-17T21:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2018 at 02:18:26PM -0800, Josh Steadmon wrote:\n\n> On 2018.12.12 17:17, Masaya Suzuki wrote:\n> > On Wed, Dec 12, 2018 at 3:02 AM Jeff King wrote:\n> > > This ERR handling has been moved to a very low level. What happens if\n> > > we're passing arbitrary data via the packet_read() code? Could we\n> > > erroneously trigger an error if a packfile happens to have the bytes\n> > > \"ERR \" at a packet boundary?\n> > >\n> > > For packfiles via upload-pack, I _think_ we're OK, because we only\n> > > packetize it when a sideband is in use. In which case this would never\n> > > match, because we'd have \"\\1\" in the first byte slot.\n> > >\n> > > But are there are other cases we need to worry about? Just\n> > > brainstorming, I can think of:\n> > >\n> > > 1. We also pass packetized packfiles between git-remote-https and\n> > > the stateless-rpc mode of fetch-pack/send-pack. And I don't think\n> > > we use sidebands there.\n> > >\n> > > 2. The packet code is used for long-lived clean/smudge filters these\n> > > days, which also pass arbitrary data.\n> > >\n> > > So I think it's probably not a good idea to unconditionally have callers\n> > > of packet_read_with_status() handle this. We'd need a flag like\n> > > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n> > \n> > This is outside of the Git pack protocol so having a separate parsing\n> > mode makes sense to me.\n> \n> This sounds like it could be a significant refactoring. Should we go\n> back to V2 of this series, and then work on the new parsing mode\n> separately?\n\nWhich one is v2? :)\n\nJust the remote-curl cleanups from me, and then your \"die on server-side\nerrors\" patch?\n\n-Peff\n"},{"id":"365610","messageId":"20181219233005.GI37614@google.com","threadId":"49814","inReplyTo":"20181217213310.GA14251@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2018-12-19T23:30:05Z","receivedAt":"2018-12-19T23:30:13Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2018.12.17 16:33, Jeff King wrote:\n> On Thu, Dec 13, 2018 at 02:18:26PM -0800, Josh Steadmon wrote:\n> \n> > On 2018.12.12 17:17, Masaya Suzuki wrote:\n> > > On Wed, Dec 12, 2018 at 3:02 AM Jeff King wrote:\n> > > > This ERR handling has been moved to a very low level. What happens if\n> > > > we're passing arbitrary data via the packet_read() code? Could we\n> > > > erroneously trigger an error if a packfile happens to have the bytes\n> > > > \"ERR \" at a packet boundary?\n> > > >\n> > > > For packfiles via upload-pack, I _think_ we're OK, because we only\n> > > > packetize it when a sideband is in use. In which case this would never\n> > > > match, because we'd have \"\\1\" in the first byte slot.\n> > > >\n> > > > But are there are other cases we need to worry about? Just\n> > > > brainstorming, I can think of:\n> > > >\n> > > > 1. We also pass packetized packfiles between git-remote-https and\n> > > > the stateless-rpc mode of fetch-pack/send-pack. And I don't think\n> > > > we use sidebands there.\n> > > >\n> > > > 2. The packet code is used for long-lived clean/smudge filters these\n> > > > days, which also pass arbitrary data.\n> > > >\n> > > > So I think it's probably not a good idea to unconditionally have callers\n> > > > of packet_read_with_status() handle this. We'd need a flag like\n> > > > PACKET_READ_RESPECT_ERR, and to trigger it from the appropriate callers.\n> > > \n> > > This is outside of the Git pack protocol so having a separate parsing\n> > > mode makes sense to me.\n> > \n> > This sounds like it could be a significant refactoring. Should we go\n> > back to V2 of this series, and then work on the new parsing mode\n> > separately?\n> \n> Which one is v2? :)\n> \n> Just the remote-curl cleanups from me, and then your \"die on server-side\n> errors\" patch?\n\nYes, that one :)\n"},{"id":"365648","messageId":"20181220154900.GF27361@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20181219233005.GI37614@google.com","subject":"Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-12-20T15:49:00Z","receivedAt":"2018-12-20T15:49:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 19, 2018 at 03:30:05PM -0800, Josh Steadmon wrote:\n\n> > > > This is outside of the Git pack protocol so having a separate parsing\n> > > > mode makes sense to me.\n> > > \n> > > This sounds like it could be a significant refactoring. Should we go\n> > > back to V2 of this series, and then work on the new parsing mode\n> > > separately?\n> > \n> > Which one is v2? :)\n> > \n> > Just the remote-curl cleanups from me, and then your \"die on server-side\n> > errors\" patch?\n> \n> Yes, that one :)\n\nThen yes, that sounds reasonable to me.\n\n-Peff\n"},{"id":"368549","messageId":"xmqq5ztxstkh.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"20181116084725.GA31603@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-05T23:29:18Z","receivedAt":"2019-02-05T23:29:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> After making initial contact with an http server, we have to decide if\n> the server supports smart-http, and if so, which version. Our rules are\n> a bit inconsistent:\n> ...\n>\n> - we now predicate the smart/dumb decision entirely on the presence of\n> the correct content-type\n>\n> - we do a real pkt-line parse before deciding how to proceed (and die\n> if it isn't valid)\n>\n> - use skip_prefix() for comparing service strings, instead of\n> constructing expected output in a strbuf; this avoids dealing with\n> memory cleanup\n>\n> Note that this _is_ tightening what the client will allow. It's all\n> according to the spec, but it's possible that other implementations\n> might violate these. However, violating these particular rules seems\n> like an odd choice for a server to make.\n>\n> [1] Documentation/technical/http-protocol.txt, l. 166-167\n> [2] Documentation/technical/protocol-v2.txt, l. 63-64\n> [3] Documentation/technical/http-protocol.txt, l. 247\n>\n> Helped-by: Josh Steadmon \n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 59 insertions(+), 34 deletions(-)\n\nIt turns out that this has interactions with 01f9ec64 (\"Use\npacket_reader instead of packet_read_line\", 2018-12-29) on the\nms/packet-err-check branch. Can we get this rebased on top of\na more recent 'master'?\n\nThanks.\n"},{"id":"368630","messageId":"20190206191657.GE10231@sigill.intra.peff.net","threadId":"49814","inReplyTo":"xmqq5ztxstkh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T19:16:57Z","receivedAt":"2019-02-06T19:17:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2019 at 03:29:18PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > After making initial contact with an http server, we have to decide if\n> > the server supports smart-http, and if so, which version. Our rules are\n> > a bit inconsistent:\n> > ...\n> >\n> > - we now predicate the smart/dumb decision entirely on the presence of\n> > the correct content-type\n> >\n> > - we do a real pkt-line parse before deciding how to proceed (and die\n> > if it isn't valid)\n> >\n> > - use skip_prefix() for comparing service strings, instead of\n> > constructing expected output in a strbuf; this avoids dealing with\n> > memory cleanup\n> >\n> > Note that this _is_ tightening what the client will allow. It's all\n> > according to the spec, but it's possible that other implementations\n> > might violate these. However, violating these particular rules seems\n> > like an odd choice for a server to make.\n> >\n> > [1] Documentation/technical/http-protocol.txt, l. 166-167\n> > [2] Documentation/technical/protocol-v2.txt, l. 63-64\n> > [3] Documentation/technical/http-protocol.txt, l. 247\n> >\n> > Helped-by: Josh Steadmon \n> > Signed-off-by: Jeff King \n> > ---\n> > remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------\n> > 1 file changed, 59 insertions(+), 34 deletions(-)\n> \n> It turns out that this has interactions with 01f9ec64 (\"Use\n> packet_reader instead of packet_read_line\", 2018-12-29) on the\n> ms/packet-err-check branch. Can we get this rebased on top of\n> a more recent 'master'?\n\nYep. Here it is.\n\nRather than a range-diff, which is quite large due to the code movement,\nI'll include below the interesting hunk of a diff between the two\nendpoints (i.e., what we would have seen applying the packet-err-check\nchanges on top of my code movement, which is more or less what I did to\ngenerate it).\n\nJosh's original 3/3 isn't needed anymore, since ms/packet-err-check\ncovers that case already. However, he did write tests, which\nms/packet-err-check does not have. So I've converted his final patch\ninto just a test addition.\n\n [1/3]: remote-curl: refactor smart-http discovery\n [2/3]: remote-curl: tighten \"version 2\" check for smart-http\n [3/3]: t5551: test server-side ERR packet\n\n remote-curl.c | 100 ++++++++++++++++++--------------\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++\n t/lib-httpd/error-smart-http.sh | 3 +\n t/t5551-http-fetch-smart.sh | 5 ++\n 5 files changed, 70 insertions(+), 43 deletions(-)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\nHere's that hunk:\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex b1309f2bdc..bb7421023b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -333,10 +334,8 @@ static int get_protocol_http_header(enum protocol_version version,\n static void check_smart_http(struct discovery *d, const char *service,\n \t\t\t struct strbuf *type)\n {\n-\tchar *src_buf;\n-\tsize_t src_len;\n-\tchar *line;\n \tconst char *p;\n+\tstruct packet_reader reader;\n \n \t/*\n \t * If we don't see x-$service-advertisement, then it's not smart-http.\n@@ -348,45 +347,42 @@ static void check_smart_http(struct discovery *d, const char *service,\n \t strcmp(p, \"-advertisement\"))\n \t\treturn;\n \n-\t/*\n-\t * \"Peek\" at the first packet by using a separate buf/len pair; some\n-\t * cases below require us leaving the originals intact.\n-\t */\n-\tsrc_buf = d->buf;\n-\tsrc_len = d->len;\n-\tline = packet_read_line_buf(&src_buf, &src_len, NULL);\n-\tif (!line)\n+\tpacket_reader_init(&reader, -1, d->buf, d->len,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n \t\tdie(\"invalid server response; expected service, got flush packet\");\n \n-\tif (skip_prefix(line, \"# service=\", &p) && !strcmp(p, service)) {\n+\tif (skip_prefix(reader.line, \"# service=\", &p) && !strcmp(p, service)) {\n \t\t/*\n \t\t * The header can include additional metadata lines, up\n \t\t * until a packet flush marker. Ignore these now, but\n \t\t * in the future we might start to scan them.\n \t\t */\n-\t\twhile (packet_read_line_buf(&src_buf, &src_len, NULL))\n-\t\t\t;\n+\t\tfor (;;) {\n+\t\t\tpacket_reader_read(&reader);\n+\t\t\tif (reader.pktlen <= 0) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \n \t\t/*\n \t\t * v0 smart http; callers expect us to soak up the\n \t\t * service and header packets\n \t\t */\n-\t\td->buf = src_buf;\n-\t\td->len = src_len;\n+\t\td->buf = reader.src_buffer;\n+\t\td->len = reader.src_len;\n \t\td->proto_git = 1;\n \n-\t} else if (!strcmp(line, \"version 2\")) {\n+\t} else if (!strcmp(reader.line, \"version 2\")) {\n \t\t/*\n \t\t * v2 smart http; do not consume version packet, which will\n \t\t * be handled elsewhere.\n \t\t */\n \t\td->proto_git = 1;\n \n-\t} else if (skip_prefix(line, \"ERR \", &p)) {\n-\t\tdie(_(\"remote error: %s\"), p);\n-\n \t} else {\n-\t\tdie(\"invalid server response; got '%s'\", line);\n+\t\tdie(\"invalid server response; got '%s'\", reader.line);\n \t}\n }\n \n"},{"id":"368631","messageId":"20190206191848.GA10893@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20190206191657.GE10231@sigill.intra.peff.net","subject":"[PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T19:18:48Z","receivedAt":"2019-02-06T19:18:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After making initial contact with an http server, we have to decide if\nthe server supports smart-http, and if so, which version. Our rules are\na bit inconsistent:\n\n 1. For v0, we require that the content-type indicates a smart-http\n response. We also require the response to look vaguely like a\n pkt-line starting with \"#\". If one of those does not match, we fall\n back to dumb-http.\n\n But according to our http protocol spec[1]:\n\n Dumb servers MUST NOT return a return type starting with\n `application/x-git-`.\n\n If we see the expected content-type, we should consider it\n smart-http. At that point we can parse the pkt-line for real, and\n complain if it is not syntactically valid.\n\n 2. For v2, we do not actually check the content-type. Our v2 protocol\n spec says[2]:\n\n When using the http:// or https:// transport a client makes a\n \"smart\" info/refs request as described in `http-protocol.txt`[...]\n\n and the http spec is clear that for a smart-http response[3]:\n\n The Content-Type MUST be `application/x-$servicename-advertisement`.\n\n So it is required according to the spec.\n\nThese inconsistencies were easy to miss because of the way the original\ncode was written as an inline conditional. Let's pull it out into its\nown function for readability, and improve a few things:\n\n - we now predicate the smart/dumb decision entirely on the presence of\n the correct content-type\n\n - we do a real pkt-line parse before deciding how to proceed (and die\n if it isn't valid)\n\n - use skip_prefix() for comparing service strings, instead of\n constructing expected output in a strbuf; this avoids dealing with\n memory cleanup\n\nNote that this _is_ tightening what the client will allow. It's all\naccording to the spec, but it's possible that other implementations\nmight violate these. However, violating these particular rules seems\nlike an odd choice for a server to make.\n\n[1] Documentation/technical/http-protocol.txt, l. 166-167\n[2] Documentation/technical/protocol-v2.txt, l. 63-64\n[3] Documentation/technical/http-protocol.txt, l. 247\n\nHelped-by: Josh Steadmon \nSigned-off-by: Jeff King \n---\n remote-curl.c | 100 ++++++++++++++++++++++++++++----------------------\n 1 file changed, 57 insertions(+), 43 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2e04d53ac8..c78ba83744 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -331,9 +331,63 @@ static int get_protocol_http_header(enum protocol_version version,\n \treturn 0;\n }\n \n+static void check_smart_http(struct discovery *d, const char *service,\n+\t\t\t struct strbuf *type)\n+{\n+\tconst char *p;\n+\tstruct packet_reader reader;\n+\n+\t/*\n+\t * If we don't see x-$service-advertisement, then it's not smart-http.\n+\t * But once we do, we commit to it and assume any other protocol\n+\t * violations are hard errors.\n+\t */\n+\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n+\t !skip_prefix(p, service, &p) ||\n+\t strcmp(p, \"-advertisement\"))\n+\t\treturn;\n+\n+\tpacket_reader_init(&reader, -1, d->buf, d->len,\n+\t\t\t PACKET_READ_CHOMP_NEWLINE |\n+\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n+\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n+\t\tdie(\"invalid server response; expected service, got flush packet\");\n+\n+\tif (skip_prefix(reader.line, \"# service=\", &p) && !strcmp(p, service)) {\n+\t\t/*\n+\t\t * The header can include additional metadata lines, up\n+\t\t * until a packet flush marker. Ignore these now, but\n+\t\t * in the future we might start to scan them.\n+\t\t */\n+\t\tfor (;;) {\n+\t\t\tpacket_reader_read(&reader);\n+\t\t\tif (reader.pktlen <= 0) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/*\n+\t\t * v0 smart http; callers expect us to soak up the\n+\t\t * service and header packets\n+\t\t */\n+\t\td->buf = reader.src_buffer;\n+\t\td->len = reader.src_len;\n+\t\td->proto_git = 1;\n+\n+\t} else if (starts_with(reader.line, \"version 2\")) {\n+\t\t/*\n+\t\t * v2 smart http; do not consume version packet, which will\n+\t\t * be handled elsewhere.\n+\t\t */\n+\t\td->proto_git = 1;\n+\n+\t} else {\n+\t\tdie(\"invalid server response; got '%s'\", reader.line);\n+\t}\n+}\n+\n static struct discovery *discover_refs(const char *service, int for_push)\n {\n-\tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n@@ -405,47 +459,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n-\tif (maybe_smart &&\n-\t (5 <= last->len && last->buf[4] == '#') &&\n-\t !strbuf_cmp(&exp, &type)) {\n-\t\tstruct packet_reader reader;\n-\t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n-\t\t\t\t PACKET_READ_CHOMP_NEWLINE |\n-\t\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n-\n-\t\t/*\n-\t\t * smart HTTP response; validate that the service\n-\t\t * pkt-line matches our request.\n-\t\t */\n-\t\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n-\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n-\n-\t\tstrbuf_reset(&exp);\n-\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n-\t\tif (strcmp(reader.line, exp.buf))\n-\t\t\tdie(\"invalid server response; got '%s'\", reader.line);\n-\t\tstrbuf_release(&exp);\n-\n-\t\t/* The header can include additional metadata lines, up\n-\t\t * until a packet flush marker. Ignore these now, but\n-\t\t * in the future we might start to scan them.\n-\t\t */\n-\t\tfor (;;) {\n-\t\t\tpacket_reader_read(&reader);\n-\t\t\tif (reader.pktlen <= 0) {\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n-\n-\t\tlast->buf = reader.src_buffer;\n-\t\tlast->len = reader.src_len;\n-\n-\t\tlast->proto_git = 1;\n-\t} else if (maybe_smart &&\n-\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n-\t\tlast->proto_git = 1;\n-\t}\n+\tif (maybe_smart)\n+\t\tcheck_smart_http(last, service, &type);\n \n \tif (last->proto_git)\n \t\tlast->refs = parse_git_refs(last, for_push);\n@@ -453,7 +468,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \t\tlast->refs = parse_info_refs(last);\n \n \tstrbuf_release(&refs_url);\n-\tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n-- \n2.20.1.1122.g2972e48916\n\n"},{"id":"368632","messageId":"20190206191858.GB10893@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20190206191657.GE10231@sigill.intra.peff.net","subject":"[PATCH 2/3] remote-curl: tighten \"version 2\" check for smart-http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T19:18:58Z","receivedAt":"2019-02-06T19:19:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In a v2 smart-http conversation, the server should reply to our initial\nrequest with a pkt-line saying \"version 2\". We check that with\nstarts_with(), but really that should be the only thing in that packet.\nA response of \"version 20\" should not match.\n\nLet's tighten this check to use strcmp(). Note that we don't need to\nworry about a trailing newline here, because the ptk-line code will have\nchomped it for us already.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c78ba83744..bb7421023b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -374,7 +374,7 @@ static void check_smart_http(struct discovery *d, const char *service,\n \t\td->len = reader.src_len;\n \t\td->proto_git = 1;\n \n-\t} else if (starts_with(reader.line, \"version 2\")) {\n+\t} else if (!strcmp(reader.line, \"version 2\")) {\n \t\t/*\n \t\t * v2 smart http; do not consume version packet, which will\n \t\t * be handled elsewhere.\n-- \n2.20.1.1122.g2972e48916\n\n"},{"id":"368633","messageId":"20190206191910.GC10893@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20190206191657.GE10231@sigill.intra.peff.net","subject":"[PATCH 3/3] t5551: test server-side ERR packet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T19:19:10Z","receivedAt":"2019-02-06T19:19:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Josh Steadmon \n\nWhen a smart HTTP server sends an error message via pkt-line, we detect\nthe error due to using PACKET_READ_DIE_ON_ERR_PACKET. This case was\nadded by 2d103c31c2 (pack-protocol.txt: accept error packets in any\ncontext, 2018-12-29), but not covered by tests.\n\nSigned-off-by: Josh Steadmon \nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error-smart-http.sh | 3 +++\n t/t5551-http-fetch-smart.sh | 5 +++++\n 4 files changed, 13 insertions(+)\n create mode 100644 t/lib-httpd/error-smart-http.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex e465116ef9..216281eabc 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -131,6 +131,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-sed.sh\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 5d63ed90c5..06a81b54c7 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -119,6 +119,7 @@ Alias /auth/dumb/ www/auth/dumb/\n ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n@@ -127,6 +128,9 @@ ScriptAliasMatch /one_time_sed/(.*) apply-one-time-sed.sh/$1\n \n \tOptions ExecCGI\n \n+\n+\tOptions ExecCGI\n+\n \n Options ExecCGI\n \ndiff --git a/t/lib-httpd/error-smart-http.sh b/t/lib-httpd/error-smart-http.sh\nnew file mode 100644\nindex 0000000000..e65d447fc4\n--- /dev/null\n+++ b/t/lib-httpd/error-smart-http.sh\n@@ -0,0 +1,3 @@\n+echo \"Content-Type: application/x-git-upload-pack-advertisement\"\n+echo\n+printf \"%s\" \"0019ERR server-side error\"\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 8630b0cc39..ba83e567e5 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -429,5 +429,10 @@ test_expect_success 'GIT_TRACE_CURL_NO_DATA prevents data from being traced' '\n \t! grep \"=> Send data\" err\n '\n \n+test_expect_success 'server-side error detected' '\n+\ttest_must_fail git clone $HTTPD_URL/error_smart/repo.git 2>actual &&\n+\tgrep \"server-side error\" actual\n+'\n+\n stop_httpd\n test_done\n-- \n2.20.1.1122.g2972e48916\n"},{"id":"368635","messageId":"20190206192903.GE72177@google.com","threadId":"49814","inReplyTo":"20190206191848.GA10893@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-02-06T19:29:03Z","receivedAt":"2019-02-06T19:29:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2019.02.06 14:18, Jeff King wrote:\n> After making initial contact with an http server, we have to decide if\n> the server supports smart-http, and if so, which version. Our rules are\n> a bit inconsistent:\n> \n> 1. For v0, we require that the content-type indicates a smart-http\n> response. We also require the response to look vaguely like a\n> pkt-line starting with \"#\". If one of those does not match, we fall\n> back to dumb-http.\n> \n> But according to our http protocol spec[1]:\n> \n> Dumb servers MUST NOT return a return type starting with\n> `application/x-git-`.\n> \n> If we see the expected content-type, we should consider it\n> smart-http. At that point we can parse the pkt-line for real, and\n> complain if it is not syntactically valid.\n> \n> 2. For v2, we do not actually check the content-type. Our v2 protocol\n> spec says[2]:\n> \n> When using the http:// or https:// transport a client makes a\n> \"smart\" info/refs request as described in `http-protocol.txt`[...]\n> \n> and the http spec is clear that for a smart-http response[3]:\n> \n> The Content-Type MUST be `application/x-$servicename-advertisement`.\n> \n> So it is required according to the spec.\n> \n> These inconsistencies were easy to miss because of the way the original\n> code was written as an inline conditional. Let's pull it out into its\n> own function for readability, and improve a few things:\n> \n> - we now predicate the smart/dumb decision entirely on the presence of\n> the correct content-type\n> \n> - we do a real pkt-line parse before deciding how to proceed (and die\n> if it isn't valid)\n> \n> - use skip_prefix() for comparing service strings, instead of\n> constructing expected output in a strbuf; this avoids dealing with\n> memory cleanup\n> \n> Note that this _is_ tightening what the client will allow. It's all\n> according to the spec, but it's possible that other implementations\n> might violate these. However, violating these particular rules seems\n> like an odd choice for a server to make.\n> \n> [1] Documentation/technical/http-protocol.txt, l. 166-167\n> [2] Documentation/technical/protocol-v2.txt, l. 63-64\n> [3] Documentation/technical/http-protocol.txt, l. 247\n> \n> Helped-by: Josh Steadmon \n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 100 ++++++++++++++++++++++++++++----------------------\n> 1 file changed, 57 insertions(+), 43 deletions(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index 2e04d53ac8..c78ba83744 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -331,9 +331,63 @@ static int get_protocol_http_header(enum protocol_version version,\n> \treturn 0;\n> }\n> \n> +static void check_smart_http(struct discovery *d, const char *service,\n> +\t\t\t struct strbuf *type)\n> +{\n> +\tconst char *p;\n> +\tstruct packet_reader reader;\n> +\n> +\t/*\n> +\t * If we don't see x-$service-advertisement, then it's not smart-http.\n> +\t * But once we do, we commit to it and assume any other protocol\n> +\t * violations are hard errors.\n> +\t */\n> +\tif (!skip_prefix(type->buf, \"application/x-\", &p) ||\n> +\t !skip_prefix(p, service, &p) ||\n> +\t strcmp(p, \"-advertisement\"))\n> +\t\treturn;\n> +\n> +\tpacket_reader_init(&reader, -1, d->buf, d->len,\n> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> +\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> +\t\tdie(\"invalid server response; expected service, got flush packet\");\n\nThis can also trigger on an EOF or a delim packet, should we clarify the\nerror message?\n\n\n> +\n> +\tif (skip_prefix(reader.line, \"# service=\", &p) && !strcmp(p, service)) {\n> +\t\t/*\n> +\t\t * The header can include additional metadata lines, up\n> +\t\t * until a packet flush marker. Ignore these now, but\n> +\t\t * in the future we might start to scan them.\n> +\t\t */\n> +\t\tfor (;;) {\n> +\t\t\tpacket_reader_read(&reader);\n> +\t\t\tif (reader.pktlen <= 0) {\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n\nCould we make this loop cleaner as:\n\nwhile (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n ;\n\n\n> +\n> +\t\t/*\n> +\t\t * v0 smart http; callers expect us to soak up the\n> +\t\t * service and header packets\n> +\t\t */\n> +\t\td->buf = reader.src_buffer;\n> +\t\td->len = reader.src_len;\n> +\t\td->proto_git = 1;\n> +\n> +\t} else if (starts_with(reader.line, \"version 2\")) {\n> +\t\t/*\n> +\t\t * v2 smart http; do not consume version packet, which will\n> +\t\t * be handled elsewhere.\n> +\t\t */\n> +\t\td->proto_git = 1;\n> +\n> +\t} else {\n> +\t\tdie(\"invalid server response; got '%s'\", reader.line);\n> +\t}\n> +}\n> +\n> static struct discovery *discover_refs(const char *service, int for_push)\n> {\n> -\tstruct strbuf exp = STRBUF_INIT;\n> \tstruct strbuf type = STRBUF_INIT;\n> \tstruct strbuf charset = STRBUF_INIT;\n> \tstruct strbuf buffer = STRBUF_INIT;\n> @@ -405,47 +459,8 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n> \tlast->buf = last->buf_alloc;\n> \n> -\tstrbuf_addf(&exp, \"application/x-%s-advertisement\", service);\n> -\tif (maybe_smart &&\n> -\t (5 <= last->len && last->buf[4] == '#') &&\n> -\t !strbuf_cmp(&exp, &type)) {\n> -\t\tstruct packet_reader reader;\n> -\t\tpacket_reader_init(&reader, -1, last->buf, last->len,\n> -\t\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> -\t\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> -\n> -\t\t/*\n> -\t\t * smart HTTP response; validate that the service\n> -\t\t * pkt-line matches our request.\n> -\t\t */\n> -\t\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> -\t\t\tdie(\"invalid server response; expected service, got flush packet\");\n> -\n> -\t\tstrbuf_reset(&exp);\n> -\t\tstrbuf_addf(&exp, \"# service=%s\", service);\n> -\t\tif (strcmp(reader.line, exp.buf))\n> -\t\t\tdie(\"invalid server response; got '%s'\", reader.line);\n> -\t\tstrbuf_release(&exp);\n> -\n> -\t\t/* The header can include additional metadata lines, up\n> -\t\t * until a packet flush marker. Ignore these now, but\n> -\t\t * in the future we might start to scan them.\n> -\t\t */\n> -\t\tfor (;;) {\n> -\t\t\tpacket_reader_read(&reader);\n> -\t\t\tif (reader.pktlen <= 0) {\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t}\n> -\n> -\t\tlast->buf = reader.src_buffer;\n> -\t\tlast->len = reader.src_len;\n> -\n> -\t\tlast->proto_git = 1;\n> -\t} else if (maybe_smart &&\n> -\t\t last->len > 5 && starts_with(last->buf + 4, \"version 2\")) {\n> -\t\tlast->proto_git = 1;\n> -\t}\n> +\tif (maybe_smart)\n> +\t\tcheck_smart_http(last, service, &type);\n> \n> \tif (last->proto_git)\n> \t\tlast->refs = parse_git_refs(last, for_push);\n> @@ -453,7 +468,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n> \t\tlast->refs = parse_info_refs(last);\n> \n> \tstrbuf_release(&refs_url);\n> -\tstrbuf_release(&exp);\n> \tstrbuf_release(&type);\n> \tstrbuf_release(&charset);\n> \tstrbuf_release(&effective_url);\n> -- \n> 2.20.1.1122.g2972e48916\n> \n"},{"id":"368639","messageId":"xmqqzhr8ps2m.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"20190206192903.GE72177@google.com","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-06T20:42:09Z","receivedAt":"2019-02-06T20:42:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Steadmon writes:\n\n>> +\tpacket_reader_init(&reader, -1, d->buf, d->len,\n>> +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n>> +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n>> +\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n>> +\t\tdie(\"invalid server response; expected service, got flush packet\");\n>\n> This can also trigger on an EOF or a delim packet, should we clarify the\n> error message?\n\nYou mean that it may not be \"flush packet\"? I guess we can remove\n\", got X\" part and that would probably be an improvement.\n\n>> +\n>> +\tif (skip_prefix(reader.line, \"# service=\", &p) && !strcmp(p, service)) {\n>> +\t\t/*\n>> +\t\t * The header can include additional metadata lines, up\n>> +\t\t * until a packet flush marker. Ignore these now, but\n>> +\t\t * in the future we might start to scan them.\n>> +\t\t */\n>> +\t\tfor (;;) {\n>> +\t\t\tpacket_reader_read(&reader);\n>> +\t\t\tif (reader.pktlen <= 0) {\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>\n> Could we make this loop cleaner as:\n>\n> while (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> ;\n\nThe only case as far as I can tell that would make the difference\nbetween the original and your simplified one would be if a packet\nhad a single newline and nothing else in it, in which case pktlen\nwould be zero (since we pass CHOMP_NEWLINE) and the return value\nwould be READ_NORMAL. The original would break, while yours will\nspin one more time.\n\nThanks.\n"},{"id":"368641","messageId":"20190206211414.GA12737@sigill.intra.peff.net","threadId":"49814","inReplyTo":"20190206192903.GE72177@google.com","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-06T21:14:14Z","receivedAt":"2019-02-06T21:14:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 06, 2019 at 11:29:03AM -0800, Josh Steadmon wrote:\n\n> > +\tpacket_reader_init(&reader, -1, d->buf, d->len,\n> > +\t\t\t PACKET_READ_CHOMP_NEWLINE |\n> > +\t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> > +\tif (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> > +\t\tdie(\"invalid server response; expected service, got flush packet\");\n> \n> This can also trigger on an EOF or a delim packet, should we clarify the\n> error message?\n\nMaybe, though I'd prefer to do it as a patch on top; these lines were\nmoved straight from the existing code.\n\n> > +\tif (skip_prefix(reader.line, \"# service=\", &p) && !strcmp(p, service)) {\n> > +\t\t/*\n> > +\t\t * The header can include additional metadata lines, up\n> > +\t\t * until a packet flush marker. Ignore these now, but\n> > +\t\t * in the future we might start to scan them.\n> > +\t\t */\n> > +\t\tfor (;;) {\n> > +\t\t\tpacket_reader_read(&reader);\n> > +\t\t\tif (reader.pktlen <= 0) {\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> \n> Could we make this loop cleaner as:\n> \n> while (packet_reader_read(&reader) != PACKET_READ_NORMAL)\n> ;\n\nLikewise here.\n\n-Peff\n"},{"id":"368649","messageId":"xmqqsgx0pnjs.fsf@gitster-ct.c.googlers.com","threadId":"49814","inReplyTo":"20190206191657.GE10231@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] remote-curl: refactor smart-http discovery","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-06T22:19:51Z","receivedAt":"2019-02-06T22:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yep. Here it is.\n>\n> Rather than a range-diff, which is quite large due to the code movement,\n> I'll include below the interesting hunk of a diff between the two\n> endpoints (i.e., what we would have seen applying the packet-err-check\n> changes on top of my code movement, which is more or less what I did to\n> generate it).\n\nThanks. The hunks shown below are to use packet_reader interface in\ncheck_smart_http(), which is exactly what we want to see in this\nstep, and what I was too lazy to recreate as a part of conflict\nresolution ;-)\n\nThanks, queued.\n"}]}