[PATCH 0/3] make smart-http more robust against bogus server input

For the most part, smart-http just passes data to fetch-pack and
send-pack, which take care of the heavy lifting. However, I did find a
few corner cases around truncated data from the server, one of which can
actually cause a deadlock.

I found these because I was trying to figure out what was going on with
some hung git processes which were in a deadlock like the one described
in patch 3. But having experimented and read the code, I don't think
that it is triggerable from a normal clone, but rather only when you
poke git-remote-curl in the right way. So it may or may not be my
culprit, but these patches do make remote-curl more robust, which is a
good thing.

  [1/3]: pkt-line: teach packet_get_line a no-op mode
  [2/3]: remote-curl: verify smart-http metadata lines
  [3/3]: remote-curl: sanity check ref advertisement from server

-Peff

You can use packet_get_line to parse a single packet out of
a stream and into a buffer. However, if you just want to
throw away a set of packets from the stream, there's no need
to even bother copying the bytes. This patch treats a NULL
output buffer as a hint that the caller does not even want
to see the output.

We have to tweak the packet_trace call, too, since it showed
the trace from the copied buffer, which now might not exist.
The new code is actually more correct, though, as it shows
just what we parsed, not any cruft that may have been in the
output buffer before (it never mattered, though, because all
callers gave us a fresh buffer).

Signed-off-by: Jeff King <redacted>
---
 pkt-line.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..7f28701 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -234,9 +234,10 @@ int packet_get_line(struct strbuf *out,
 	*src_len -= 4;
 	len -= 4;
 
-	strbuf_add(out, *src_buf, len);
+	if (out)
+		strbuf_add(out, *src_buf, len);
+	packet_trace(*src_buf, len, 0);
 	*src_buf += len;
 	*src_len -= len;
-	packet_trace(out->buf, out->len, 0);
 	return len;
 }

-- 
1.8.1.2.11.g1a2f572

A smart http ref advertisement starts with a packet
containing the service header, followed by an arbitrary
number of packets containing other metadata headers,
followed by a flush packet.

We don't currently recognize any other metadata headers, so
we just parse through any extra packets, throwing away their
contents. However, we don't do so very carefully, and just
stop at the first error or flush packet.

Let's flag any errors we see here, which might be a sign of
truncated or corrupted output. Since the rest of the data
should be the ref advertisement, and since we pass that
along to our helper programs (like fetch-pack), they will
probably notice the error, as whatever cruft is in the
buffer will not parse. However, it's nice to note problems
as early as possible, which can help in debugging the root
cause.

Signed-off-by: Jeff King <redacted>
---
 remote-curl.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..73134f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -90,6 +90,17 @@ static void free_discovery(struct discovery *d)
 	}
 }
 
+static int read_packets_until_flush(char **buf, size_t *len)
+{
+	while (1) {
+		int r = packet_get_line(NULL, buf, len);
+		if (r < 0)
+			return -1;
+		if (r == 0)
+			return 0;
+	}
+}
+
 static struct discovery* discover_refs(const char *service)
 {
 	struct strbuf exp = STRBUF_INIT;

@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)
 
 		/* 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.
+		 * in the future we might start to scan them. However, we do
+		 * still check to make sure we are getting valid packet lines,
+		 * ending with a flush.
 		 */
-		strbuf_reset(&buffer);
-		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
-			strbuf_reset(&buffer);
+		if (read_packets_until_flush(&last->buf, &last->len) < 0)
+			die("smart-http metadata lines are invalid at %s",
+			    refs_url);
 
 		last->proto_git = 1;
 	}

-- 
1.8.1.2.11.g1a2f572

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 <redacted>
---
 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

Jeff King wrote:

For what it's worth,
Reviewed-by: Jonathan Nieder <redacted>

The above code has a structure of

	prepare to return(buf, len);
	trace(buf, len);

	discard used part of buf;
	return;

which is nice and readable.
Jonathan

Jeff King wrote:

[...]

[...]

-		strbuf_reset(&buffer);
-		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
-			strbuf_reset(&buffer);
+		if (read_packets_until_flush(&last->buf, &last->len) < 0)

Style nit: this made me wonder "What would it mean if
read_packets_until_flush() > 0?"  Since the convention for this
function is "0 for success", I would personally find

		if (read_packets_until_flush(...))
			handle error;

easier to read.

+			die("smart-http metadata lines are invalid at %s",
+			    refs_url);

Especially given that other clients would be likely to run into
trouble in the same situation, as long as this cooks in "next" for a
suitable amount of time to catch bad servers, it looks like a good
idea.

Reviewed-by: Jonathan Nieder <redacted>

Thanks.

Jeff King wrote:

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.

Mostly harmless since the operator could hit ^C, but still unpleasant.

[...]

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.

Ah.  Would a test for this make sense?

[...]

[...]

Won't this error out with

	protocol error: bad line length character: ERR

instead of the current more helpful behavior for ERR lines?

Same stylistic comment about "what would it mean for the return value
to be positive?" as in patch 2/3.

Aside from those two details, the idea looks sane, though.  Good
catch, and thanks for a pleasant read.

Good night,
Jonathan

On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:

Jeff King wrote:

quoted

--- a/remote-curl.c
+++ b/remote-curl.c

[...]

quoted

@@ -155,11 +166,13 @@ static struct discovery* discover_refs(const char *service)

[...]

quoted

-		strbuf_reset(&buffer);
-		while (packet_get_line(&buffer, &last->buf, &last->len) > 0)
-			strbuf_reset(&buffer);
+		if (read_packets_until_flush(&last->buf, &last->len) < 0)

Style nit: this made me wonder "What would it mean if
read_packets_until_flush() > 0?"  Since the convention for this
function is "0 for success", I would personally find

		if (read_packets_until_flush(...))
			handle error;

easier to read.

My intent was that it followed the error convention of "negative is
error, 0 is success, and positive is not used, but reserved for
future use". And I tend to think the "< 0" makes it obvious that we are
interested in error. But I don't feel that strongly, so if people would
rather see it the other way, I can live with it.

