{"thread":{"id":"53119","subject":"[RFC PATCH] http-backend: write error packet if backend command fails","startedAt":"2020-03-28T02:50:45Z","lastAt":"2020-03-29T05:34:28Z","messageCount":5,"participants":["Denton Liu","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394213","messageId":"b5f8b81498e1d152014acab92fa1b6e9701b3a0e.1585363771.git.liu.denton@gmail.com","threadId":"53119","inReplyTo":null,"subject":"[RFC PATCH] http-backend: write error packet if backend command fails","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-03-28T02:50:32Z","receivedAt":"2020-03-28T02:50:45Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"If one tries to fetch packs with an incorrectly formatted parameter\nwhile using the smart HTTP protocol v2, the client will block forever.\nThis is seen with the following command which does not terminate:\n\n\t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\nMeanwhile, if one uses v1, the command will terminate as expected:\n\n\t$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012\n\tCloning into 'git'...\n\tfatal: the remote end hung up unexpectedly\n\nThis happens because when upload-pack detects invalid parameters, it\nwill die(). When http-backend calls finish_command() and detects a\nnon-zero exit code, it will simply call exit(1). Since there is no way\nin a CGI script to force a connection to terminate, the client keeps\nblocking on the read(), expecting more output.\n\nWrite an error packet if the backend command fails to start or finishes\nwith an error so that the client can terminate the connection.\n\nNote that if the included test case is run without the remainder of the\npatch, it will run forever and fail to terminate.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nNotes:\n    So this is the first time I've touched the networking code of Git. There\n    are a few problems with this patch that I'd appreciate some help\n    addressing.\n    \n    First of all, is this even the right approach? I tried to find some\n    other way to force a CGI script to terminate a connection but I don't\n    think that it's possible so I had to get the client to do it instead.\n    \n    Next, with this patch applied it seems like t5539-fetch-http-shallow\n    fails on 'no shallow lines after receiving ACK ready'. I've been trying\n    to dig into why this happens but I can't quite figure it out.\n    \n    Finally, I'd like the test case I added in t5702 to be guarded by some\n    timeout in case we regress instead of blocking the test suite forever\n    but I'm not sure if that's even available functionality. It seems like\n    we don't use the `timeout` program yet so I'm not sure if it's a good\n    idea to introduce it here.\n    \n    Thanks,\n    \n    Denton\n\n http-backend.c         | 10 +++++++---\n t/t5702-protocol-v2.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex ec3144b444..7ea3a688fd 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -488,10 +488,11 @@ static void run_service(const char **argv, int buffer_input)\n \tcld.git_cmd = 1;\n \tcld.clean_on_exit = 1;\n \tcld.wait_after_clean = 1;\n-\tif (start_command(&cld))\n+\tif (start_command(&cld)) {\n+\t\tpacket_write_fmt(1, \"ERR %s failed to start\\n\", argv[0]);\n \t\texit(1);\n+\t}\n \n-\tclose(1);\n \tif (gzipped_request)\n \t\tinflate_request(argv[0], cld.in, buffer_input, req_len);\n \telse if (buffer_input)\n@@ -501,8 +502,11 @@ static void run_service(const char **argv, int buffer_input)\n \telse\n \t\tclose(0);\n \n-\tif (finish_command(&cld))\n+\tif (finish_command(&cld)) {\n+\t\tpacket_write_fmt(1, \"ERR %s failed\\n\", argv[0]);\n \t\texit(1);\n+\t}\n+\tclose(1);\n }\n \n static int show_text_ref(const char *name, const struct object_id *oid,\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 5039e66dc4..69a920a34b 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -586,6 +586,23 @@ test_expect_success 'clone with http:// using protocol v2' '\n \t! grep \"Send header: Transfer-Encoding: chunked\" log\n '\n \n+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '\n+\ttest_when_finished \"rm -f log\" &&\n+\n+\ttest_must_fail env GIT_TRACE_PACKET=\"$(pwd)/log\" GIT_TRACE_CURL=\"$(pwd)/log\" \\\n+\t\tgit -c protocol.version=2 \\\n+\t\tclone --shallow-since=20151012 \"$HTTPD_URL/smart/http_parent\" http_child_invalid &&\n+\n+\t# Client requested to use protocol v2\n+\tgrep \"Git-Protocol: version=2\" log &&\n+\t# Server responded using protocol v2\n+\tgrep \"git< version 2\" log &&\n+\t# Verify that we sucessfully errored out\n+\tgrep -F \"ERR upload-pack failed\" log &&\n+\t# Verify that the chunked encoding sending codepath is NOT exercised\n+\t! grep \"Send header: Transfer-Encoding: chunked\" log\n+'\n+\n test_expect_success 'clone big repository with http:// using protocol v2' '\n \ttest_when_finished \"rm -f log\" &&\n \n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"394233","messageId":"20200328145741.GA1209174@coredump.intra.peff.net","threadId":"53119","inReplyTo":"b5f8b81498e1d152014acab92fa1b6e9701b3a0e.1585363771.git.liu.denton@gmail.com","subject":"Re: [RFC PATCH] http-backend: write error packet if backend command fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T14:57:41Z","receivedAt":"2020-03-28T14:57:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 27, 2020 at 10:50:32PM -0400, Denton Liu wrote:\n\n> If one tries to fetch packs with an incorrectly formatted parameter\n> while using the smart HTTP protocol v2, the client will block forever.\n> This is seen with the following command which does not terminate:\n> \n> \t$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012\n> \tCloning into 'git'...\n\nThat can't possibly be fixed by a change to http-backend.c, because\ngithub.com does not use git-http-backend. :)\n\nWe have a custom proxy layer that terminates the http connection, and\nspeaks to \"upload-pack --stateless-rpc\" on the backend. There may be a\nbug in that proxy layer, but it's usually pretty robust to upload-pack\nhanging up the connection.\n\nBut since it works for v1 (which also dies!), and since you were able to\nreproduce the problem locally, I suspect it may be an issue in\nupload-pack itself.\n\n> This happens because when upload-pack detects invalid parameters, it\n> will die(). When http-backend calls finish_command() and detects a\n> non-zero exit code, it will simply call exit(1). Since there is no way\n> in a CGI script to force a connection to terminate, the client keeps\n> blocking on the read(), expecting more output.\n\nWhen the CGI exits, that should close the descriptor it holds. If the\nwebserver hands off the socket to the CGI, that would be sufficient. If\nit doesn't, then it should notice the CGI exiting and close the socket\nitself.\n\n> Write an error packet if the backend command fails to start or finishes\n> with an error so that the client can terminate the connection.\n\nThis is probably not a good idea; we don't know what state the protocol\nwas in when the child died.\n\n-Peff\n"},{"id":"394234","messageId":"20200328151300.GA1215566@coredump.intra.peff.net","threadId":"53119","inReplyTo":"20200328145741.GA1209174@coredump.intra.peff.net","subject":"Re: [RFC PATCH] http-backend: write error packet if backend command fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T15:13:00Z","receivedAt":"2020-03-28T15:13:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:\n\n> But since it works for v1 (which also dies!), and since you were able to\n> reproduce the problem locally, I suspect it may be an issue in\n> upload-pack itself.\n\nActually, I think the problem is on the client side.\n\nIf I run your test without the http-backend change, then nothing is left\nrunning on the server side, but on the client we have two processes:\ngit-clone and the git-remote-https helper. And they're deadlocked trying\nto read from each other.\n\nI think the issue is that git-remote-https in v2 mode calls into\nstateless_connect(), and just pumps http request/response pairs from\ngit-clone to the server. But if a request generates an empty response,\nthen clone has no idea that anything was received at all. It continues\nwaiting, but remote-https has looped to expect another request from it.\n\nYour patch fixes _this case_ because it causes the server to send a\nnon-empty response. But the client can't rely on that. Besides that not\nbeing a documented server behavior, in the worst case the connection\ncould be severed mid-stream. So I think remote-https needs to complain\nabout an empty response. This isn't a problem in v1 because it would\nactually try to look at that empty response; in v2 it's just proxying\nbytes around.\n\n-Peff\n"},{"id":"394236","messageId":"20200328154936.GA1217052@coredump.intra.peff.net","threadId":"53119","inReplyTo":"20200328151300.GA1215566@coredump.intra.peff.net","subject":"Re: [RFC PATCH] http-backend: write error packet if backend command fails","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-28T15:49:36Z","receivedAt":"2020-03-28T15:49:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 28, 2020 at 11:13:00AM -0400, Jeff King wrote:\n\n> On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:\n> \n> > But since it works for v1 (which also dies!), and since you were able to\n> > reproduce the problem locally, I suspect it may be an issue in\n> > upload-pack itself.\n> \n> Actually, I think the problem is on the client side.\n> \n> If I run your test without the http-backend change, then nothing is left\n> running on the server side, but on the client we have two processes:\n> git-clone and the git-remote-https helper. And they're deadlocked trying\n> to read from each other.\n> \n> I think the issue is that git-remote-https in v2 mode calls into\n> stateless_connect(), and just pumps http request/response pairs from\n> git-clone to the server. But if a request generates an empty response,\n> then clone has no idea that anything was received at all. It continues\n> waiting, but remote-https has looped to expect another request from it.\n> \n> Your patch fixes _this case_ because it causes the server to send a\n> non-empty response. But the client can't rely on that. Besides that not\n> being a documented server behavior, in the worst case the connection\n> could be severed mid-stream. So I think remote-https needs to complain\n> about an empty response. This isn't a problem in v1 because it would\n> actually try to look at that empty response; in v2 it's just proxying\n> bytes around.\n\nUgh, it's actually much worse than this. We dealt with the\nempty-response case in 296b847c0d (remote-curl: don't hang when a server\ndies before any output, 2016-11-18). The issue here is that we got a\n_partial_ response, and clone is waiting for the rest of it.\n\nThe server said \"0011shallow-info\\n\" before it bailed. So from the\nperspective of git-clone, it's now waiting to read through a flush\npacket. But remote-https has nothing left to send. The root of the issue\nis that it has no way to signal to clone that it has already sent\neverything the server gave it. In non-helper code, clone would see the\nEOF. And in v1 https, I think there's some extra framing between\nremote-https and \"fetch-pack --stateless-rpc\" so that EOF effectively\ngets passed along. But v2's stateless_connect() strategy is\nfundamentally missing this signal, and there are probably other spots in\nthe protocol that could cause similar deadlocks.\n\nI wonder if remote-https's stateless_connect() could complain if there's\nno flush packet in the output it writes back. That would certainly fix\nthis case, but I'd worry there are rpc messages that don't end in a\nflush. And it would be susceptible to data cut-offs that happened to\nlook like a flush packet.\n\nI think the robust solution is to introduce an extra level of pktline\nframing into the remote-helper stateless-connect protocol.\n\n-Peff\n"},{"id":"394261","messageId":"20200329053421.GA685115@generichostname","threadId":"53119","inReplyTo":"20200328154936.GA1217052@coredump.intra.peff.net","subject":"Re: [RFC PATCH] http-backend: write error packet if backend command fails","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-03-29T05:34:21Z","receivedAt":"2020-03-29T05:34:28Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Peff,\n\nOn Sat, Mar 28, 2020 at 11:49:36AM -0400, Jeff King wrote:\n> On Sat, Mar 28, 2020 at 11:13:00AM -0400, Jeff King wrote:\n> \n> > On Sat, Mar 28, 2020 at 10:57:42AM -0400, Jeff King wrote:\n> > \n> > > But since it works for v1 (which also dies!), and since you were able to\n> > > reproduce the problem locally, I suspect it may be an issue in\n> > > upload-pack itself.\n> > \n> > Actually, I think the problem is on the client side.\n> > \n> > If I run your test without the http-backend change, then nothing is left\n> > running on the server side, but on the client we have two processes:\n> > git-clone and the git-remote-https helper. And they're deadlocked trying\n> > to read from each other.\n> > \n> > I think the issue is that git-remote-https in v2 mode calls into\n> > stateless_connect(), and just pumps http request/response pairs from\n> > git-clone to the server. But if a request generates an empty response,\n> > then clone has no idea that anything was received at all. It continues\n> > waiting, but remote-https has looped to expect another request from it.\n> > \n> > Your patch fixes _this case_ because it causes the server to send a\n> > non-empty response. But the client can't rely on that. Besides that not\n> > being a documented server behavior, in the worst case the connection\n> > could be severed mid-stream. So I think remote-https needs to complain\n> > about an empty response. This isn't a problem in v1 because it would\n> > actually try to look at that empty response; in v2 it's just proxying\n> > bytes around.\n> \n> Ugh, it's actually much worse than this. We dealt with the\n> empty-response case in 296b847c0d (remote-curl: don't hang when a server\n> dies before any output, 2016-11-18). The issue here is that we got a\n> _partial_ response, and clone is waiting for the rest of it.\n> \n> The server said \"0011shallow-info\\n\" before it bailed. So from the\n> perspective of git-clone, it's now waiting to read through a flush\n> packet. But remote-https has nothing left to send. The root of the issue\n> is that it has no way to signal to clone that it has already sent\n> everything the server gave it. In non-helper code, clone would see the\n> EOF. And in v1 https, I think there's some extra framing between\n> remote-https and \"fetch-pack --stateless-rpc\" so that EOF effectively\n> gets passed along. But v2's stateless_connect() strategy is\n> fundamentally missing this signal, and there are probably other spots in\n> the protocol that could cause similar deadlocks.\n> \n> I wonder if remote-https's stateless_connect() could complain if there's\n> no flush packet in the output it writes back. That would certainly fix\n> this case, but I'd worry there are rpc messages that don't end in a\n> flush. And it would be susceptible to data cut-offs that happened to\n> look like a flush packet.\n> \n> I think the robust solution is to introduce an extra level of pktline\n> framing into the remote-helper stateless-connect protocol.\n\nThanks for doing some more investigation and correcting my faulty\nanalysis. I'll use this information to try and create a new patch.\n\n-Denton\n\n> -Peff\n"}]}