git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] remote-curl: die on server-side errors

From
Jeff King <peff@peff.net>
Date
Nov 14, 2018, 00:49 UTC
Message-ID
<20181114004915.GA5059@sigill.intra.peff.net>
In-Reply-To
<20181113222540.GE126896@google.com>
On Tue, Nov 13, 2018 at 02:25:40PM -0800, Josh Steadmon wrote:
Show 25 quoted lines
> > The magic "4" here and in the existing "version 2" check is because we
> > are expecting pkt-lines. The original conditional always calls
> > packed_read_line_buf() and will complain if we didn't actually get a
> > pkt-line.
> > 
> > Should we confirm that we got a real packet-line? Or at least that those
> > first four are even plausible hex chars?
> > 
> > I admit that it's pretty unlikely that the server is going to fool us
> > here. It would need something like "foo ERRORS ARE FUN!". And even then
> > we'd report an error (whereas the correct behavior is falling back to
> > dumb http, but we know that won't work anyway because that's not a valid
> > ref advertisement). So I doubt this is really a bug per se, but it might
> > make it easier to understand what's going on if we actually parsed the
> > packet.
> 
> Unfortunately we can't just directly parse the data in last->buf,
> because other parts of the code are expecting to see the raw pkt-line
> data there. I tried adding a duplicate pointer and length variable for
> this data and parsing that through packet_read_line_buf(), but even
> without using the results this apparently has side-effects that break
> all of t5550 (and probably other tests as well). It also fails if I
> completely duplicate last->buf into a new char* and call
> packet_readline_buf() on that, so there's clearly some interaction in
> the pkt-line guts that I'm not properly accounting for.