quoted

+			die("smart-http metadata lines are invalid at %s",
+			    refs_url);

Especially given that other clients would be likely to run into
trouble in the same situation, as long as this cooks in "next" for a
suitable amount of time to catch bad servers, it looks like a good
idea.

Yeah, I have a slight concern that this series would break something in
another implementation, so I would like to see this cook in "next" for a
while (and would be slated for master probably not in this release, but
in the next one). But I think this change is pretty straightforward. If
an implementation is producing bogus packet lines and expecting us not
to complain, it really needs to be fixed.

-Peff

On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:

Jeff King wrote:

quoted

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.

Mostly harmless since the operator could hit ^C, but still unpleasant.

Fetching is not always interactive.  The deadlock I ran into (and again,
I am not sure if this fixes it or not, but it is _a_ deadlock) was on a
server farm doing a large number of "fetch && checkout && deploy"
operations. Only some of them hung, but it took a while to figure out
what was going on.

[...]

quoted

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.

Ah.  Would a test for this make sense?

A test would be great, if you can devise a way to reliably produce
truncated git output (but still valid http output). In the real-world
problem I had, I believe the truncation was caused by an intermediate
reverse proxy that hit a timeout. I simulated truncation by using netcat
to replay munged http headers and git output.

I suspect the simplest portable thing would be a static file of
truncated git output, served by apache, which would need custom
configuration to serve it with the correct content-type header. It
seemed like a lot of test infrastructure to check for a very specific
thing, so I abandoned trying to make a test.

quoted

+		if (verify_ref_advertisement(last->buf, last->len) < 0)
+			die("ref advertisement is invalid at %s", refs_url);

Won't this error out with

	protocol error: bad line length character: ERR

instead of the current more helpful behavior for ERR lines?

I don't think so. Don't ERR lines appear inside their own packets? We
are just verifying that our packets are syntactically correct here, and
my reading of get_remote_heads is that the ERR appears inside the
packetized data.

The one thing we do also check, though, is that we end with a flush
packet. So depending on what servers produce, it may mean we trigger
this complaint instead of passing the ERR along to fetch-pack.

Rather than doing this fake syntactic verification, I wonder if we
should simply call get_remote_heads, which does a more thorough check
(and is what we _would_ call in the list case, and what fetch-pack will
call once we pass data to it). It's slightly less efficient, in that it
starts a new thread and actually builds the linked list of refs. But it
probably isn't that big a deal (and normal operation does a "list" first
which does that _anyway_).

Same stylistic comment about "what would it mean for the return value
to be positive?" as in patch 2/3.

Same response. :)

-Peff

Jeff King wrote:

On Sun, Feb 17, 2013 at 02:49:39AM -0800, Jonathan Nieder wrote:

quoted

Jeff King wrote:

quoted

quoted

--- a/remote-curl.c

[...]

quoted

quoted

+		if (read_packets_until_flush(&last->buf, &last->len) < 0)

Style nit: this made me wonder "What would it mean if
read_packets_until_flush() > 0?"

[...]

My intent was that it followed the error convention of "negative is
error, 0 is success, and positive is not used, but reserved for
future use".

From a maintainability perspective, that kind of contract would be

dangerous, since some *other* caller could arrive and use the function
without a "< 0" without knowing it is doing anything wrong.  When new
return values appear, the function should be renamed to help the patch
author and reviewers remember to check all callers.

That is, from the point of view of maintainability, there is no
distinction between "if (read_packets_until_... < 0)" and
"if (read_packets_until_...)" and either form is fine.

My comment was just to say the "< 0" forced me to pause a moment and
check out the implementation.  This is basically a stylistic thing and
if you prefer to keep the "< 0", that's fine with me.

                                                                     If
an implementation is producing bogus packet lines and expecting us not
to complain, it really needs to be fixed.

Agreed completely.  Thanks again for the patch.

Jonathan

Jeff King wrote:

On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:

quoted

Jeff King wrote:

quoted

quoted

+		if (verify_ref_advertisement(last->buf, last->len) < 0)
+			die("ref advertisement is invalid at %s", refs_url);

Won't this error out with

	protocol error: bad line length character: ERR

instead of the current more helpful behavior for ERR lines?

I don't think so. Don't ERR lines appear inside their own packets?

Yes, I misread get_remote_heads for some reason.  Thanks for checking.

[...]

The one thing we do also check, though, is that we end with a flush
packet. So depending on what servers produce, it may mean we trigger
this complaint instead of passing the ERR along to fetch-pack.

Rather than doing this fake syntactic verification, I wonder if we
should simply call get_remote_heads, which does a more thorough check

I'm not sure whether servers are expected to send a flush after an
ERR packet.  The only codepath I know of in git itself that sends
such packets is git-daemon, which does not flush after the error (but
is not used in the stateless-rpc case).  http-backend uses HTTP error
codes for its errors.

If I am reading get_remote_heads correctly, calling it with the
following tweak should work ok.  The extra thread is just to feed a
string into a fd-based interface and could be avoided for "list", too,
if it costs too much.

diff --git i/connect.c w/connect.c
index 49e56ba3..55ee7de7 100644
--- i/connect.c
+++ w/connect.c

@@ -68,7 +68,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 {
 	int got_at_least_one_head = 0;
 
-	*list = NULL;
+	if (list)
+		*list = NULL;
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];

@@ -92,6 +93,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			die("protocol error: expected sha/ref, got '%s'", buffer);
 		name = buffer + 41;
 
