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?
[...]
> --- a/remote-curl.c
> +++ b/remote-curl.c
[...]
> @@ -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
--
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