On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:
> 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.
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.
> [...]
> > 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.
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html