+		if (!list)
+			continue;
+
 		name_len = strlen(name);
 		if (len != name_len + 41) {
 			free(server_capabilities);

On Sun, Feb 17, 2013 at 04:54:43PM -0800, Jonathan Nieder wrote:

quoted

My intent was that it followed the error convention of "negative is
error, 0 is success, and positive is not used, but reserved for
future use".

From a maintainability perspective, that kind of contract would be
dangerous, since some *other* caller could arrive and use the function
without a "< 0" without knowing it is doing anything wrong.  When new
return values appear, the function should be renamed to help the patch
author and reviewers remember to check all callers.

True. That's why I always write "< 0". :)

That is, from the point of view of maintainability, there is no
distinction between "if (read_packets_until_... < 0)" and
"if (read_packets_until_...)" and either form is fine.

My comment was just to say the "< 0" forced me to pause a moment and
check out the implementation.  This is basically a stylistic thing and
if you prefer to keep the "< 0", that's fine with me.

Interesting. To me, "foo() < 0" just reads idiomatically as "error-check
the foo call".

Anyway, I've redone the patch series to just re-use get_remote_heads,
which is more robust. So this function has gone away in the new version.

-Peff

On Sun, Feb 17, 2013 at 05:41:13PM -0800, Jonathan Nieder wrote:

quoted

I don't think so. Don't ERR lines appear inside their own packets?

Yes, I misread get_remote_heads for some reason.  Thanks for checking.

Thanks for bringing it up. I had not even thought about ERR at all. So
it was luck rather than skill that I was right. :)

I'm not sure whether servers are expected to send a flush after an
ERR packet.  The only codepath I know of in git itself that sends
such packets is git-daemon, which does not flush after the error (but
is not used in the stateless-rpc case).  http-backend uses HTTP error
codes for its errors.

I just checked, and GitHub also does not send flush packets after ERR.
Which makes sense; ERR is supposed to end the conversation. I can change
GitHub, of course, but who knows what other implementations exist (e.g.,
I do not know off-hand whether gitolite has custom ERR responses). So it
seems pretty clear that just checking for a flush packet is not the
right thing, and we need to actually parse the packet contents (at least
to some degree).

If I am reading get_remote_heads correctly, calling it with the
following tweak should work ok.  The extra thread is just to feed a
string into a fd-based interface and could be avoided for "list", too,
if it costs too much.

Yeah, your patch does work, though we miss out on some of the refname
checks.  I think what I'd rather do is just teach get_remote_heads to
read from a buffer (to avoid the extra thread and pipe), and then just
run (and cache) the ref parsing unconditionally once we've read from the
server.  It shouldn't make a difference in the normal case, as we would
usually do a "list" anyway (and by caching, "list" can just feed out the
cached copy).

While looking into this, I noticed a bunch of other possible cleanups.
Patches to follow:

  [01/10]: pkt-line: move a misplaced comment
  [02/10]: pkt-line: drop safe_write function
  [03/10]: pkt-line: clean up "gentle" reading function
  [04/10]: pkt-line: change error message for oversized packet
  [05/10]: pkt-line: rename s/packet_read_line/packet_read/

These are all just cleanups I noticed while looking at pkt-line. Any of
them can be dropped, though there would be some textual conflicts for
the later patches.

  [06/10]: pkt-line: share buffer/descriptor reading implementation
  [07/10]: teach get_remote_heads to read from a memory buffer
  [08/10]: remote-curl: pass buffer straight to get_remote_heads

These all build on each other to get rid of the extra thread/pipe, which
I think is worth doing even without the rest of the series.

  [09/10]: remote-curl: move ref-parsing code up in file
  [10/10]: remote-curl: always parse incoming refs

And these ones actually fix the problem I noticed.

-Peff

The comment describing the packet writing interface was
originally written above packet_write, but migrated to be
above safe_write in f3a3214, probably because it is meant to
generally describe the packet writing interface and not a
single function. Let's move it into the header file, where
users of the interface are more likely to see it.

Signed-off-by: Jeff King <redacted>
---
I just left the comment intact as I moved it. It kind of implies to me
that you hand a big buffer to these functions and they would packetize
it for you, which is not true. I don't know if anybody else sees that;
it might be worth tweaking the text.

 pkt-line.c | 15 ---------------
 pkt-line.h | 14 +++++++++++++-
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index eaba15f..5138f47 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -46,21 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
-/*
- * Write a packetized stream, where each line is preceded by
- * its length (including the header) as a 4-byte hex number.
- * A length of 'zero' means end of stream (and a length of 1-3
- * would be an error).
- *
- * This is all pretty stupid, but we use this packetized line
- * format to make a streaming format possible without ever
- * over-running the read buffers. That way we'll never read
- * into what might be the pack data (which should go to another
- * process entirely).
- *
- * The writing side could use stdio, but since the reading
- * side can't, we stay with pure read/write interfaces.
- */
 ssize_t safe_write(int fd, const void *buf, ssize_t n)
 {
 	ssize_t nn = n;

diff --git a/pkt-line.h b/pkt-line.h
index 8cfeb0c..7a67e9c 100644
--- a/pkt-line.h
+++ b/pkt-line.h

@@ -5,7 +5,19 @@
 #include "strbuf.h"
 
 /*
- * Silly packetized line writing interface
+ * Write a packetized stream, where each line is preceded by
+ * its length (including the header) as a 4-byte hex number.
+ * A length of 'zero' means end of stream (and a length of 1-3
+ * would be an error).
+ *
+ * This is all pretty stupid, but we use this packetized line
+ * format to make a streaming format possible without ever
+ * over-running the read buffers. That way we'll never read
+ * into what might be the pack data (which should go to another
+ * process entirely).
+ *
+ * The writing side could use stdio, but since the reading
+ * side can't, we stay with pure read/write interfaces.
  */
 void packet_flush(int fd);
 void packet_write(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));

-- 
1.8.1.20.g7078b03

This is just write_or_die by another name.

Signed-off-by: Jeff King <redacted>
---
Actually, they are not quite the same. write_or_die will exit(0) when it
sees EPIPE. Which makes me a little nervous.

 builtin/receive-pack.c |  2 +-
 builtin/send-pack.c    |  2 +-
 fetch-pack.c           |  2 +-
 http-backend.c         |  8 ++++----
 pkt-line.c             | 21 ++-------------------
 remote-curl.c          |  4 ++--
 send-pack.c            |  2 +-
 sideband.c             |  9 +++++----
 upload-pack.c          |  3 ++-
 9 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e8878de..48cd5dc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -908,7 +908,7 @@ static void report(struct command *commands, const char *unpack_status)
 	if (use_sideband)
 		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
 	else
