[PATCH 6/6] remote-curl: ensure last packet is a flush

[Denton Liu <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