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,
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to