{"thread":{"id":"40710","subject":"[PATCH v3] http.c: use CURLOPT_RANGE for range requests","startedAt":"2015-11-02T19:36:26Z","lastAt":"2015-11-02T22:45:57Z","messageCount":6,"participants":["David Turner","Jeff King","Andreas Schwab"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"272749","messageId":"1446492986-32350-1-git-send-email-dturner@twopensource.com","threadId":"40710","inReplyTo":null,"subject":"[PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-02T19:36:26Z","receivedAt":"2015-11-02T19:36:26Z","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 breaks the range option formatting/setting out to a\nhelper function, as suggested by Junio and Jeff.\n\nIn addition, it clears the range option when curl slots are cleared\nbefore reuse, also as suggested\n\n---\n http.c | 39 ++++++++++++++++++---------------------\n http.h |  1 -\n 2 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 6b89dea..eea5836 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,19 @@ static const char *get_accept_language(void)\n \treturn cached_accept_language;\n }\n \n+static void http_opt_request_remainder(CURL *curl, ssize_t lo)\n+{\n+\tchar buf[128];\n+\tint len = 0;\n+\n+\tif (lo >= 0)\n+\t\tlen += xsnprintf(buf + len, 128 - len, \"%\"PRIiMAX,\n+\t\t\t\t (intmax_t)lo);\n+\tlen += xsnprintf(buf + len, 128 - len, \"-\");\n+\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 +1225,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 +1536,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 +1599,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 +1636,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 +1686,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 +1791,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":"272751","messageId":"20151102201831.GA10722@sigill.intra.peff.net","threadId":"40710","inReplyTo":"1446492986-32350-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-02T20:18:32Z","receivedAt":"2015-11-02T20:18:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2015 at 02:36:26PM -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 version breaks the range option formatting/setting out to a\n> helper function, as suggested by Junio and Jeff.\n> \n> In addition, it clears the range option when curl slots are cleared\n> before reuse, also as suggested\n\nThanks, this looks much nicer to me.\n\nA few minor comments:\n\n> +static void http_opt_request_remainder(CURL *curl, ssize_t lo)\n\nI notice you used ssize_t here. If we are going to deal with large files, I\nwould think off_t would make more sense (i.e., to allow >2GB on a 32-bit\nsystem).\n\nBut much worse than that, the value we are passing typically comes from\nftell(), which only returns a long. So we're truncated anyway in that\ncase.\n\nI certainly don't think we are making anything _worse_ here; the problem\nis in the existing code. But I don't think ssize_t is making anything\nbetter (it's generally the same size as a long anyway). So I think I'd\nprefer one of the following:\n\n  1. Leave it as \"long\". At least then we are matching ftell(), which is\n     clear (and works fine on 64-bit machines).\n\n  2. Use off_t here instead. It doesn't fix the problem, but at least\n     fixes our one component, so it's working towards a better solution\n     in the long run.\n\n  3. Detect and complain when we overflow the long. Hopefully ftell()\n     returns -1 on a 32-bit system when the file is larger than 2GB, so\n     this Just Works already, and we don't create a broken output.\n\n  4. Fix all of the callers. I suspect this would involve calling\n     fstat(fileno(fh)) to get a real off_t.\n\nOptions (3) and (4) are obviously more work, and I don't necessarily\nexpect you to do them. But I think I'd prefer (2) to what you have now.\nUsing off_t has an issue with being unsigned, but...\n\n> +{\n> +\tchar buf[128];\n> +\tint len = 0;\n> +\n> +\tif (lo >= 0)\n> +\t\tlen += xsnprintf(buf + len, 128 - len, \"%\"PRIiMAX,\n> +\t\t\t\t (intmax_t)lo);\n> +\tlen += xsnprintf(buf + len, 128 - len, \"-\");\n\nI think we could just drop this \"lo >= 0\". Now that there is no \"hi\"\n(which I think is fine), there's no reason to call the function at all\nif you do not have a limit.\n\nAlso, we should prefer \"sizeof(buf)\" to repeating the \"128\", as the two\ngetting out of sync would be disastrous. So altogether something like:\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\nwould be enough, I think.\n\n-Peff\n"},{"id":"272761","messageId":"87mvuwcbql.fsf@igel.home","threadId":"40710","inReplyTo":"20151102201831.GA10722@sigill.intra.peff.net","subject":"Re: [PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2015-11-02T21:50:10Z","receivedAt":"2015-11-02T21:50:10Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   4. Fix all of the callers. I suspect this would involve calling\n>      fstat(fileno(fh)) to get a real off_t.\n\nYou can also use ftello which returns off_t.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"272764","messageId":"20151102221027.GA13996@sigill.intra.peff.net","threadId":"40710","inReplyTo":"87mvuwcbql.fsf@igel.home","subject":"Re: [PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-02T22:10:27Z","receivedAt":"2015-11-02T22:10:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2015 at 10:50:10PM +0100, Andreas Schwab wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >   4. Fix all of the callers. I suspect this would involve calling\n> >      fstat(fileno(fh)) to get a real off_t.\n> \n> You can also use ftello which returns off_t.\n\nThanks, I forgot about that function. That would probably be the sanest\npath forward.\n\nI think it is as simple as this (on top of David's patch):\n\n-- >8 --\nSubject: [PATCH] http: use off_t to store partial file size\n\nWhen we try to resume transfer of a partially-downloaded\nobject or pack, we fopen() the existing file for append,\nthen use ftell() to get the current position. We use a\n\"long\", which can hold only 2GB on a 32-bit system, even\nthough packfiles may be larger than that.\n\nLet's switch to using off_t, which should hold any file size\nour system is capable of storing. We need to use ftello() to\nget the off_t. This is in POSIX and hopefully available\neverywhere; if not, we should be able to wrap it by falling\nback to ftell(), which would presumably return \"-1\" on such\na large file (and we would simply skip resuming in that case).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 5f36846..42f29ce 100644\n--- a/http.c\n+++ b/http.c\n@@ -1205,7 +1205,7 @@ static int http_request(const char *url,\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, result);\n \n \t\tif (target == HTTP_REQUEST_FILE) {\n-\t\t\tlong posn = ftell(result);\n+\t\t\toff_t posn = ftello(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@@ -1581,7 +1581,7 @@ int finish_http_pack_request(struct http_pack_request *preq)\n struct http_pack_request *new_http_pack_request(\n \tstruct packed_git *target, const char *base_url)\n {\n-\tlong prev_posn = 0;\n+\toff_t prev_posn = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct http_pack_request *preq;\n \n@@ -1613,7 +1613,7 @@ struct http_pack_request *new_http_pack_request(\n \t * If there is data present from a previous transfer attempt,\n \t * resume where it left off\n \t */\n-\tprev_posn = ftell(preq->packfile);\n+\tprev_posn = ftello(preq->packfile);\n \tif (prev_posn>0) {\n \t\tif (http_is_verbose)\n \t\t\tfprintf(stderr,\n@@ -1668,7 +1668,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tint prevlocal;\n \tchar prev_buf[PREV_BUF_SIZE];\n \tssize_t prev_read = 0;\n-\tlong prev_posn = 0;\n+\toff_t prev_posn = 0;\n \tstruct http_object_request *freq;\n \n \tfreq = xcalloc(1, sizeof(*freq));\n-- \n2.6.2.627.g377c1c5\n"},{"id":"272768","messageId":"1446504084.4131.33.camel@twopensource.com","threadId":"40710","inReplyTo":"20151102221027.GA13996@sigill.intra.peff.net","subject":"Re: [PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-11-02T22:41:24Z","receivedAt":"2015-11-02T22:41:24Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Mon, 2015-11-02 at 17:10 -0500, Jeff King wrote:\n> On Mon, Nov 02, 2015 at 10:50:10PM +0100, Andreas Schwab wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > >   4. Fix all of the callers. I suspect this would involve calling\n> > >      fstat(fileno(fh)) to get a real off_t.\n> > \n> > You can also use ftello which returns off_t.\n> \n> Thanks, I forgot about that function. That would probably be the sanest\n> path forward.\n> \n> I think it is as simple as this (on top of David's patch):\n> \n> -- >8 --\n> Subject: [PATCH] http: use off_t to store partial file size\n> \n> When we try to resume transfer of a partially-downloaded\n> object or pack, we fopen() the existing file for append,\n> then use ftell() to get the current position. We use a\n> \"long\", which can hold only 2GB on a 32-bit system, even\n> though packfiles may be larger than that.\n> \n> Let's switch to using off_t, which should hold any file size\n> our system is capable of storing. We need to use ftello() to\n> get the off_t. This is in POSIX and hopefully available\n> everywhere; if not, we should be able to wrap it by falling\n> back to ftell(), which would presumably return \"-1\" on such\n> a large file (and we would simply skip resuming in that case).\n\nIt would skip resuming, but would still maybe write to the end of the\nexisting file, right?  So I think we would need to seek to the beginning\nof the file in that case.\n\n(other than that, lgtm)\n"},{"id":"272769","messageId":"20151102224556.GA16387@sigill.intra.peff.net","threadId":"40710","inReplyTo":"1446504084.4131.33.camel@twopensource.com","subject":"Re: [PATCH v3] http.c: use CURLOPT_RANGE for range requests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-02T22:45:57Z","receivedAt":"2015-11-02T22:45:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 02, 2015 at 05:41:24PM -0500, David Turner wrote:\n\n> > Let's switch to using off_t, which should hold any file size\n> > our system is capable of storing. We need to use ftello() to\n> > get the off_t. This is in POSIX and hopefully available\n> > everywhere; if not, we should be able to wrap it by falling\n> > back to ftell(), which would presumably return \"-1\" on such\n> > a large file (and we would simply skip resuming in that case).\n> \n> It would skip resuming, but would still maybe write to the end of the\n> existing file, right?  So I think we would need to seek to the beginning\n> of the file in that case.\n\nOh, you're right. That's no worse than the current behavior, but it's\ndefinitely not ideal. I'd leave that for a follow-on patch that actually\nimplements such an ftello wrapper.\n\n-Peff\n"}]}