-		safe_write(1, buf.buf, buf.len);
+		write_or_die(1, buf.buf, buf.len);
 	strbuf_release(&buf);
 }
 

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 57a46b2..8778519 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c

@@ -79,7 +79,7 @@ static void print_helper_status(struct ref *ref)
 		}
 		strbuf_addch(&buf, '\n');
 
-		safe_write(1, buf.buf, buf.len);
+		write_or_die(1, buf.buf, buf.len);
 	}
 	strbuf_release(&buf);
 }

diff --git a/fetch-pack.c b/fetch-pack.c
index 6d8926a..b90dadf 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -245,7 +245,7 @@ static void send_request(struct fetch_pack_args *args,
 		send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);
 		packet_flush(fd);
 	} else
-		safe_write(fd, buf->buf, buf->len);
+		write_or_die(fd, buf->buf, buf->len);
 }
 
 static void insert_one_alternate_ref(const struct ref *ref, void *unused)

diff --git a/http-backend.c b/http-backend.c
index f50e77f..8144f3a 100644
--- a/http-backend.c
+++ b/http-backend.c

@@ -70,7 +70,7 @@ static void format_write(int fd, const char *fmt, ...)
 	if (n >= sizeof(buffer))
 		die("protocol error: impossibly long line");
 
-	safe_write(fd, buffer, n);
+	write_or_die(fd, buffer, n);
 }
 
 static void http_status(unsigned code, const char *msg)

@@ -111,7 +111,7 @@ static void end_headers(void)
 
 static void end_headers(void)
 {
-	safe_write(1, "\r\n", 2);
+	write_or_die(1, "\r\n", 2);
 }
 
 __attribute__((format (printf, 1, 2)))

@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 	hdr_int(content_length, buf->len);
 	hdr_str(content_type, type);
 	end_headers();
-	safe_write(1, buf->buf, buf->len);
+	write_or_die(1, buf->buf, buf->len);
 }
 
 static void send_local_file(const char *the_type, const char *name)

@@ -185,7 +185,7 @@ static void send_local_file(const char *the_type, const char *name)
 			die_errno("Cannot read '%s'", p);
 		if (!n)
 			break;
-		safe_write(1, buf, n);
+		write_or_die(1, buf, n);
 	}
 	close(fd);
 	free(buf);

diff --git a/pkt-line.c b/pkt-line.c
index 5138f47..699c2dd 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -46,23 +46,6 @@ static void packet_trace(const char *buf, unsigned int len, int write)
 	strbuf_release(&out);
 }
 
