[PATCH] remote-curl: don't hang when a server dies before any output

In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come.  Instead, we should die
immediately.

One case where this happens is when attempting to fetch a dangling
object by SHA.

Still to do: it would be good to give a better error message
than "fatal: The remote end hung up unexpectedly".

Signed-off-by: David Turner <redacted>
---

Note: if you run t5551 before applying the code patch, the second new
test will hang forever.

FWIW, I also saw this kind of hang at Twitter from time to time, but I
was never able to reliably reproduce it there, and thus never able to
fix it.  I suspect that a bad load balancer might have been killing
connections just after the headers, but this is pure speculation.

I am sorry that this patch does not provide a more useful error
message.  For us, it is important to fix the hangs (so that we can
retry on another server, for instance), but less important to be
user-friendly (since we were only seeing these with automated
processes).  I hope that someone who actually understands the http
code, and has some time, could help improve this aspect of the code.

If not, then at least we won't have inexplicable hangs.

 remote-curl.c               |  8 ++++++++
 t/t5551-http-fetch-smart.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index f14c41f..ee44236 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -400,6 +400,7 @@ struct rpc_state {
 	size_t pos;
 	int in;
 	int out;
+	int any_written;
 	struct strbuf result;
 	unsigned gzip_request : 1;
 	unsigned initial_buffer : 1;

@@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_state *rpc = buffer_;
+	if (size)
+		rpc->any_written = 1;
 	write_or_die(rpc->in, ptr, size);
 	return size;
 }

