{"thread":{"id":"40711","subject":"[PATCH v4] http.c: use CURLOPT_RANGE for range requests","startedAt":"2015-11-02T21:39:58Z","lastAt":"2015-11-02T21:53:24Z","messageCount":2,"participants":["David Turner","Jeff King"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"272760","messageId":"1446500398-12257-1-git-send-email-dturner@twopensource.com","threadId":"40711","inReplyTo":null,"subject":"[PATCH v4] http.c: use CURLOPT_RANGE for range requests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-02T21:39:58Z","receivedAt":"2015-11-02T21:39:58Z","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 one incorporates Jeff's suggestions about off_t.  It also\nsimplifies by removing the possiblity of a missing low-end of a range;\nthe entire point of this function is to add a range HEADER and a\nrange of - is nugatory.\n\n---\n http.c | 33 ++++++++++++---------------------\n http.h |  1 -\n 2 files changed, 12 insertions(+), 22 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 6b89dea..f9a0dc5 100644\n--- a/http.c\n+++ b/http.c\n@@ -30,7 +30,6 @@ static CURL *curl_default;\n #endif\n \n #define PREV_BUF_SIZE 4096\n-#define RANGE_HEADER_SIZE 30\n \n char curl_errorstr[CURL_ERROR_SIZE];\n \n@@ -692,6 +691,7 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);\n #endif\n@@ -1184,6 +1184,13 @@ static const char *get_accept_language(void)\n \treturn cached_accept_language;\n }\n \n+static void http_opt_request_remainder(CURL *curl, off_t pos)\n+{\n+\tchar buf[128];\n+\txsnprintf(buf, sizeof(buf), \"%\"PRIuMAX\"-\", (uintmax_t)pos);\n+\tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -1212,11 +1219,8 @@ static int http_request(const char *url,\n \t\t\tlong posn = ftell(result);\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_reset(&buf);\n-\t\t\t}\n+\t\t\tif (posn > 0)\n+\t\t\t\thttp_opt_request_remainder(slot->curl, posn);\n \t\t} else\n \t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n \t\t\t\t\t fwrite_buffer);\n@@ -1526,10 +1530,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@@ -1593,7 +1593,6 @@ struct http_pack_request *new_http_pack_request(\n \tstruct packed_git *target, const char *base_url)\n {\n \tlong prev_posn = 0;\n-\tchar range[RANGE_HEADER_SIZE];\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct http_pack_request *preq;\n \n@@ -1631,10 +1630,7 @@ 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\thttp_opt_request_remainder(preq->slot->curl, prev_posn);\n \t}\n \n \treturn preq;\n@@ -1684,8 +1680,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tchar prev_buf[PREV_BUF_SIZE];\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,7 @@ 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\thttp_opt_request_remainder(freq->slot->curl, prev_posn);\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":"272762","messageId":"20151102215324.GA13765@sigill.intra.peff.net","threadId":"40711","inReplyTo":"1446500398-12257-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v4] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-02T21:53:24Z","receivedAt":"2015-11-02T21:53:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2015 at 04:39:58PM -0500, 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> \n> Signed-off-by: David Turner <dturner@twopensource.com>\n> ---\n> \n> This one incorporates Jeff's suggestions about off_t.  It also\n> simplifies by removing the possiblity of a missing low-end of a range;\n> the entire point of this function is to add a range HEADER and a\n> range of - is nugatory.\n\nThanks, looks good to me. And I learned a new vocabulary word. :)\n\n-Peff\n"}]}