Yes, the packet_read_line_buf() interface will both advance the buf pointer and decrement the length. So if we want to "peek", we have to do so with a copy (there's a peek function if you use the packet_reader interface, but that might be overkill here).

You can rewrite it like this, which is a pretty faithful conversion and passes the tests (but see below).

diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..66c57c9d62 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -330,9 +330,78 @@ static int get_protocol_http_header(enum protocol_version version,
 	return 0;
 }
 
+static void check_smart_http(struct discovery *d, const char *service,
+			     struct strbuf *type)
+{
+	char *src_buf;
+	size_t src_len;
+	char pkt[LARGE_PACKET_MAX];
+	int pkt_len;
+	enum packet_read_status r;
+
+	/*
+	 * We speculatively try to read a packet, which means we must preserve
+	 * the original buf/len pair in some cases.
+	 */
+	src_buf = d->buf;
+	src_len = d->len;
+	r = packet_read_with_status(-1, &src_buf, &src_len,
+				    pkt, sizeof(pkt), &pkt_len,
+				    PACKET_READ_GENTLE |
+				    PACKET_READ_CHOMP_NEWLINE);
+
+	/*
+	 * This could probably just be handled as "not smart" like all the
+	 * other pkt-line error cases, but traditionally we've complained
+	 * about it (technically we used to do so only when we got the right
+	 * content-type, but it probably doesn't matter).
+	 */
+	if (r == PACKET_READ_FLUSH)
+		die("invalid server response; expected service, got flush packet");
+	if (r != PACKET_READ_NORMAL)
+		return; /* not smart */
+
+	if (pkt[0] == '#') {
+		/* v0 smart http */
+		struct strbuf exp = STRBUF_INIT;
+
+		strbuf_addf(&exp, "application/x-%s-advertisement", service);
+		if (strbuf_cmp(&exp, type)) {
+			strbuf_release(&exp);
+			return;
+		}
+
+		strbuf_reset(&exp);
+		strbuf_addf(&exp, "# service=%s", service);
+		if (strcmp(pkt, exp.buf))
+			die("invalid server response; got '%s'", pkt);
+
+		strbuf_release(&exp);
+
+		/*
+		 * The header can include additional metadata lines, up
+		 * until a packet flush marker.  Ignore these now, but
+		 * in the future we might start to scan them.
+		 */
+		while (packet_read_line_buf(&src_buf, &src_len, NULL))
+			;
+
+		d->buf = src_buf;
+		d->len = src_len;
+		d->proto_git = 1;
+
+	} else if (starts_with(pkt, "version 2")) {
+		/*
+		 * v2 smart http; do not consume version packet, which will
+		 * be handled elsewhere. Should we be checking the content-type
+		 * in this code-path, too?
+		 */
+		d->proto_git = 1;
+	}
+}
+
 static struct discovery *discover_refs(const char *service, int for_push)
 {
-	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf charset = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
@@ -405,38 +474,8 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	strbuf_addf(&exp, "application/x-%s-advertisement", service);
-	if (maybe_smart &&
-	    (5 <= last->len && last->buf[4] == '#') &&
-	    !strbuf_cmp(&exp, &type)) {
-		char *line;
-
-		/*
-		 * smart HTTP response; validate that the service
-		 * pkt-line matches our request.
-		 */
-		line = packet_read_line_buf(&last->buf, &last->len, NULL);
-		if (!line)
-			die("invalid server response; expected service, got flush packet");
-
-		strbuf_reset(&exp);
-		strbuf_addf(&exp, "# service=%s", service);
-		if (strcmp(line, exp.buf))
-			die("invalid server response; got '%s'", line);
-		strbuf_release(&exp);
-
-		/* The header can include additional metadata lines, up
-		 * until a packet flush marker.  Ignore these now, but
-		 * in the future we might start to scan them.
-		 */
-		while (packet_read_line_buf(&last->buf, &last->len, NULL))
-			;
-
-		last->proto_git = 1;
-	} else if (maybe_smart &&
-		   last->len > 5 && starts_with(last->buf + 4, "version 2")) {
-		last->proto_git = 1;
-	}
+	if (maybe_smart)
+		check_smart_http(last, service, &type);
 
 	if (last->proto_git)
 		last->refs = parse_git_refs(last, for_push);
@@ -444,7 +483,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 		last->refs = parse_info_refs(last);
 
 	strbuf_release(&refs_url);
-	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&charset);
 	strbuf_release(&effective_url);

So the few tricky things are:

  - the ordering with respect to checking the packet and the
    content-type is a little different here. Should v2 protocol be
    expecting that content-type, too? If so, then I think it would make
    sense to check that first, before even considering if there's a
    packet to read (and the current code is overly-permissive in that
    case).

  - there is no way to speculatively read a packet and not die() if the
    buffer doesn't have pkt-lines in it. So we'd additionally need the
    patch below (which annoyingly has to touch a bunch of switch
    statements to keep the compiler happy). If we _can_ check the
    content-type first, then all of that could go away (i.e., once we
    see the right content-type, we're confident enough in expecting
    packets to die() in the existing packet code-paths).

diff --git a/connect.c b/connect.c
index 24281b6082..1caafc3ce7 100644
--- a/connect.c
+++ b/connect.c
@@ -125,6 +125,8 @@ enum protocol_version discover_version(struct packet_reader *reader)
 	switch (packet_reader_peek(reader)) {
 	case PACKET_READ_EOF:
 		die_initial_contact(0);
+	case PACKET_READ_FORMAT_ERROR:
+		BUG("format error from non-gentle packet read");
 	case PACKET_READ_FLUSH:
 	case PACKET_READ_DELIM:
 		version = protocol_v0;
@@ -304,6 +306,8 @@ struct ref **get_remote_heads(struct packet_reader *reader,
 		switch (packet_reader_read(reader)) {
 		case PACKET_READ_EOF:
 			die_initial_contact(1);
+		case PACKET_READ_FORMAT_ERROR:
+			BUG("format error from non-gentle packet read");
 		case PACKET_READ_NORMAL:
 			len = reader->pktlen;
 			if (len > 4 && skip_prefix(reader->line, "ERR ", &arg))
diff --git a/pkt-line.c b/pkt-line.c
index 04d10bbd03..2daacc6188 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -293,7 +293,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
 
 	/* And complain if we didn't get enough bytes to satisfy the read. */
 	if (ret != size) {
-		if (options & PACKET_READ_GENTLE_ON_EOF)
+		if ((options & PACKET_READ_GENTLE_ON_EOF) ||
+		    (options & PACKET_READ_GENTLE))
 			return -1;
 
 		die(_("the remote end hung up unexpectedly"));
@@ -324,6 +325,10 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 	len = packet_length(linelen);
 
 	if (len < 0) {
+		if (options & PACKET_READ_GENTLE) {
+			*pktlen = -1;
+			return PACKET_READ_FORMAT_ERROR;
+		}
 		die(_("protocol error: bad line length character: %.4s"), linelen);
 	} else if (!len) {
 		packet_trace("0000", 4, 0);
@@ -334,12 +339,21 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		*pktlen = 0;
 		return PACKET_READ_DELIM;
 	} else if (len < 4) {
+		if (options & PACKET_READ_GENTLE) {
+			*pktlen = -1;
+			return PACKET_READ_FORMAT_ERROR;
+		}
 		die(_("protocol error: bad line length %d"), len);
 	}
 
 	len -= 4;
-	if ((unsigned)len >= size)
+	if ((unsigned)len >= size) {
+		if (options & PACKET_READ_GENTLE) {
+			*pktlen = -1;
+			return PACKET_READ_FORMAT_ERROR;
+		}
 		die(_("protocol error: bad line length %d"), len);
+	}
 
 	if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0) {
 		*pktlen = -1;
diff --git a/pkt-line.h b/pkt-line.h
index 5b28d43472..7580f83cdc 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -44,7 +44,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  * If src_buffer (or *src_buffer) is NULL, then data is read from the
  * descriptor "fd".
  *
- * If options does not contain PACKET_READ_GENTLE_ON_EOF, we will die under any
+ * If options does not contain PACKET_READ_GENTLE, we will die under any
  * of the following conditions:
  *
  *   1. Read error from descriptor.
@@ -56,6 +56,8 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  *   4. Truncated output from the remote (e.g., we expected a packet but got
  *      EOF, or we got a partial packet followed by EOF).
  *
+ * If options does contain PACKET_READ_GENTLE, we'll instead return -1.
+ *
  * If options does contain PACKET_READ_GENTLE_ON_EOF, we will not die on
  * condition 4 (truncated input), but instead return -1. However, we will still
  * die for the other 3 conditions.
@@ -65,6 +67,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
  */
 #define PACKET_READ_GENTLE_ON_EOF (1u<<0)
 #define PACKET_READ_CHOMP_NEWLINE (1u<<1)
+#define PACKET_READ_GENTLE        (1u<<2)
 int packet_read(int fd, char **src_buffer, size_t *src_len, char
 		*buffer, unsigned size, int options);
 
@@ -79,6 +82,7 @@ enum packet_read_status {
 	PACKET_READ_NORMAL,
 	PACKET_READ_FLUSH,
 	PACKET_READ_DELIM,
+	PACKET_READ_FORMAT_ERROR,
 };
 enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 						size_t *src_len, char *buffer,
diff --git a/remote-curl.c b/remote-curl.c
index 762a55a75f..66c57c9d62 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1214,6 +1252,8 @@ static size_t proxy_in(char *buffer, size_t eltsize,
 		switch (packet_reader_read(&p->reader)) {
 		case PACKET_READ_EOF:
 			die("unexpected EOF when reading from parent process");
+		case PACKET_READ_FORMAT_ERROR:
+			BUG("format error from non-gentle packet read");
 		case PACKET_READ_NORMAL:
 			packet_buf_write_len(&p->request_buffer, p->reader.line,
 					     p->reader.pktlen);
diff --git a/serve.c b/serve.c
index bda085f09c..e88936262b 100644
--- a/serve.c
+++ b/serve.c
@@ -181,6 +181,8 @@ static int process_request(void)
 		switch (packet_reader_peek(&reader)) {
 		case PACKET_READ_EOF:
 			BUG("Should have already died when seeing EOF");
+		case PACKET_READ_FORMAT_ERROR:
+			BUG("format error from non-gentle packet read");
 		case PACKET_READ_NORMAL:
 			/* collect request; a sequence of keys and values */
 			if (is_command(reader.line, &command) ||
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index 282d536384..1522176a2f 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -37,6 +37,8 @@ static void unpack(void)
 		switch (reader.status) {
 		case PACKET_READ_EOF:
 			break;
+		case PACKET_READ_FORMAT_ERROR:
+			BUG("format error from non-gentle packet read");
 		case PACKET_READ_NORMAL:
 			printf("%s\n", reader.line);
 			break;
@@ -64,6 +66,8 @@ static void unpack_sideband(void)
 		switch (reader.status) {
 		case PACKET_READ_EOF:
 			break;
+		case PACKET_READ_FORMAT_ERROR:
+			BUG("format error from non-gentle packet read");
 		case PACKET_READ_NORMAL:
 			band = reader.line[0] & 0xff;
 			if (band < 1 || band > 2)
Previous: Josh SteadmonNext: Jeff King
Message 8 of 41 in “remote-curl: die on server-side errors”
  1. remote-curl: die on server-side errorssteadmon@google.com, Nov 12, 2018
  2. Stefan BellerNov 12, 2018
  3. Junio C HamanoNov 13, 2018
  4. Junio C HamanoNov 13, 2018
  5. Josh SteadmonNov 13, 2018
  6. Jeff KingNov 13, 2018
  7. Josh SteadmonNov 13, 2018
  8. Jeff KingNov 14, 2018
  9. Jeff KingNov 14, 2018
  10. Josh SteadmonNov 15, 2018
  11. 0/3 remote-curl smart-http discovery cleanupJeff King, Nov 16, 2018
  12. 1/3 remote-curl: refactor smart-http discoveryJeff King, Nov 16, 2018
  13. Josh SteadmonNov 16, 2018
  14. Junio C HamanoFeb 5, 2019
  15. Jeff KingFeb 6, 2019
  16. 1/3 remote-curl: refactor smart-http discoveryJeff King, Feb 6, 2019
  17. Josh SteadmonFeb 6, 2019
  18. Junio C HamanoFeb 6, 2019
  19. Jeff KingFeb 6, 2019
  20. 2/3 remote-curl: tighten "version 2" check for smart-httpJeff King, Feb 6, 2019
  21. 3/3 t5551: test server-side ERR packetJeff King, Feb 6, 2019
  22. Junio C HamanoFeb 6, 2019
  23. 2/3 remote-curl: tighten "version 2" check for smart-httpJeff King, Nov 16, 2018
  24. Josh SteadmonNov 16, 2018
  25. 3/3 remote-curl: die on server-side errorsJeff King, Nov 16, 2018
  26. Josh SteadmonNov 16, 2018
  27. 0/4 Unify pkt-line error handling and refactor smart-httpJosh Steadmon, Dec 12, 2018
  28. 1/4 pack-protocol.txt: accept error packets in any contextJosh Steadmon, Dec 12, 2018
  29. Jeff KingDec 12, 2018
  30. Masaya SuzukiDec 13, 2018
  31. Jeff KingDec 13, 2018
  32. Josh SteadmonDec 13, 2018
  33. Jeff KingDec 17, 2018
  34. Josh SteadmonDec 19, 2018
  35. Jeff KingDec 20, 2018
  36. 2/4 remote-curl: refactor smart-http discoveryJosh Steadmon, Dec 12, 2018
  37. 3/4 remote-curl: tighten "version 2" check for smart-httpJosh Steadmon, Dec 12, 2018
  38. 4/4 lib-httpd, t5551: check server-side HTTP errorsJosh Steadmon, Dec 12, 2018
  39. Junio C HamanoDec 12, 2018
  40. Junio C HamanoNov 13, 2018
  41. Josh SteadmonNov 13, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.