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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 17, 2013, 11:05 UTC
Message-ID: <20130217110533.GF6759@elie.Belkin>
In-Reply-To: <20130216064929.GC22626@sigill.intra.peff.net>

Jeff King wrote:

Show 11 quoted lines

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

[...]

Show 5 quoted lines

> 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/remote-curl.c
> +++ b/remote-curl.c

[...]

Show 6 quoted lines

> @@ -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);

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