patch, 3 partsmake 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 <peff@peff.net>
---
 pkt-line.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Jeff King wrote:

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

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 <peff@peff.net>
---
 remote-curl.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Jeff King wrote:

[...]

[...]

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.

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 <jrnieder@gmail.com>

Thanks.

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

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.

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

Jeff King wrote:

[...]

[...]

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.

Agreed completely.  Thanks again for the patch.

Jonathan

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

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

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

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(+)

Jeff King wrote:

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

[...]

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 03:05:34AM -0800, Jonathan Nieder wrote:

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.

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.

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 response. :)

-Peff

Jeff King wrote:

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

[...]

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.

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

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 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).

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 <peff@peff.net>
---
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(-)

This is just write_or_die by another name.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

Jeff King wrote:

That information definitely belongs in the commit message.

Me, too.  Let's see:

[...]

If the connection to send-pack is lost and stdout becomes a broken pipe and I am updating enough refs to overflow the pipe buffer, receive-pack will die with SIGPIPE. So unless the sadistic caller has set the inherited SIGPIPE action to SIG_IGN (for example by wrapping git with an uncautious Python wrapper that uses subprocess.Popen), the change to EPIPE handling is not a behavior change.

Since the pipe is closed, presumably the calling send-pack has hung up and won't notice the exit status, so this should be safe.

Arguably it would be more friendly to stay alive to run the post-receive and post-update hooks, though, given that a ref update has occurred. Maybe transport commands like this one should always set the disposition of SIGPIPE to SIG_IGN.

[...]

A signal will kill send-pack before write_or_die has a chance to intervene so this change is a no-op unless the caller is sadistic (as in the [1] case). In the signal(SIGPIPE, SIG_IGN) case, it might be a regression, since "git push" should not declare success when its connection to receive-pack closes early.

[1] http://www.chiark.greenend.org.uk/ucgi/~cjwatson/blosxom/2009-07-02-python-sigpipe.html

[...]

Also a no-op except when the parent process is insane enough to let us inherit signal(SIGPIPE, SIG_IGN).

In that case, if triggerable this looks like a bad change: if upload-pack has gone missing, the fetch should not be considered a success.

[...]

Etc.  I'm stopping here.

I'm thinking before a patch like this we should make the following change:

 1. at startup, set the signal action of SIGPIPE to SIG_DFL, to make
    the behavior a little more predictable.

Perhaps the following as well:

 2. in write_or_die(), when encountering EPIPE, set the signal action
    of SIGPIPE to SIG_DFL and raise(SIGPIPE), ensuring the exit status
    reflects the broken pipe.  If the parent process is unnecessarily
    noisy about that, that's a bug in the parent process (hopefully
    uncommon).

Or alternatively:

 2b. never set SIGPIPE to SIG_IGN except in short blocks of code that
     do not call write_or_die()

What do you think? Jonathan

On Mon, Feb 18, 2013 at 01:56:45AM -0800, Jonathan Nieder wrote:

Yeah, this one was more RFC; I was hoping for some input on the EPIPE thing, so I could know _what_ to put in the commit message. Is it safe, and if so, why? And if not, we should drop the patch.

Yeah, but I don't want to count on always catching SIGPIPE. There's the inherited signal handler thing, but there's also the fact that we may end up ignoring SIGPIPE from backend programs like upload-pack and receive-pack; they check their writes anyway, and we have already run into issues with getting SIGPIPE when we don't necessarily expect or care about it.

The nice thing about write_or_die is that it still _exits_ on EPIPE. It's just that it doesn't print an error (which is really not a big deal) and exit with a 0 return code. I really wonder if we should just change the latter. For programs which are creating copious output (e.g., "git log"), the return value is not important anyway. For backend programs, an unexpected EPIPE from something like write_or_die should probably involve a non-successful return code.

Yeah, I've suggested that in the past. And I do think it's sane, because if you took a ref update, you almost certainly want to run the post-receive, even if the client is no longer around (e.g., if it is going to email out the changeset).

But that isn't going to receive-pack, is it? Send-pack's stdout is really just going to the user (or wherever). So it would have an effect more for:

  (git push && echo >&2 OK) | grep -m1 foo

which might print "OK" even if we failed. That's quite contrived, but it is at least a measurable change. And anyway...

Yeah, there are definitely some bad ones.

I'm lukewarm on that, just because we may want to ignore SIGPIPE ourselves at some point.

I like this. My suggestion would be to just exit(1) instead of exit(0). But really, raising SIGPIPE makes the most sense, because it communicates to the parent what happened (and the shell will wisely not print a message, but careful parents like "git fetch" and "git push" will check it properly and notice that the child did not succeed).

Yuck. :)