@@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
+
+	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request) {
 		credential_fill(&http_auth);

@@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
 	if (err != HTTP_OK)
 		err = -1;
 
+	if (!rpc->any_written)
+		err = -1;
+
 	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 1ec5b27..43665ab 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh

@@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
 	test_line_count = 2 posts
 '
 
+test_expect_success 'test allowreachablesha1inwant' '
+	test_when_finished "rm -rf test_reachable.git" &&
+	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+	git init --bare test_reachable.git &&
+	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+	git -C test_reachable.git fetch origin "$master_sha"
+'
+
+test_expect_success 'test allowreachablesha1inwant with unreachable' '
+	test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+	#create unreachable sha
+	echo content >file2 &&
+	git add file2 &&
+	git commit -m two &&
+	git push public HEAD:refs/heads/doomed &&
+	git push public :refs/heads/doomed &&
+
+	server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+	git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+	git init --bare test_reachable.git &&
+	git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+	test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
 test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	(
 		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&

-- 
2.8.0.rc4.22.g8ae061a

On Wed, Nov 09, 2016 at 05:18:30PM -0500, David Turner wrote:

In the event that a HTTP server closes the connection after giving a
200 but before giving any packets, we don't want to hang forever
waiting for a response that will never come.  Instead, we should die
immediately.

I agree we don't want to hang forever, but this leaves open the
question: what is hanging?

My guess is that fetch-pack is waiting for more data from the server,
and remote-curl is waiting for fetch-pack to tell us what to send for
the next request. Neither will make forward progress because they are
effectively waiting on each other.

Which means this is likely a special case of malformed input from the
server. A server which likewise sends a partial response could end up in
the same deadlock, I would think (e.g., a half-finished pktline, or a
pktline but no trailing flush).

That doesn't make it wrong to fix this specific case (especially if it's
a common one), but I wonder if we could do better.

The root of the issue is that only fetch-pack understands the protocol,
and remote-curl is blindly proxying the data. But only remote-curl knows
that the HTTP request has ended, and it doesn't relay that information
to fetch-pack. So I can think of two solutions:

  1. Some way of remote-curl communicating the EOF to fetch-pack. It
     can't just close the descriptor, since we need to pass more data
     over it for the followup requests. You'd need something
     out-of-band, or to frame the HTTP data inside another layer of
     pktlines, both of which are kind of gross.

  2. Have remote-curl understand enough of the protocol that it can
     abort rather than hang.

     I think that's effectively the approach of your patch, but for one
     specific case. But could we, for example, make sure that everything
     we proxy is a complete set of pktlines and ends with a flush? And
     if not, then we hang up on fetch-pack.

     I _think_ that would work, because even the pack is always encased
     in pktlines for smart-http.

Extra blank line here?

I wondered if there were any cases where it was normal for the server to
return zero bytes. Possibly the ref advertisement is one, but this is
_just_ handling post_rpc(), so that's OK. And I think by definition
every response has to at least return a flush packet, or we would make
no forward progress (i.e., the exact case you are dealing with here).

-Peff

On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:

  2. Have remote-curl understand enough of the protocol that it can
     abort rather than hang.

     I think that's effectively the approach of your patch, but for one
     specific case. But could we, for example, make sure that everything
     we proxy is a complete set of pktlines and ends with a flush? And
     if not, then we hang up on fetch-pack.

     I _think_ that would work, because even the pack is always encased
     in pktlines for smart-http.

So something like this. It turned out to be a lot uglier than I had
hoped because we get fed the data from curl in odd-sized chunks, so we
need a state machine.

But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.

---

diff --git a/remote-curl.c b/remote-curl.c
index f14c41f4c..605357d77 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -403,6 +403,18 @@ struct rpc_state {
 	struct strbuf result;
 	unsigned gzip_request : 1;
 	unsigned initial_buffer : 1;
+
+	enum {
+		RPC_PKTLINE_ERROR, /* bogus hex chars in length */
+		RPC_PKTLINE_INITIAL, /* no packets received yet */
+		RPC_PKTLINE_1, /* got one hex char */
+		RPC_PKTLINE_2, /* got two hex chars */
+		RPC_PKTLINE_3, /* got three hex chars */
+		RPC_PKTLINE_DATA, /* reading data; pktline_len holds remaining */
+		RPC_PKTLINE_END_OF_PACKET, /* last packet completed */
+		RPC_PKTLINE_FLUSH, /* last packet was flush */
+	} pktline_state;
+	size_t pktline_len;
 };
 
 static size_t rpc_out(void *ptr, size_t eltsize,

@@ -451,11 +463,77 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+static void update_pktline_state(struct rpc_state *rpc,
+				 const char *buf, size_t len)
+{
+#define READ_ONE_HEX(shift) do { \
+	int val = hexval(buf[0]); \
+	if (val < 0) { \
+		warning("error on %d", *buf); \
+		rpc->pktline_state = RPC_PKTLINE_ERROR; \
+		return; \
+	} \
+	rpc->pktline_len |= val << shift; \
+	buf++; \
+	len--; \
+} while(0)
+
+	while (len > 0) {
+		switch (rpc->pktline_state) {
+		case RPC_PKTLINE_ERROR:
+			/* previous error; there is no recovery */
+			return;
+
+		/* We can start a new pktline at any of these states */
+		case RPC_PKTLINE_INITIAL:
+		case RPC_PKTLINE_FLUSH:
+		case RPC_PKTLINE_END_OF_PACKET:
+			rpc->pktline_len = 0;
+			READ_ONE_HEX(12);
+			rpc->pktline_state = RPC_PKTLINE_1;
+			break;
+
+		case RPC_PKTLINE_1:
+			READ_ONE_HEX(8);
+			rpc->pktline_state = RPC_PKTLINE_2;
+			break;
+
+		case RPC_PKTLINE_2:
+			READ_ONE_HEX(4);
+			rpc->pktline_state = RPC_PKTLINE_3;
+			break;
+
+		case RPC_PKTLINE_3:
+			READ_ONE_HEX(0);
+			if (rpc->pktline_len) {
+				rpc->pktline_state = RPC_PKTLINE_DATA;
+				rpc->pktline_len -= 4;
+			} else
+				rpc->pktline_state = RPC_PKTLINE_FLUSH;
+			break;
+
+		case RPC_PKTLINE_DATA:
+			if (len < rpc->pktline_len) {
+				rpc->pktline_len -= len;
+				len = 0;
+			} else {
+				buf += rpc->pktline_len;
+				len -= rpc->pktline_len;
+				rpc->pktline_len = 0;
+				rpc->pktline_state = RPC_PKTLINE_END_OF_PACKET;
+			}
+			break;
+		}
+	}
+#undef READ_ONE_HEX
+}
+
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_state *rpc = buffer_;
+	update_pktline_state(rpc, ptr, size);
 	write_or_die(rpc->in, ptr, size);
 	return size;
 }