-ssize_t safe_write(int fd, const void *buf, ssize_t n)
-{
-	ssize_t nn = n;
-	while (n) {
-		int ret = xwrite(fd, buf, n);
-		if (ret > 0) {
-			buf = (char *) buf + ret;
-			n -= ret;
-			continue;
-		}
-		if (!ret)
-			die("write error (disk full?)");
-		die_errno("write error");
-	}
-	return nn;
-}
-
 /*
  * If we buffered things up above (we don't, but we should),
  * we'd flush it here

@@ -70,7 +53,7 @@ void packet_flush(int fd)
 void packet_flush(int fd)
 {
 	packet_trace("0000", 4, 1);
-	safe_write(fd, "0000", 4);
+	write_or_die(fd, "0000", 4);
 }
 
 void packet_buf_flush(struct strbuf *buf)

@@ -106,7 +89,7 @@ void packet_write(int fd, const char *fmt, ...)
 	va_start(args, fmt);
 	n = format_packet(fmt, args);
 	va_end(args);
-	safe_write(fd, buffer, n);
+	write_or_die(fd, buffer, n);
 }
 
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...)

diff --git a/remote-curl.c b/remote-curl.c
index 933c69a..7be4b53 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -685,7 +685,7 @@ static int fetch_git(struct discovery *heads,
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		safe_write(1, rpc.result.buf, rpc.result.len);
+		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
 	strbuf_release(&preamble);
 	free(depth_arg);

@@ -805,7 +805,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
-		safe_write(1, rpc.result.buf, rpc.result.len);
+		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
 	free(argv);
 	return err;

diff --git a/send-pack.c b/send-pack.c
index 97ab336..a99a1e4 100644
--- a/send-pack.c
+++ b/send-pack.c

@@ -281,7 +281,7 @@ int send_pack(struct send_pack_args *args,
 			send_sideband(out, -1, req_buf.buf, req_buf.len, LARGE_PACKET_MAX);
 		}
 	} else {
-		safe_write(out, req_buf.buf, req_buf.len);
+		write_or_die(out, req_buf.buf, req_buf.len);
 		packet_flush(out);
 	}
 	strbuf_release(&req_buf);

diff --git a/sideband.c b/sideband.c
index d5ffa1c..8f7b25b 100644
--- a/sideband.c
+++ b/sideband.c

@@ -1,3 +1,4 @@
+#include "cache.h"
 #include "pkt-line.h"
 #include "sideband.h"
 

@@ -108,7 +109,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 			} while (len);
 			continue;
 		case 1:
-			safe_write(out, buf + pf+1, len);
+			write_or_die(out, buf + pf+1, len);
 			continue;
 		default:
 			fprintf(stderr, "%s: protocol error: bad band #%d\n",

@@ -138,12 +139,12 @@ ssize_t send_sideband(int fd, int band, const char *data, ssize_t sz, int packet
 		if (0 <= band) {
 			sprintf(hdr, "%04x", n + 5);
 			hdr[4] = band;
-			safe_write(fd, hdr, 5);
+			write_or_die(fd, hdr, 5);
 		} else {
 			sprintf(hdr, "%04x", n + 4);
-			safe_write(fd, hdr, 4);
+			write_or_die(fd, hdr, 4);
 		}
-		safe_write(fd, p, n);
+		write_or_die(fd, p, n);
 		p += n;
 		sz -= n;
 	}

diff --git a/upload-pack.c b/upload-pack.c
index 7c05b15..04d6bd7 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -69,7 +69,8 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 		xwrite(fd, data, sz);
 		return sz;
 	}
-	return safe_write(fd, data, sz);
+	write_or_die(fd, data, sz);
+	return sz;
 }
 
 static FILE *pack_pipe = NULL;

-- 
1.8.1.20.g7078b03

Originally we had a single function for reading packetized
data: packet_read_line. Commit 46284dd grew a more "gentle"
form that would return an error instead of dying upon
reading a truncated input stream. However:

  1. The two functions were called "packet_read" and
     "packet_read_line", with no indication that the only
     difference is in the error handling.

  2. There was no documentation about which error conditions
     were handled in the gentle form, and which still caused
     a death.

  3. The internal variable to trigger the gentle mode was
     called "return_line_fail", which was not very
     expressive.

This patch converts packet_line to packet_read_line_gently
to more clearly indicate its relationship to
packet_read_line, and renames the internal variable to
"gently". This is also not incredibly expressive, but it is
at least a convention within the git code. And finally, we
document the exact behavior for the gentle and non-gentle
modes.

While we are cleaning up the names, we can drop the
"return_line_fail" checks in packet_read_internal entirely.
They look like this:

  ret = safe_read(..., return_line_fail);
  if (return_line_fail && ret < 0)
	  ...

The check for return_line_fail is a no-op; safe_read will
only ever return an error value if we passed it
return_line_fail in the first place.

Signed-off-by: Jeff King <redacted>
---
Obviously this one is a matter of taste, but I think the result is much
better. Certainly the documentation bits are hard to argue with. :)

 connect.c  |  2 +-
 pkt-line.c | 16 ++++++++--------
 pkt-line.h | 21 ++++++++++++++++++++-
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index 49e56ba..7e0920d 100644
--- a/connect.c
+++ b/connect.c

@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read(in, buffer, sizeof(buffer));
+		len = packet_read_line_gently(in, buffer, sizeof(buffer));
 		if (len < 0)
 			die_initial_contact(got_at_least_one_head);
 

diff --git a/pkt-line.c b/pkt-line.c
index 699c2dd..62479d3 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -103,13 +103,13 @@ static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
 	strbuf_add(buf, buffer, n);
 }
 
-static int safe_read(int fd, void *buffer, unsigned size, int return_line_fail)
+static int safe_read(int fd, void *buffer, unsigned size, int gently)
 {
 	ssize_t ret = read_in_full(fd, buffer, size);
 	if (ret < 0)
 		die_errno("read error");
 	else if (ret < size) {
-		if (return_line_fail)
+		if (gently)
 			return -1;
 
 		die("The remote end hung up unexpectedly");

@@ -143,13 +143,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_
 	return len;
 }
 
-static int packet_read_internal(int fd, char *buffer, unsigned size, int return_line_fail)
+static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
 {
 	int len, ret;
 	char linelen[4];
 
-	ret = safe_read(fd, linelen, 4, return_line_fail);
-	if (return_line_fail && ret < 0)
+	ret = safe_read(fd, linelen, 4, gently);
+	if (ret < 0)
 		return ret;
 	len = packet_length(linelen);
 	if (len < 0)

@@ -161,15 +161,15 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int return_
 	len -= 4;
 	if (len >= size)
 		die("protocol error: bad line length %d", len);
-	ret = safe_read(fd, buffer, len, return_line_fail);
-	if (return_line_fail && ret < 0)
+	ret = safe_read(fd, buffer, len, gently);
+	if (ret < 0)
 		return ret;
 	buffer[len] = 0;
 	packet_trace(buffer, len, 0);
 	return len;
 }
 
-int packet_read(int fd, char *buffer, unsigned size)
+int packet_read_line_gently(int fd, char *buffer, unsigned size)
 {
 	return packet_read_internal(fd, buffer, size, 1);
 }

diff --git a/pkt-line.h b/pkt-line.h
index 7a67e9c..31bd069 100644
--- a/pkt-line.h
+++ b/pkt-line.h

@@ -24,8 +24,27 @@ int packet_read_line(int fd, char *buffer, unsigned size);
 void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 
+/*
+ * Read a packetized line from the descriptor into the buffer. We will die
+ * under any of the following conditions:
+ *
+ *   1. Read error from descriptor.
+ *
+ *   2. Protocol error from the remote (e.g., bogus length characters).
+ *
+ *   3. Receiving a packet larger than "size" bytes.
+ *
+ *   4. Truncated output from the remote (e.g., we expected a packet but got
+ *      EOF, or we got a partial packet followed by EOF).
+ */
 int packet_read_line(int fd, char *buffer, unsigned size);
-int packet_read(int fd, char *buffer, unsigned size);
+
+/*
+ * Same as packet_read_line, but do not die on condition 4 (truncated input);
+ * instead return -1.  We still die for the other conditions.
+ */
+int packet_read_line_gently(int fd, char *buffer, unsigned size);
+
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 ssize_t safe_write(int, const void *, ssize_t);
 

-- 
1.8.1.20.g7078b03

If we get a packet from the remote side that is too large to
fit in our buffer, we currently complain "protocol error:
bad line length".  This is a bit vague. The line length the
other side sent is not "bad" per se; the actual problem is
that it exceeded our expectation for buffer length.

This will generally not happen between two git-core
implementations, because the sender limits themselves to
either 1000, or to LARGE_PACKET_MAX (depending on what is
being sent, sideband-64k negotiation, etc), and the receiver
uses a buffer of the appropriate size.

The protocol document indicates the LARGE_PACKET_MAX limit
(of 65520), but does not actually specify the 1000-byte
limit for ref lines. It is likely that other implementations
just create a packet as large as they need, and this doesn't
come up often because nobody has 1000-character ref names
(or close to it, counting sha1 and other boilerplate).

We may want to increase the size of our receive buffers for
ref lines to prepare for such writers (whether they are
other implementations, or if we eventually want to bump the
write size in git-core). In the meantime, this patch tries
to give a more clear message in case it does come up.