I really like option 2. That exit(0) when we see SIGPIPE bugs me. Because we _are_ dying due to our write failing, and I think the only reason to exit(0) was to avoid unnecessary complaints from parents. But raising SIGPIPE seems like the best of both worlds to me.

-Peff

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 <peff@peff.net>
---
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(-)

Jeff King wrote:

In other words:

	Based on the names of two functions "packet_read" and
	"packet_read_line", it is not obvious which to use and what the
	ramifications of that choice are.

	Rename packet_read to packet_read_line_gently and add a comment
	explaining that the latter is a "gentler" form that returns an
	error instead of dying upon reading a truncated input stream.

	While at it:

	 * Rename the internal argument triggering the gentle mode to
	   "gentle" instead of "return_line_fail".

	 * Drop the redundant "return_line_fail &&" in checks like
	   "if (return_line_fail && ret < 0)".  safe_read() never
	   returns an error when !gentle.

	No functional change intended.

FWIW, the patch itself is
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

On Mon, Feb 18, 2013 at 02:12:09AM -0800, Jonathan Nieder wrote:

Hmph. I had originally written a commit message organized more like yours, then I changed it to try to be more clear. I guess that didn't work.

But yes, you get the intent exactly.

-Peff

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 <peff@peff.net>
---
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(-)

Jeff King <peff@peff.net> writes:

I thought that 1000-byte limit was kept when we introduced the 64k interface to interoperate with other side that does not yet support the 64k interface. Is your justification that such an old version of Git no longer matters in the real world (which is true, I think), or we use 1000-byte limit in some codepaths even when we know that we are talking with a 64k-capable version of Git on the other side?

On Mon, Feb 18, 2013 at 01:40:17AM -0800, Junio C Hamano wrote:

I should have been more clear that I want to bump only the _reading_ side, not the writing.

The sideband-64k capability bumps the sideband chunk size. But the size for packetized ref advertisement lines has remained at 1000. Which it must, since we start outputting them before doing capability negotiation. The sender will die before writing a longer ref line (see pkg-line.c:format_packet), and most of the reading callsites feed a 1000-byte buffer to packet_read_line (which will die if we get a larger packet). So the upgrade path for that would be:

  1. Git bumps up all reading buffers to LARGE_PACKET_MAX, just in case.
     This immediately covers us for any alternate implementations that
     send larger ref packets (I do not know if any exist, but the
     documentation does not mention this limitation at all, so I would
     not be surprised if other implementations just use LARGE_PACKET_MAX
     as a maximum).

  2. Many years pass. We decide that Git v1.8.2 and older are ancient
     history and not worth caring about.

  3. We bump the 1000-byte limit for format_packet to LARGE_PACKET_MAX.

This is not pressing at all; I wouldn't have even noticed it if I hadn't been wondering how large to make the new buffer I was adding in a later patch. I have not heard of anyone running up against this limit in practice. But it's easy to do (1), and it starts the clock ticking for the 1000-byte readers to become obsolete.

-Peff

Jeff King <peff@peff.net> writes:

Yup, I agree with that goal.

On Mon, Feb 18, 2013 at 01:25:35PM -0800, Junio C Hamano wrote:

Having just looked at the pkt-line callers a lot, I think most of them could go for something like:

  char *packet_read(int fd, unsigned *len_p)
  {
          static char buffer[LARGE_PACKET_MAX];
          int len;

          len = packet_read_to_buf(fd, buffer, sizeof(buffer));
          if (len < 0)
                  return NULL;

          *len_p = len;
          return buffer;
  }

That would actually simplify the callers a bit, and would harmonize the buffer sizes at the same time. I'll look into doing a series tonight.

-Peff

On Mon, Feb 18, 2013 at 04:33:31PM -0500, Jeff King wrote:

Just a quick update on this series. It ended up taking more nights than I thought. :) The result looks much better than what I posted before, and I found several other corner cases and bugs in packet parsing, too.

I'm going to hold off on posting it tonight, as I'm now up to 19 patches, and after several rounds of "rebase -i", I need to give it a final read-through myself before inflicting it on anyone else. I'll do that and post it tomorrow.

In the meantime, please hold off on what I've posted so far (that includes the jk/smart-http-robustify topic).

-Peff

Jeff King <peff@peff.net> wrote:

Surely. I'm done for the night already. Looking forward to see the reroll tomorrow.

Thanks.

Jeff King wrote:

Makes sense. I think this should say "expected < %u, got %d", since we don't actually expect most lines to be 1004 bytes in practice.

With or without such a change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

On Mon, Feb 18, 2013 at 02:15:23AM -0800, Jonathan Nieder wrote:

Yeah, I had toyed with writing "expected max %u" for the same reason. I'll tweak it in the re-roll.

