[PATCH 3/3] remote-curl: sanity check ref advertisement from server

[Jeff King <peff@peff.net>]

If the smart HTTP response from the server is truncated for
any reason, we will get an incomplete ref advertisement. If
we then feed this incomplete list to "fetch-pack", one of a
few things may happen:

  1. If the truncation is in a packet header, fetch-pack
     will notice the bogus line and complain.

  2. If the truncation is inside a packet, fetch-pack will
     keep waiting for us to send the rest of the packet,
     which we never will.

  3. If the truncation is at a packet boundary, fetch-pack
     will keep waiting for us to send the next packet, which
     we never will.

As a result, fetch-pack hangs, waiting for input. However,
remote-curl believes it has sent all of the advertisement,
and therefore waits for fetch-pack to speak. The two
processes end up in a deadlock.

This fortunately doesn't happen in the normal fetching
workflow, because git-fetch first uses the "list" command,
which feeds the refs to get_remote_heads, which does notice
the error. However, you can trigger it by sending a direct
"fetch" to the remote-curl helper.

We can make this more robust by verifying that the packet
stream we got from the server does indeed parse correctly
and ends with a flush packet, which means that what
fetch-pack receives will at least be syntactically correct.

The normal non-stateless-rpc case does not have to deal with
this problem; it detects a truncation by getting EOF on the
file descriptor before it has read all data. So it is
tempting to think that we can solve this by closing the
descriptor after relaying the server's advertisement.
Unfortunately, in the stateless rpc case, we need to keep
the descriptor to fetch-pack open in order to pass more data
to it.

We could solve that by using two descriptors, but our
run-command interface does not support that (and modifying
it to create more pipes would make life hard for the Windows
port of git).

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index 73134f5..c7647c7 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -101,6 +101,15 @@ static int read_packets_until_flush(char **buf, size_t *len)
 	}
 }
 
+static int verify_ref_advertisement(char *buf, size_t len)
+{
+	/*
+	 * Our function parameters are copies, so we do not
+	 * have to care that read_packets will increment our pointers.
+	 */
+	return read_packets_until_flush(&buf, &len);
+}
+
 static struct discovery* discover_refs(const char *service)
 {
 	struct strbuf exp = STRBUF_INIT;
@@ -174,6 +183,9 @@ static struct discovery* discover_refs(const char *service)
 			die("smart-http metadata lines are invalid at %s",
 			    refs_url);
 
+		if (verify_ref_advertisement(last->buf, last->len) < 0)
+			die("ref advertisement is invalid at %s", refs_url);
+
 		last->proto_git = 1;
 	}
 
-- 
1.8.1.2.11.g1a2f572