[PATCH 6/6] remote-curl: ensure last packet is a flush
- From
Denton Liu <liu.denton@gmail.com>
- Date
- May 13, 2020, 18:04 UTC
- Message-ID
- <7a689da2bb820f70d9e668d656b088af2297d456.1589393036.git.liu.denton@gmail.com>
- In-Reply-To
- <cover.1589393036.git.liu.denton@gmail.com>
Currently, remote-curl acts as a proxy and blindly forwards packets between an HTTP server and fetch-pack. In the case of a stateless RPC connection where the connection is terminated before the transaction is complete, remote-curl will blindly forward the packets before waiting on more input from fetch-pack. Meanwhile, fetch-pack will read the transaction and continue reading, expecting more input to continue the transaction. This results in a deadlock between the two processes.
This can be seen in the following command which does not terminate:
$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012 Cloning into 'git'...
whereas the v1 version does terminate as expected:
$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012 Cloning into 'git'... fatal: the remote end hung up unexpectedly
Instead of blindly forwarding packets, raise a flag when a flush packet is encountered. Ensure that the last packet sent is a flush packet otherwise error out, breaking the deadlock.
This is not a complete solution to the problem, however. It is possible that a flush packet could be sent in the middle of a message and the connection could die immediately after. Then, remote-curl would not error out and fetch-pack would still be in the middle of a transaction and they would enter deadlock. A complete solution would involve reframing the stateless-connect protocol, possibly by introducing another control packet ("0002"?) as a stateless request separator packet which is always sent at the end of post_rpc().
Although this is not a complete solution, it is better than nothing and it resolves the reported issue for now.
Reported-by: Force Charlie <charlieio@outlook.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Denton Liu <liu.denton@gmail.com> ---
Notes:
I wish there were some way to insert a timeout on the test case so that
we don't block forever in case we regress.remote-curl.c | 9 +++++++++ t/t5702-protocol-v2.sh | 17 +++++++++++++++++ 2 files changed, 26 insertions(+)
diff --git a/remote-curl.c b/remote-curl.c index 8b740354e5..aab17851be 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -684,6 +684,7 @@ struct rpc_in_data { struct active_request_slot *slot; struct strbuf len_buf; int remaining; + int last_flush; }; /* @@ -707,6 +708,8 @@ static size_t rpc_in(char *ptr, size_t eltsize, data->rpc->any_written = 1; while (unwritten) { + data->last_flush = 0; + if (!data->remaining) { int digits_remaining = 4 - data->len_buf.len; if (digits_remaining > unwritten) @@ -720,6 +723,8 @@ static size_t rpc_in(char *ptr, size_t eltsize, if (data->remaining < 0) { die(_("remote-curl: bad line length character: %.4s"), data->len_buf.buf); } else if (data->remaining <= 1) { + if (!data->remaining) + data->last_flush = 1; data->remaining = 0; } else if (data->remaining < 4) { die(_("remote-curl: bad line length %d"), data->remaining); @@ -960,6 +965,7 @@ static int post_rpc(struct rpc_state *rpc, int flush_received) rpc_in_data.slot = slot; strbuf_init(&rpc_in_data.len_buf, 4); rpc_in_data.remaining = 0; + rpc_in_data.last_flush = 0; curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); @@ -979,6 +985,9 @@ static int post_rpc(struct rpc_state *rpc, int flush_received) if (rpc_in_data.remaining) err = error(_("%d bytes are still expected"), rpc_in_data.remaining); + if (!rpc_in_data.last_flush) + err = error(_("last packet was not a flush")); + curl_slist_free_all(headers); free(gzip_body); return err; diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 5039e66dc4..4570d0746c 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -586,6 +586,23 @@ test_expect_success 'clone with http:// using protocol v2' ' ! grep "Send header: Transfer-Encoding: chunked" log ' +test_expect_success 'clone with http:// using protocol v2 and invalid parameters' ' + test_when_finished "rm -f log" && + + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \ + git -c protocol.version=2 \ + clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid 2>err && + + # Client requested to use protocol v2 + grep "Git-Protocol: version=2" log && + # Server responded using protocol v2 + grep "git< version 2" log && + # Verify that we errored out in the expected way + test_i18ngrep "last packet was not a flush" err && + # Verify that the chunked encoding sending codepath is NOT exercised + ! grep "Send header: Transfer-Encoding: chunked" log +' + test_expect_success 'clone big repository with http:// using protocol v2' ' test_when_finished "rm -f log" &&
-- 2.26.2.706.g87896c9627