@@ -659,6 +737,8 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
+	rpc->pktline_state = RPC_PKTLINE_INITIAL;
+
 	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request) {
 		credential_fill(&http_auth);

@@ -667,6 +747,11 @@ static int post_rpc(struct rpc_state *rpc)
 	if (err != HTTP_OK)
 		err = -1;
 
+	if (rpc->pktline_state != RPC_PKTLINE_FLUSH) {
+		error("invalid or truncated response from http server");
+		err = -1;
+	}
+
 	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;

-----Original Message-----
From: Jeff King [mailto:peff@peff.net]
Sent: Monday, November 14, 2016 2:41 PM
To: David Turner
Cc: git@vger.kernel.org; spearce@spearce.org
Subject: Re: [PATCH] remote-curl: don't hang when a server dies before any
output

On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:

quoted

  2. Have remote-curl understand enough of the protocol that it can
     abort rather than hang.

     I think that's effectively the approach of your patch, but for one
     specific case. But could we, for example, make sure that everything
     we proxy is a complete set of pktlines and ends with a flush? And
     if not, then we hang up on fetch-pack.

     I _think_ that would work, because even the pack is always encased
     in pktlines for smart-http.

So something like this. It turned out to be a lot uglier than I had hoped
because we get fed the data from curl in odd-sized chunks, so we need a
state machine.

But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the same
way.

Overall, this looks good to me.  The state machine is pretty clean. I think I would have used a tiny buffer for the length field, and then I would have regretted it.  Your way looks nicer than my unwritten patch would have looked.

+#define READ_ONE_HEX(shift) do { \
+	int val = hexval(buf[0]); \
+	if (val < 0) { \
+		warning("error on %d", *buf); \
+		rpc->pktline_state = RPC_PKTLINE_ERROR; \
+		return; \
+	} \
+	rpc->pktline_len |= val << shift; \

Nit: parenthesize shift here, since it is a parameter to a macro.

On Mon, Nov 14, 2016 at 11:25:30PM +0000, David Turner wrote:

quoted

But it does seem to work. At least it doesn't seem to break anything
in the test suite, and it fixes the new tests you added. I'd worry
that there's some obscure case where the response isn't packetized
in the same way.

Overall, this looks good to me.  The state machine is pretty clean. I
think I would have used a tiny buffer for the length field, and then I
would have regretted it.  Your way looks nicer than my unwritten patch
would have looked.

Heh, I started it that way but you end up dealing with the same states
(they're just implicit in your "how big is my temp buffer" field).

quoted

+#define READ_ONE_HEX(shift) do { \
+	int val = hexval(buf[0]); \
+	if (val < 0) { \
+		warning("error on %d", *buf); \
+		rpc->pktline_state = RPC_PKTLINE_ERROR; \
+		return; \
+	} \
+	rpc->pktline_len |= val << shift; \

Nit: parenthesize shift here, since it is a parameter to a macro.

Yeah, I'm often a bit slack on these one-off inside-a-function macros.
But it does not hurt to to be careful.

I'll make that change and then try to wrap this up with a commit
message. I plan to steal your tests, if that's OK.

-Peff

On Mon, Nov 14, 2016 at 02:40:49PM -0500, Jeff King wrote:

On Mon, Nov 14, 2016 at 01:24:31PM -0500, Jeff King wrote:

quoted

  2. Have remote-curl understand enough of the protocol that it can
     abort rather than hang.

     I think that's effectively the approach of your patch, but for one
     specific case. But could we, for example, make sure that everything
     we proxy is a complete set of pktlines and ends with a flush? And
     if not, then we hang up on fetch-pack.

     I _think_ that would work, because even the pack is always encased
     in pktlines for smart-http.

So something like this. It turned out to be a lot uglier than I had
hoped because we get fed the data from curl in odd-sized chunks, so we
need a state machine.

But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.

Actually, I take it back. I think it works for a single round of ref
negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
fail t5551.

I think I've probably made a mis-assumption on exactly when in the HTTP
protocol we will see a flush packet (and perhaps that is a sign that
this protocol-snooping approach is not a good one).

I don't have time to dig more on this tonight, and I'll be traveling for
the rest of the week. So if anybody is interested, please feel free to
dig into it.

-Peff

On Mon, Nov 14, 2016 at 07:44:26PM -0500, Jeff King wrote:

quoted

But it does seem to work. At least it doesn't seem to break anything in
the test suite, and it fixes the new tests you added. I'd worry that
there's some obscure case where the response isn't packetized in the
same way.

Actually, I take it back. I think it works for a single round of ref
negotiation, but not for multiple. Enabling GIT_TEST_LONG=1 causes it to
fail t5551.

I think I've probably made a mis-assumption on exactly when in the HTTP
protocol we will see a flush packet (and perhaps that is a sign that
this protocol-snooping approach is not a good one).

Yep, that is it. The server may end with an ACK or a NAK, not a flush
packet. So going this route really does mean teaching remote-curl a lot
more about the protocol, which is pretty nasty. I think trying to
somehow signal the end-of-response to fetch-pack would be a more
maintainable approach.

-Peff

-----Original Message-----
From: Jeff King [mailto:peff@peff.net]

...

I'll make that change and then try to wrap this up with a commit message.
I plan to steal your tests, if that's OK.

Please do!