Signed-off-by: Jeff King <redacted>
---
I'm really tempted to bump all of our 1000-byte buffers to just use
LARGE_PACKET_MAX. If we provided a packet_read variant that used a
static buffer (which is fine for all but one or two callers), then it
would not take much memory (right now we stick some LARGE_PACKET_MAX
buffers on the stack, which is slightly questionable for
stack-restricted systems). But I left that for a different topic (and
even if we do, we would still want this message to catch anything over
the bizarre 65520 limit).

Out of curiosity, I grepped the list archives, and found only one
instance of this message. And it was somebody whose data stream was tainted
with random crud that happened to be numbers (much more common is "bad line
length character", when the crud does not look like a packet length).

 pkt-line.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index 62479d3..f2a7575 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -160,7 +160,8 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
 	}
 	len -= 4;
 	if (len >= size)
-		die("protocol error: bad line length %d", len);
+		die("protocol error: line too large: (expected %u, got %d)",
+		    size, len);
 	ret = safe_read(fd, buffer, len, gently);
 	if (ret < 0)
 		return ret;

-- 
1.8.1.20.g7078b03

Originally packets were used just for the line-oriented ref
advertisement and negotiation. These days, we also stuff
packfiles and sidebands into them, and they do not
necessarily represent a line. Drop the "_line" suffix, as it
is not informative and makes the function names quite long
(especially as we add "_gently" and other variants).

Signed-off-by: Jeff King <redacted>
---
Again, this is a taste issue. Can be optional.

 builtin/archive.c        | 4 ++--
 builtin/fetch-pack.c     | 2 +-
 builtin/receive-pack.c   | 2 +-
 builtin/upload-archive.c | 2 +-
 connect.c                | 2 +-
 daemon.c                 | 2 +-
 fetch-pack.c             | 6 +++---
 pkt-line.c               | 4 ++--
 pkt-line.h               | 6 +++---
 remote-curl.c            | 6 +++---
 send-pack.c              | 4 ++--
 sideband.c               | 2 +-
 upload-pack.c            | 4 ++--
 13 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index 9a1cfd3..bf600f5 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c

@@ -53,7 +53,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		packet_write(fd[1], "argument %s\n", argv[i]);
 	packet_flush(fd[1]);
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
+	len = packet_read(fd[0], buf, sizeof(buf));
 	if (!len)
 		die(_("git archive: expected ACK/NAK, got EOF"));
 	if (buf[len-1] == '\n')

@@ -66,7 +66,7 @@ static int run_remote_archiver(int argc, const char **argv,
 		die(_("git archive: protocol error"));
 	}
 
-	len = packet_read_line(fd[0], buf, sizeof(buf));
+	len = packet_read(fd[0], buf, sizeof(buf));
 	if (len)
 		die(_("git archive: expected a flush"));
 

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 940ae35..b59e60e 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c

