Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2015, 23:13 UTC
Message-ID: <xmqqwpu4hrvq.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1446245682-18087-1-git-send-email-dturner@twopensource.com>

David Turner <dturner@twopensource.com> writes:

Show 8 quoted lines

> A HTTP server is permitted to return a non-range response to a HTTP
> range request (and Apache httpd in fact does this in some cases).
> While libcurl knows how to correctly handle this (by skipping bytes
> before and after the requested range), it only turns on this handling
> if it is aware that a range request is being made.  By manually
> setting the range header instead of using CURLOPT_RANGE, we were
> hiding the fact that this was a range request from libcurl.  This
> could cause corruption.

Wow, that looks really bad.

Show 21 quoted lines

> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>
> This version applies on top of pu.  It also catches all of the range
> requests instead of just the one that I happened to notice.
>
> ---
>  http.c | 24 ++++++++----------------
>  http.h |  1 -
>  2 files changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/http.c b/http.c
> index 6b89dea..16610b9 100644
> --- a/http.c
> +++ b/http.c
> @@ -30,7 +30,7 @@ static CURL *curl_default;
>  #endif
>  
>  #define PREV_BUF_SIZE 4096
> -#define RANGE_HEADER_SIZE 30
> +#define RANGE_HEADER_SIZE 17

Was this change necessary and is 17-byte string sufficient for 64-bit longs?

Show 10 quoted lines

> @@ -1213,8 +1213,9 @@ static int http_request(const char *url,
>  			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
>  					 fwrite);
>  			if (posn > 0) {
> -				strbuf_addf(&buf, "Range: bytes=%ld-", posn);
> -				headers = curl_slist_append(headers, buf.buf);
> +				strbuf_addf(&buf, "%ld-", posn);
> +				curl_easy_setopt(slot->curl, CURLOPT_RANGE,
> +						 &buf.buf);
>  				strbuf_reset(&buf);

Makes me wonder if all the callers have a CURL* and a long so that a new helper range_opt_ask_remainder(CURL *, long) can make the resulting code even simpler; we only say "we know what comes before this position, give us everything from there" in all callers, right?