{"thread":{"id":"40683","subject":"[PATCH v2] http.c: use CURLOPT_RANGE for range requests","startedAt":"2015-10-30T22:54:42Z","lastAt":"2015-11-02T07:05:05Z","messageCount":8,"participants":["David Turner","Junio C Hamano","Jeff King","Daniel Stenberg"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"272597","messageId":"1446245682-18087-1-git-send-email-dturner@twopensource.com","threadId":"40683","inReplyTo":null,"subject":"[PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-10-30T22:54:42Z","receivedAt":"2015-10-30T22:54:42Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"A HTTP server is permitted to return a non-range response to a HTTP\nrange request (and Apache httpd in fact does this in some cases).\nWhile libcurl knows how to correctly handle this (by skipping bytes\nbefore and after the requested range), it only turns on this handling\nif it is aware that a range request is being made.  By manually\nsetting the range header instead of using CURLOPT_RANGE, we were\nhiding the fact that this was a range request from libcurl.  This\ncould cause corruption.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\n---\n\nThis version applies on top of pu.  It also catches all of the range\nrequests instead of just the one that I happened to notice.\n\n---\n http.c | 24 ++++++++----------------\n http.h |  1 -\n 2 files changed, 8 insertions(+), 17 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 6b89dea..16610b9 100644\n--- a/http.c\n+++ b/http.c\n@@ -30,7 +30,7 @@ static CURL *curl_default;\n #endif\n \n #define PREV_BUF_SIZE 4096\n-#define RANGE_HEADER_SIZE 30\n+#define RANGE_HEADER_SIZE 17\n \n char curl_errorstr[CURL_ERROR_SIZE];\n \n@@ -1213,8 +1213,9 @@ static int http_request(const char *url,\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n \t\t\t\t\t fwrite);\n \t\t\tif (posn > 0) {\n-\t\t\t\tstrbuf_addf(&buf, \"Range: bytes=%ld-\", posn);\n-\t\t\t\theaders = curl_slist_append(headers, buf.buf);\n+\t\t\t\tstrbuf_addf(&buf, \"%ld-\", posn);\n+\t\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE,\n+\t\t\t\t\t\t &buf.buf);\n \t\t\t\tstrbuf_reset(&buf);\n \t\t\t}\n \t\t} else\n@@ -1526,10 +1527,6 @@ void release_http_pack_request(struct http_pack_request *preq)\n \t\tfclose(preq->packfile);\n \t\tpreq->packfile = NULL;\n \t}\n-\tif (preq->range_header != NULL) {\n-\t\tcurl_slist_free_all(preq->range_header);\n-\t\tpreq->range_header = NULL;\n-\t}\n \tpreq->slot = NULL;\n \tfree(preq->url);\n \tfree(preq);\n@@ -1631,10 +1628,8 @@ struct http_pack_request *new_http_pack_request(\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Resuming fetch of pack %s at byte %ld\\n\",\n \t\t\t\tsha1_to_hex(target->sha1), prev_posn);\n-\t\txsnprintf(range, sizeof(range), \"Range: bytes=%ld-\", prev_posn);\n-\t\tpreq->range_header = curl_slist_append(NULL, range);\n-\t\tcurl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,\n-\t\t\tpreq->range_header);\n+\t\txsnprintf(range, sizeof(range), \"%ld-\", prev_posn);\n+\t\tcurl_easy_setopt(preq->slot->curl, CURLOPT_RANGE, range);\n \t}\n \n \treturn preq;\n@@ -1685,7 +1680,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tssize_t prev_read = 0;\n \tlong prev_posn = 0;\n \tchar range[RANGE_HEADER_SIZE];\n-\tstruct curl_slist *range_header = NULL;\n \tstruct http_object_request *freq;\n \n \tfreq = xcalloc(1, sizeof(*freq));\n@@ -1791,10 +1785,8 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t\tfprintf(stderr,\n \t\t\t\t\"Resuming fetch of object %s at byte %ld\\n\",\n \t\t\t\thex, prev_posn);\n-\t\txsnprintf(range, sizeof(range), \"Range: bytes=%ld-\", prev_posn);\n-\t\trange_header = curl_slist_append(range_header, range);\n-\t\tcurl_easy_setopt(freq->slot->curl,\n-\t\t\t\t CURLOPT_HTTPHEADER, range_header);\n+\t\txsnprintf(range, sizeof(range), \"%ld-\", prev_posn);\n+\t\tcurl_easy_setopt(freq->slot->curl, CURLOPT_RANGE, range);\n \t}\n \n \treturn freq;\ndiff --git a/http.h b/http.h\nindex 49afe39..4f97b60 100644\n--- a/http.h\n+++ b/http.h\n@@ -190,7 +190,6 @@ struct http_pack_request {\n \tstruct packed_git **lst;\n \tFILE *packfile;\n \tchar tmpfile[PATH_MAX];\n-\tstruct curl_slist *range_header;\n \tstruct active_request_slot *slot;\n };\n \n-- \n2.4.2.691.g714732c-twtrsrc\n"},{"id":"272598","messageId":"xmqqwpu4hrvq.fsf@gitster.mtv.corp.google.com","threadId":"40683","inReplyTo":"1446245682-18087-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T23:13:29Z","receivedAt":"2015-10-30T23:13:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> A HTTP server is permitted to return a non-range response to a HTTP\n> range request (and Apache httpd in fact does this in some cases).\n> While libcurl knows how to correctly handle this (by skipping bytes\n> before and after the requested range), it only turns on this handling\n> if it is aware that a range request is being made.  By manually\n> setting the range header instead of using CURLOPT_RANGE, we were\n> hiding the fact that this was a range request from libcurl.  This\n> could cause corruption.\n\nWow, that looks really bad.\n\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n>\n> This version applies on top of pu.  It also catches all of the range\n> requests instead of just the one that I happened to notice.\n>\n> ---\n>  http.c | 24 ++++++++----------------\n>  http.h |  1 -\n>  2 files changed, 8 insertions(+), 17 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 6b89dea..16610b9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -30,7 +30,7 @@ static CURL *curl_default;\n>  #endif\n>  \n>  #define PREV_BUF_SIZE 4096\n> -#define RANGE_HEADER_SIZE 30\n> +#define RANGE_HEADER_SIZE 17\n\nWas this change necessary and is 17-byte string sufficient for\n64-bit longs?\n\n> @@ -1213,8 +1213,9 @@ static int http_request(const char *url,\n>  \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n>  \t\t\t\t\t fwrite);\n>  \t\t\tif (posn > 0) {\n> -\t\t\t\tstrbuf_addf(&buf, \"Range: bytes=%ld-\", posn);\n> -\t\t\t\theaders = curl_slist_append(headers, buf.buf);\n> +\t\t\t\tstrbuf_addf(&buf, \"%ld-\", posn);\n> +\t\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE,\n> +\t\t\t\t\t\t &buf.buf);\n>  \t\t\t\tstrbuf_reset(&buf);\n\nMakes me wonder if all the callers have a CURL* and a long so that a\nnew helper range_opt_ask_remainder(CURL *, long) can make the\nresulting code even simpler; we only say \"we know what comes before\nthis position, give us everything from there\" in all callers, right?\n"},{"id":"272600","messageId":"20151031000837.GA25849@sigill.intra.peff.net","threadId":"40683","inReplyTo":"1446245682-18087-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-31T00:08:37Z","receivedAt":"2015-10-31T00:08:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2015 at 06:54:42PM -0400, David Turner wrote:\n\n> A HTTP server is permitted to return a non-range response to a HTTP\n> range request (and Apache httpd in fact does this in some cases).\n> While libcurl knows how to correctly handle this (by skipping bytes\n> before and after the requested range), it only turns on this handling\n> if it is aware that a range request is being made.  By manually\n> setting the range header instead of using CURLOPT_RANGE, we were\n> hiding the fact that this was a range request from libcurl.  This\n> could cause corruption.\n\nThe goal makes sense. Why weren't we using CURLOPT_RANGE before? Did it\nnot exist (or otherwise have limitations) in 2005, and if so, when did\nit become usable? Do we need to protect this with an #ifdef for the curl\nversion?\n\n> @@ -1213,8 +1213,9 @@ static int http_request(const char *url,\n>  \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n>  \t\t\t\t\t fwrite);\n>  \t\t\tif (posn > 0) {\n> -\t\t\t\tstrbuf_addf(&buf, \"Range: bytes=%ld-\", posn);\n> -\t\t\t\theaders = curl_slist_append(headers, buf.buf);\n> +\t\t\t\tstrbuf_addf(&buf, \"%ld-\", posn);\n> +\t\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE,\n> +\t\t\t\t\t\t &buf.buf);\n>  \t\t\t\tstrbuf_reset(&buf);\n>  \t\t\t}\n>  \t\t} else\n\nWe reuse curl slots from request to request, and curl options may\npersist.  The old code appended to the header list, which then gets\nadded to the curl object with CURLOPT_HTTPHEADER. Subsequent requests,\neven if they are not using ranges, would also set CURLOPT_HTTPHEADER,\npossibly to NULL, so the range header would not accidentally creep into\nthe next request.\n\nBut with CURLOPT_RANGE, I think the value would persist to the next\nrequest (unless curl automagically forgets it after making the request,\nbut I don't think it generally does so). I think the cleanest thing\nwould be to reset it back to NULL in get_active_slot (there are several\nother fields that we reset there, too).\n\n> @@ -1685,7 +1680,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n>  \tssize_t prev_read = 0;\n>  \tlong prev_posn = 0;\n>  \tchar range[RANGE_HEADER_SIZE];\n> -\tstruct curl_slist *range_header = NULL;\n>  \tstruct http_object_request *freq;\n\nJunio asked elsewhere if we really need to tweak RANGE_HEADER_SIZE. But\nI think we should go even further, and bump the definition much closer\nto the point of use[1], and not worry about an arbitrary constant. After\nall, we already use sizeof(), so we do not even end up repeating the\nconstant.\n\nWe could even hide the whole thing away with something like:\n\n  void http_set_range(CURL *curl, long lo, long hi)\n  {\n\tchar buf[128];\n\tint len = 0;\n\n\tif (lo >= 0)\n\t\tlen += xsnprintf(buf + len, \"%ld\", lo);\n\tlen += xsnprintf(buf + len, \"-\");\n\tif (hi >= 0)\n\t\tlen += xsnprintf(buf + len, \"%ld\", hi);\n\n\tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n  }\n\nThat would also make it easier to replace if we do need to keep an\n#ifdef for older versions of curl. But maybe it is just\nover-engineering.\n\n-Peff\n\n[1] Antique versions of curl needed the buffer to remain valid through\n    the whole request, but these days it makes its own copy (and even\n    under the old regime, I am not sure if the existing code would work\n    anyway, as we return the request from this function, and the buffer\n    is function-local).\n"},{"id":"272631","messageId":"xmqqfv0rhr7m.fsf@gitster.mtv.corp.google.com","threadId":"40683","inReplyTo":"20151031000837.GA25849@sigill.intra.peff.net","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-31T17:40:13Z","receivedAt":"2015-10-31T17:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We could even hide the whole thing away with something like:\n>\n>   void http_set_range(CURL *curl, long lo, long hi)\n>   {\n> \tchar buf[128];\n> \tint len = 0;\n>\n> \tif (lo >= 0)\n> \t\tlen += xsnprintf(buf + len, \"%ld\", lo);\n> \tlen += xsnprintf(buf + len, \"-\");\n> \tif (hi >= 0)\n> \t\tlen += xsnprintf(buf + len, \"%ld\", hi);\n>\n> \tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n>   }\n>\n> That would also make it easier to replace if we do need to keep an\n> #ifdef for older versions of curl. But maybe it is just\n> over-engineering.\n\nI personally do not think this is an over-engineered version.  This\nis exactly what I had in mind when I alluded to a small helper ;-)\n"},{"id":"272633","messageId":"20151031174312.GA2962@sigill.intra.peff.net","threadId":"40683","inReplyTo":"xmqqfv0rhr7m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-10-31T17:43:12Z","receivedAt":"2015-10-31T17:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 31, 2015 at 10:40:13AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > We could even hide the whole thing away with something like:\n> >\n> >   void http_set_range(CURL *curl, long lo, long hi)\n> >   {\n> > \tchar buf[128];\n> > \tint len = 0;\n> >\n> > \tif (lo >= 0)\n> > \t\tlen += xsnprintf(buf + len, \"%ld\", lo);\n> > \tlen += xsnprintf(buf + len, \"-\");\n> > \tif (hi >= 0)\n> > \t\tlen += xsnprintf(buf + len, \"%ld\", hi);\n> >\n> > \tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n> >   }\n> >\n> > That would also make it easier to replace if we do need to keep an\n> > #ifdef for older versions of curl. But maybe it is just\n> > over-engineering.\n> \n> I personally do not think this is an over-engineered version.  This\n> is exactly what I had in mind when I alluded to a small helper ;-)\n\nYeah, I somehow missed your suggestion before writing that. I think the\nimportant thing your example noted is that we would always pass -1 for\n\"hi\" in all the current callers, so we can simplify this quite a bit,\nand use only one snprintf.\n\n-Peff\n"},{"id":"272714","messageId":"alpine.DEB.2.11.1511012359150.24283@tvnag.unkk.fr","threadId":"40683","inReplyTo":"20151031000837.GA25849@sigill.intra.peff.net","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2015-11-01T23:00:42Z","receivedAt":"2015-11-01T23:00:42Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Fri, 30 Oct 2015, Jeff King wrote:\n\n> The goal makes sense. Why weren't we using CURLOPT_RANGE before? Did it not \n> exist (or otherwise have limitations) in 2005, and if so, when did it become \n> usable? Do we need to protect this with an #ifdef for the curl version?\n\nCURLOPT_RANGE existed already in the first libcurl release: version 7.1, \nrelased in August 2000.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"272715","messageId":"20151102015157.GA25597@sigill.intra.peff.net","threadId":"40683","inReplyTo":"alpine.DEB.2.11.1511012359150.24283@tvnag.unkk.fr","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-02T01:51:58Z","receivedAt":"2015-11-02T01:51:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2015 at 12:00:42AM +0100, Daniel Stenberg wrote:\n\n> On Fri, 30 Oct 2015, Jeff King wrote:\n> \n> >The goal makes sense. Why weren't we using CURLOPT_RANGE before? Did it\n> >not exist (or otherwise have limitations) in 2005, and if so, when did it\n> >become usable? Do we need to protect this with an #ifdef for the curl\n> >version?\n> \n> CURLOPT_RANGE existed already in the first libcurl release: version 7.1,\n> relased in August 2000.\n\nAh, thanks. I guess we don't have to worry about that, then.\n\nWhile I have your attention, Daniel, am I correct in assuming that\nperforming a second unrelated request with the same CURL object will\nneed an explicit:\n\n  curl_easy_setopt(curl, CURLOPT_RANGE, NULL);\n\nto avoid using the range twice?\n\n-Peff\n"},{"id":"272722","messageId":"alpine.DEB.2.11.1511020804220.24283@tvnag.unkk.fr","threadId":"40683","inReplyTo":"20151102015157.GA25597@sigill.intra.peff.net","subject":"Re: [PATCH v2] http.c: use CURLOPT_RANGE for range requests","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2015-11-02T07:05:05Z","receivedAt":"2015-11-02T07:05:05Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Sun, 1 Nov 2015, Jeff King wrote:\n\n> While I have your attention, Daniel, am I correct in assuming that \n> performing a second unrelated request with the same CURL object will need an \n> explicit:\n>\n>  curl_easy_setopt(curl, CURLOPT_RANGE, NULL);\n>\n> to avoid using the range twice?\n\nCorrect. As the options stick until modified.\n\n-- \n\n  / daniel.haxx.se\n"}]}