@@ -102,7 +102,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			 */
 			static char line[1000];
 			for (;;) {
-				int n = packet_read_line(0, line, sizeof(line));
+				int n = packet_read(0, line, sizeof(line));
 				if (!n)
 					break;
 				if (line[n-1] == '\n')

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48cd5dc..3f58ce8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -736,7 +736,7 @@ static struct command *read_head_info(void)
 		char *refname;
 		int len, reflen;
 
-		len = packet_read_line(0, line, sizeof(line));
+		len = packet_read(0, line, sizeof(line));
 		if (!len)
 			break;
 		if (line[len-1] == '\n')

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..2ecf461 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c

@@ -40,7 +40,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	sent_argv[0] = "git-upload-archive";
 	for (p = buf;;) {
 		/* This will die if not enough free space in buf */
-		len = packet_read_line(0, p, (buf + sizeof buf) - p);
+		len = packet_read(0, p, (buf + sizeof buf) - p);
 		if (len == 0)
 			break;	/* got a flush */
 		if (sent_argc > MAX_ARGS - 2)

diff --git a/connect.c b/connect.c
index 7e0920d..59266b1 100644
--- a/connect.c
+++ b/connect.c

@@ -76,7 +76,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		char *name;
 		int len, name_len;
 
-		len = packet_read_line_gently(in, buffer, sizeof(buffer));
+		len = packet_read_gently(in, buffer, sizeof(buffer));
 		if (len < 0)
 			die_initial_contact(got_at_least_one_head);
 

diff --git a/daemon.c b/daemon.c
index 4602b46..ac26b93 100644
--- a/daemon.c
+++ b/daemon.c

@@ -612,7 +612,7 @@ static int execute(void)
 		loginfo("Connection from %s:%s", addr, port);
 
 	alarm(init_timeout ? init_timeout : timeout);
-	pktlen = packet_read_line(0, line, sizeof(line));
+	pktlen = packet_read(0, line, sizeof(line));
 	alarm(0);
 
 	len = strlen(line);

diff --git a/fetch-pack.c b/fetch-pack.c
index b90dadf..d89c9d0 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c

@@ -173,7 +173,7 @@ static void consume_shallow_list(struct fetch_pack_args *args, int fd)
 		 * is a block of have lines exchanged.
 		 */
 		char line[1000];
-		while (packet_read_line(fd, line, sizeof(line))) {
+		while (packet_read(fd, line, sizeof(line))) {
 			if (!prefixcmp(line, "shallow "))
 				continue;
 			if (!prefixcmp(line, "unshallow "))

@@ -216,7 +216,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 static enum ack_type get_ack(int fd, unsigned char *result_sha1)
 {
 	static char line[1000];
-	int len = packet_read_line(fd, line, sizeof(line));
+	int len = packet_read(fd, line, sizeof(line));
 
 	if (!len)
 		die("git fetch-pack: expected ACK/NAK, got EOF");

@@ -350,7 +350,7 @@ static int find_common(struct fetch_pack_args *args,
 		unsigned char sha1[20];
 
 		send_request(args, fd[1], &req_buf);
-		while (packet_read_line(fd[0], line, sizeof(line))) {
+		while (packet_read(fd[0], line, sizeof(line))) {
 			if (!prefixcmp(line, "shallow ")) {
 				if (get_sha1_hex(line + 8, sha1))
 					die("invalid shallow line: %s", line);

diff --git a/pkt-line.c b/pkt-line.c
index f2a7575..85faf73 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -170,12 +170,12 @@ int packet_read_line_gently(int fd, char *buffer, unsigned size)
 	return len;
 }
 
-int packet_read_line_gently(int fd, char *buffer, unsigned size)
+int packet_read_gently(int fd, char *buffer, unsigned size)
 {
 	return packet_read_internal(fd, buffer, size, 1);
 }
 
-int packet_read_line(int fd, char *buffer, unsigned size)
+int packet_read(int fd, char *buffer, unsigned size)
 {
 	return packet_read_internal(fd, buffer, size, 0);
 }

diff --git a/pkt-line.h b/pkt-line.h
index 31bd069..2dc4941 100644
--- a/pkt-line.h
+++ b/pkt-line.h

@@ -37,13 +37,13 @@ int packet_read_line(int fd, char *buffer, unsigned size);
  *   4. Truncated output from the remote (e.g., we expected a packet but got
  *      EOF, or we got a partial packet followed by EOF).
  */
-int packet_read_line(int fd, char *buffer, unsigned size);
+int packet_read(int fd, char *buffer, unsigned size);
 
 /*
- * Same as packet_read_line, but do not die on condition 4 (truncated input);
+ * Same as packet_read, but do not die on condition 4 (truncated input);
  * instead return -1.  We still die for the other conditions.
  */
-int packet_read_line_gently(int fd, char *buffer, unsigned size);
+int packet_read_gently(int fd, char *buffer, unsigned size);
 
 int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
 ssize_t safe_write(int, const void *, ssize_t);

diff --git a/remote-curl.c b/remote-curl.c
index 7be4b53..99cc016 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -308,7 +308,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 
 	if (!avail) {
 		rpc->initial_buffer = 0;
-		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		avail = packet_read(rpc->out, rpc->buf, rpc->alloc);
 		if (!avail)
 			return 0;
 		rpc->pos = 0;

@@ -425,7 +425,7 @@ static int post_rpc(struct rpc_state *rpc)
 			break;
 		}
 
-		n = packet_read_line(rpc->out, buf, left);
+		n = packet_read(rpc->out, buf, left);
 		if (!n)
 			break;
 		rpc->len += n;

@@ -579,7 +579,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->hdr_accept = strbuf_detach(&buf, NULL);
 
 	while (!err) {
-		int n = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
+		int n = packet_read(rpc->out, rpc->buf, rpc->alloc);
 		if (!n)
 			break;
 		rpc->pos = 0;

diff --git a/send-pack.c b/send-pack.c
index a99a1e4..7b22235 100644
--- a/send-pack.c
+++ b/send-pack.c

@@ -108,7 +108,7 @@ static int receive_status(int in, struct ref *refs)
 	struct ref *hint;
 	char line[1000];
 	int ret = 0;
-	int len = packet_read_line(in, line, sizeof(line));
+	int len = packet_read(in, line, sizeof(line));
 	if (len < 10 || memcmp(line, "unpack ", 7))
 		return error("did not receive remote status");
 	if (memcmp(line, "unpack ok\n", 10)) {

@@ -122,7 +122,7 @@ static int receive_status(int in, struct ref *refs)
 	while (1) {
 		char *refname;
 		char *msg;
-		len = packet_read_line(in, line, sizeof(line));
+		len = packet_read(in, line, sizeof(line));
 		if (!len)
 			break;
 		if (len < 3 ||

diff --git a/sideband.c b/sideband.c
index 8f7b25b..4a6446d 100644
--- a/sideband.c
+++ b/sideband.c

@@ -38,7 +38,7 @@ int recv_sideband(const char *me, int in_stream, int out)
 
 	while (1) {
 		int band, len;
-		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
+		len = packet_read(in_stream, buf + pf, LARGE_PACKET_MAX);
 		if (len == 0)
 			break;
 		if (len < 1) {

diff --git a/upload-pack.c b/upload-pack.c
index 04d6bd7..04231d7 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -428,7 +428,7 @@ static int get_common_commits(void)
 	save_commit_buffer = 0;
 
 	for (;;) {
-		int len = packet_read_line(0, line, sizeof(line));
+		int len = packet_read(0, line, sizeof(line));
 		reset_timeout();
 
 		if (!len) {

@@ -589,7 +589,7 @@ static void receive_needs(void)
 		struct object *o;
 		const char *features;
 		unsigned char sha1_buf[20];
-		len = packet_read_line(0, line, sizeof(line));
+		len = packet_read(0, line, sizeof(line));
 		reset_timeout();
 		if (!len)
 			break;

-- 
1.8.1.20.g7078b03

The packet_read function reads from a descriptor. The
packet_get_line function is similar, but reads from an
in-memory buffer, and uses a completely separate
implementation. This patch teaches the internal function
backing packet_read to accept either source, and use the
appropriate one. As a result:

  1. The function has been renamed to packet_read_from_buf,
     which more clearly indicates its relationship to the
     packet_read function.

  2. The original packet_get_line wrote to a strbuf; the new
     function, like its descriptor counterpart, reads into a
     buffer provided by the caller.

  3. The original function did not die on any errors, but
     instead returned an error code. Now we have the usual
     "normal" and "gently" forms.

There are only two existing calls to packet_get_line which
have to be converted, and both are in remote-curl. The first
reads and checks the "# service=git-foo" line from a smart
http server.  The second just reads past any additional
smart headers, without bothering to look at them.

This patch converts both to the new form, with a few
implications:

  1. Because we use the non-gentle form, the first caller
     can drop its own error checking. As a result, we will get
     more accurate error reporting about protocol breakage,
     since the errors come from inside the protocol code. We
     will no longer print the URL as part of the error, but
     that's OK. Protocol breakages should be rare (and we
     are pretty sure at this point in the code that it is a
     real smart server, so we won't be confused by dumb
     servers), and the first debugging step would probably
     be GIT_CURL_VERBOSE, anyway.

  2. The second caller did not error check at all, and now
     does. This can help us catch broken or truncated input
     close to the source.

  3. Since we are no longer using a strbuf, we now have a
     1000-byte limit on the smart-http headers. That should
     be fine, as the only header that has ever been sent
     here is the short "service=git-foo" header.

Signed-off-by: Jeff King <redacted>
---
The diffstat shows more lines appearing, but it is mainly from comments
and from the various parse_line{_from_buf,}{,_gently} variants; we
really do get rid of a duplicate parsing implementation, and we
harmonize all of the error conditions and messages.

We can also make "gently" a parameter to avoid the proliferation of
related functions, but would mean all but one callsite would have to
pass an extra "0". Choose your poison, I guess.

 pkt-line.c    | 69 ++++++++++++++++++++++++++++-------------------------------
 pkt-line.h    | 11 +++++++++-
 remote-curl.c | 19 ++++++++--------
 3 files changed, 53 insertions(+), 46 deletions(-)

diff --git a/pkt-line.c b/pkt-line.c
index 85faf73..7ee91e0 100644
--- a/pkt-line.c
+++ b/pkt-line.c

@@ -103,12 +103,26 @@ static int safe_read(int fd, void *buffer, unsigned size, int gently)
 	strbuf_add(buf, buffer, n);
 }
 
-static int safe_read(int fd, void *buffer, unsigned size, int gently)
+static int get_packet_data(int fd, char **src_buf, size_t *src_size,
+			   void *dst, unsigned size, int gently)
 {
-	ssize_t ret = read_in_full(fd, buffer, size);
-	if (ret < 0)
-		die_errno("read error");
-	else if (ret < size) {
+	ssize_t ret;
+
+	/* Read up to "size" bytes from our source, whatever it is. */
+	if (src_buf) {
+		ret = size < *src_size ? size : *src_size;
+		memcpy(dst, *src_buf, ret);
+		*src_buf += size;
+		*src_size -= size;
+	}
+	else {
+		ret = read_in_full(fd, dst, size);
+		if (ret < 0)
+			die_errno("read error");
+	}
+
+	/* And complain if we didn't get enough bytes to satisfy the read. */
+	if (ret < size) {
 		if (gently)
 			return -1;
 

@@ -143,12 +157,13 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
 	return len;
 }
 
-static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
+static int packet_read_internal(int fd, char **src_buf, size_t *src_len,
+				char *buffer, unsigned size, int gently)
 {
 	int len, ret;
 	char linelen[4];
 
-	ret = safe_read(fd, linelen, 4, gently);
+	ret = get_packet_data(fd, src_buf, src_len, linelen, 4, gently);
 	if (ret < 0)
 		return ret;
 	len = packet_length(linelen);

@@ -162,7 +177,7 @@ static int packet_read_internal(int fd, char *buffer, unsigned size, int gently)
 	if (len >= size)
 		die("protocol error: line too large: (expected %u, got %d)",
 		    size, len);
-	ret = safe_read(fd, buffer, len, gently);
+	ret = get_packet_data(fd, src_buf, src_len, buffer, len, gently);
 	if (ret < 0)
 		return ret;
 	buffer[len] = 0;

@@ -172,40 +187,22 @@ int packet_get_line(struct strbuf *out,
 
 int packet_read_gently(int fd, char *buffer, unsigned size)
 {
-	return packet_read_internal(fd, buffer, size, 1);
+	return packet_read_internal(fd, NULL, 0, buffer, size, 1);
 }
 
 int packet_read(int fd, char *buffer, unsigned size)
 {
-	return packet_read_internal(fd, buffer, size, 0);
+	return packet_read_internal(fd, NULL, 0, buffer, size, 0);
 }
 
-int packet_get_line(struct strbuf *out,
-	char **src_buf, size_t *src_len)
+int packet_read_from_buf(char *dst, unsigned dst_len,
+			 char **src_buf, size_t *src_len)
 {
-	int len;
-
-	if (*src_len < 4)
-		return -1;
-	len = packet_length(*src_buf);
-	if (len < 0)
-		return -1;
-	if (!len) {
-		*src_buf += 4;
-		*src_len -= 4;
-		packet_trace("0000", 4, 0);
-		return 0;
-	}
-	if (*src_len < len)
-		return -2;
-
-	*src_buf += 4;
-	*src_len -= 4;
-	len -= 4;
+	return packet_read_internal(-1, src_buf, src_len, dst, dst_len, 0);
+}
 
-	strbuf_add(out, *src_buf, len);
-	*src_buf += len;
-	*src_len -= len;
-	packet_trace(out->buf, out->len, 0);
-	return len;
+int packet_read_from_buf_gently(char *dst, unsigned dst_len,
+				char **src_buf, size_t *src_len)
+{
+	return packet_read_internal(-1, src_buf, src_len, dst, dst_len, 1);
 }

diff --git a/pkt-line.h b/pkt-line.h
index 2dc4941..287a391 100644
--- a/pkt-line.h
+++ b/pkt-line.h

@@ -45,7 +45,16 @@ int packet_read_gently(int fd, char *buffer, unsigned size);
  */
 int packet_read_gently(int fd, char *buffer, unsigned size);
 
-int packet_get_line(struct strbuf *out, char **src_buf, size_t *src_len);
+/*
+ * Same as packet_read above, but read from an in-memory buffer
+ * instead of a file descriptor. The src_buf and src_len are modified
+ * to iterate past the consumed data.
+ */
+int packet_read_from_buf(char *dst, unsigned dst_len,
+			 char **src_buf, size_t *src_len);
+int packet_read_from_buf_gently(char *dst, unsigned dst_len,
+				char **src_buf, size_t *src_len);
+
 ssize_t safe_write(int, const void *, ssize_t);
 
 #endif