-Peff

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 <peff@peff.net>
---
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(-)

Jeff King wrote:

In combination with patch 3, this changes the meaning of packet_read() without changing its signature, which could make other patches cherry-picked on top change behavior in unpredictable ways. :(

So I'd be all for this if the signature changes (for example to put the fd at the end or something), but not so if not.

Thanks, Jonathan

On Mon, Feb 18, 2013 at 02:19:15AM -0800, Jonathan Nieder wrote:

True. Though packet_read has only existed since last June, only had one callsite (which would now conflict, since I'm touching it in this series), and has no new calls in origin..origin/pu. So it's relatively low risk for such a problem. I don't know how careful we want to be.

-Peff

Jeff King wrote:

I was unclear. What I am worried about is that someone using a version of git without this patch will try some yet-to-be-written patch using packet_read from the mailing list and not notice that they are using the wrong function. For example, if someone is using 1.7.12.y or 1.8.1.y and wants to try a patch from after the above, they would get subtly different and wrong results.

The rule "change the name or signature when breaking the ABI of a global function" is easy to remember and follow. I think we want not to have to be careful at all, and such rules can help with that. :)

Thanks, Jonathan

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 <peff@peff.net>
---
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(-)

Jeff King wrote:

Ah, so this introduces a new analagous helper that reads from a strbuf, to avoid the copy-from-async-procedure hack?

[...]

Style: git cuddles its "else"s.

	assert(src_buf ? fd < 0 : fd >= 0);

	if (src_buf) {
		...
	} else {
		...
	}

This is noisy about upstream pipe gone missing, which makes sense since this is transport-related. Maybe that deserves a comment.

[...]

Was anything guaranteeing that buffer.len < 1000 before this change?

The rest looks good from a quick glance.

Jonathan

On Mon, Feb 18, 2013 at 02:43:50AM -0800, Jonathan Nieder wrote:

Not from a strbuf, but basically, yes.

That is not new code; it is just reindented from the original safe_read. But is it noisy about a missing pipe? We do not get EPIPE for reading. We should just get a short read or EOF, both of which is handled later.

No. That's discussed in point (3) of the "implications" in the commit message.

-Peff

Jeff King wrote:

Ah, that makes more sense.

[...]

Thanks.  Sorry I missed it the first time.

Now that we can read packet data from memory as easily as a descriptor, get_remote_heads can take either one as a source. This will allow further refactoring in remote-curl.

Signed-off-by: Jeff King <peff@peff.net>
---
There aren't that many callers of get_remote_heads, so I just added the
optional parameters and tweaked each callsite. We can do it as a
get_remote_heads_from_buf wrapper function and leave them be, if we
want.

 builtin/fetch-pack.c |  2 +-
 builtin/send-pack.c  |  2 +-
 cache.h              |  4 +++-
 connect.c            | 10 +++++++---
 remote-curl.c        |  2 +-
 transport.c          |  6 +++---
 6 files changed, 16 insertions(+), 10 deletions(-)

Until recently, get_remote_heads only knew how to read refs from a file descriptor. To hack around this, we spawned a thread (or forked a process) to write the buffer back to us.

Now that we can just pass it our buffer directly, we don't have to use this hack anymore.

Signed-off-by: Jeff King <peff@peff.net>
---
I don't know that this code was hurting anything, but it has always
struck me as ugly and a possible source of error. And now it's gone.

 remote-curl.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

Jeff King wrote:

Heh.  Belongs in the commit message, presumably.

I don't think the async procedure was very harmful, but it's nice to avoid the cost of a new thread and some copying.

Nice.  The patch looks sane.

Jeff King <peff@peff.net> writes:

Hmph. A flush packet was supposed to be a mark to say "all the packets before this one can be buffered and kept without getting passed to write(2), but this and all such buffered data _must_ go on the wire _now_". So in the sense, ERR not followed by a flush may not even have a chance to be seen on the other end, no? That is what the comment before the implementation of packet_flush() is all about.

On Mon, Feb 18, 2013 at 01:29:16AM -0800, Junio C Hamano wrote:

Despite the name, I always thought of packet_flush as more of a signal for the _receiver_, who then knows that they have gotten all of a particular list. In other words, we seem to use it as a sequence point as much as anything (mostly because we immediately write() out any other packets anyway, so there is no flushing to be done; but perhaps there were originally thoughts that we could do more buffering on the writing side).

-Peff

Jeff King <peff@peff.net> writes:

Exactly.

The logic behind the comment "we do not buffer, but we should" is that flush tells the receiver that the sending end is "done" feeding it a class of data, and the data before flush do not have to reach the receiver immediately, hence we can afford to buffer on the sending end if we can measure that doing so would give us better performance. We haven't measure and turned buffering on yet.

But when dying, we may of course have data before flushing. We may disconnect (by dying) without showing flush (or preceding ERR) to the other side, and for that reason, not relying on the flush packet on the receiving end is a good change. Once we turn buffering on, we probably need to be careful when sending an ERR indicator by making it always drain any buffered data and show the ERR indicator without buffering, or something.

On Mon, Feb 18, 2013 at 01:49:37AM -0800, Junio C Hamano wrote:

Yeah, I'd agree. An ERR packet should always be accompanied by a flush (whether an actual flush packet or not doesn't really matter, but definitely a buffer flush on the sender). But I think that is really the sender's problem, and we can deal with it there if and when we buffer.

an ERR as the end of the data stream. They do not have to be picky about whether it is followed by more data, or by a flush packet, or whatever. But that is not a change I am introducing; that is how get_remote_heads has always worked. I am merely correcting the proposed change from v1 of the series that did not correctly take that into account (and therefore regressed the error message in the ERR case).

-Peff

On Mon, Feb 18, 2013 at 1:49 AM, Junio C Hamano <gitster@pobox.com> wrote:

This is also my understanding of the flush-pkt ("0000"). Its an end-of-list/end-of-section marker to let the peer know the protocol is moving to the next state. Except where the protocol can move to the next state without a flush-pkt of course (see below).

So funny story. JGit actually buffers the pkt-line writes in memory and does flushes to the network socket when any of the following happen:

- fixed size in-memory buffer is full (8k or 32k by default)
- flush-pkt is needed in the protocol
- JGit forces a flush without a flush-pkt

There are places in the protocol where data needs to be shared with the peer *despite* not having a flush-pkt present in the data stream to do that. ERR is one of these places. "done\n" at the end of the negotiation in a client is another. Sending ACK/NAK in a multi_ack from upload-pack is another. I may be missing more.

JGit had to define three methods to make the pkt-line protocol work correctly:

- writeString: format a string as a single pkt-line, insert into buffer.
- end: write "000" into the buffer, flush the buffer.
- flush: flush the buffer if it has any content.

I always found that comment in front of that function funny. Its totally not true. Fixing it is harder than just stuffing a buffer in there and hoping for the best. The callers need work. At which point that function isn't really what its author was trying to do.

Yes.

The ref-parsing functions are static. Let's move them up in the file to be available to more functions, which will help us with later refactoring.

Signed-off-by: Jeff King <peff@peff.net>
---
Just a cleanup for the next patch. We could also just do extra
declarations at the top.

 remote-curl.c | 117 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 59 insertions(+), 58 deletions(-)

When remote-curl receives a list of refs from a server, it keeps the whole buffer intact. When we get a "list" command, we feed the result to get_remote_heads, and when we get a "fetch" or "push" command, we feed it to fetch-pack or send-pack, respectively.

If the 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.

We do notice the broken ref list if we feed it to get_remote_heads. So if git asks the helper to do a "list" followed by a "fetch", we are safe; we'll abort during the list operation, which parses the refs.

This patch teaches remote-curl to always parse and save the incoming ref list when we read the ref advertisement from a server. That means that we will always verify and abort before even running fetch-pack (or send-pack) when reading a corrupted list, even if we do not run the "list" command explicitly.

Since we save the result, in the common case of running "list" then "fetch", we do not do any extra parsing at all. In the case of just a "fetch", we do an extra round of parsing, but only once.

Note also that the "fetch" case will now also initialize server_capabilities from the remote (in remote-curl; we already would do so inside fetch-pack). Doing "list+fetch" already does this. It doesn't actually matter now, but the new behavior is arguably more correct, should remote-curl ever start caring about the server's capability list.

Signed-off-by: Jeff King <peff@peff.net>
---
And this does the equivalent of patch 3/3 from the first series, but I
think this is much more robust (certainly it solves the ERR problem, but
more importantly, it uses the exact same function that other code paths
do, so we do not have to worry about it diverging).

 remote-curl.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Jeff King wrote:

I like.

On Mon, Feb 18, 2013 at 1:30 AM, Jeff King <peff@peff.net> wrote:

...

Eek. Thanks for fixing this.

On Mon, Feb 18, 2013 at 2:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Me too.

On Mon, Feb 18, 2013 at 1:12 AM, Jeff King <peff@peff.net> wrote:

JGit (and by extension Gerrit Code Review, android.googlesource.com) sends ERR with no flush-pkt. I would like to sort of keep the protocol this way, given how many servers in the wild are running Gerrit and currently use ERR with no flush-pkt. IMHO its a little late to be closing that door and stuffing a flush-pkt after the ERR that ends the conversation.