{"thread":{"id":"45665","subject":"[PATCH v6] http.postbuffer: allow full range of ssize_t values","startedAt":"2017-04-11T18:14:11Z","lastAt":"2017-04-12T12:52:25Z","messageCount":5,"participants":["David Turner","Jonathan Nieder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"316617","messageId":"20170411181357.16580-1-dturner@twosigma.com","threadId":"45665","inReplyTo":null,"subject":"[PATCH v6] http.postbuffer: allow full range of ssize_t values","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-11T18:13:57Z","receivedAt":"2017-04-11T18:14:11Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Unfortunately, in order to push some large repos where a server does\nnot support chunked encoding, the http postbuffer must sometimes\nexceed two gigabytes.  On a 64-bit system, this is OK: we just malloc\na larger buffer.\n\nThis means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the\nbuffer size.\n\nSigned-off-by: David Turner <dturner@twosigma.com>\n---\n cache.h       |  1 +\n config.c      | 17 +++++++++++++++++\n http.c        |  6 ++++--\n http.h        |  2 +-\n remote-curl.c | 12 +++++++++---\n 5 files changed, 32 insertions(+), 6 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex fbdf7a815a..5e6747dbb4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *);\n extern int git_config_int(const char *, const char *);\n extern int64_t git_config_int64(const char *, const char *);\n extern unsigned long git_config_ulong(const char *, const char *);\n+extern ssize_t git_config_ssize_t(const char *, const char *);\n extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_maybe_bool(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 1a4d85537b..aae6dcc34e 100644\n--- a/config.c\n+++ b/config.c\n@@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n+static int git_parse_ssize_t(const char *value, ssize_t *ret)\n+{\n+\tintmax_t tmp;\n+\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\n+\t\treturn 0;\n+\t*ret = tmp;\n+\treturn 1;\n+}\n+\n NORETURN\n static void die_bad_number(const char *name, const char *value)\n {\n@@ -892,6 +901,14 @@ unsigned long git_config_ulong(const char *name, const char *value)\n \treturn ret;\n }\n \n+ssize_t git_config_ssize_t(const char *name, const char *value)\n+{\n+\tssize_t ret;\n+\tif (!git_parse_ssize_t(value, &ret))\n+\t\tdie_bad_number(name, value);\n+\treturn ret;\n+}\n+\n int git_parse_maybe_bool(const char *value)\n {\n \tif (!value)\ndiff --git a/http.c b/http.c\nindex 96d84bbed3..7bccb36459 100644\n--- a/http.c\n+++ b/http.c\n@@ -19,7 +19,7 @@ long int git_curl_ipresolve;\n #endif\n int active_requests;\n int http_is_verbose;\n-size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n+ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n \n #if LIBCURL_VERSION_NUM >= 0x070a06\n #define LIBCURL_CAN_HANDLE_AUTH_ANY\n@@ -331,7 +331,9 @@ static int http_options(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(\"http.postbuffer\", var)) {\n-\t\thttp_post_buffer = git_config_int(var, value);\n+\t\thttp_post_buffer = git_config_ssize_t(var, value);\n+\t\tif (http_post_buffer < 0)\n+\t\t\twarning(_(\"negative value for http.postbuffer; defaulting to %d\"), LARGE_PACKET_MAX);\n \t\tif (http_post_buffer < LARGE_PACKET_MAX)\n \t\t\thttp_post_buffer = LARGE_PACKET_MAX;\n \t\treturn 0;\ndiff --git a/http.h b/http.h\nindex 02bccb7b0c..f7bd3b26b0 100644\n--- a/http.h\n+++ b/http.h\n@@ -111,7 +111,7 @@ extern struct curl_slist *http_copy_default_headers(void);\n extern long int git_curl_ipresolve;\n extern int active_requests;\n extern int http_is_verbose;\n-extern size_t http_post_buffer;\n+extern ssize_t http_post_buffer;\n extern struct credential http_auth;\n \n extern char curl_errorstr[CURL_ERROR_SIZE];\ndiff --git a/remote-curl.c b/remote-curl.c\nindex e953d06f66..cf171b1bc9 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -531,6 +531,12 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n \treturn err;\n }\n \n+static curl_off_t xcurl_off_t(ssize_t len) {\n+\tif (len > maximum_signed_value_of_type(curl_off_t))\n+\t\tdie(\"cannot handle pushes this big\");\n+\treturn (curl_off_t) len;\n+}\n+\n static int post_rpc(struct rpc_state *rpc)\n {\n \tstruct active_request_slot *slot;\n@@ -614,7 +620,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t * and we just need to send it.\n \t\t */\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));\n \n \t} else if (use_gzip && 1024 < rpc->len) {\n \t\t/* The client backend isn't giving us compressed data so\n@@ -645,7 +651,7 @@ static int post_rpc(struct rpc_state *rpc)\n \n \t\theaders = curl_slist_append(headers, \"Content-Encoding: gzip\");\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));\n \n \t\tif (options.verbosity > 1) {\n \t\t\tfprintf(stderr, \"POST %s (gzip %lu to %lu bytes)\\n\",\n@@ -658,7 +664,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t * more normal Content-Length approach.\n \t\t */\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, rpc->buf);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(rpc->len));\n \t\tif (options.verbosity > 1) {\n \t\t\tfprintf(stderr, \"POST %s (%lu bytes)\\n\",\n \t\t\t\trpc->service_name, (unsigned long)rpc->len);\n-- \n2.11.GIT\n\n"},{"id":"316618","messageId":"20170411182740.GO8741@aiede.mtv.corp.google.com","threadId":"45665","inReplyTo":"20170411181357.16580-1-dturner@twosigma.com","subject":"Re: [PATCH v6] http.postbuffer: allow full range of ssize_t values","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-11T18:27:40Z","receivedAt":"2017-04-11T18:27:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Turner wrote:\n\n> Unfortunately, in order to push some large repos where a server does\n> not support chunked encoding, the http postbuffer must sometimes\n> exceed two gigabytes.  On a 64-bit system, this is OK: we just malloc\n> a larger buffer.\n>\n> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the\n> buffer size.\n>\n> Signed-off-by: David Turner <dturner@twosigma.com>\n> ---\n>  cache.h       |  1 +\n>  config.c      | 17 +++++++++++++++++\n>  http.c        |  6 ++++--\n>  http.h        |  2 +-\n>  remote-curl.c | 12 +++++++++---\n>  5 files changed, 32 insertions(+), 6 deletions(-)\n\nThe only unresolved issue was whether we can count on curl being new\nenough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say\n\"unresolved\" but it is resolved in my mind since git doesn't build and\npass tests with such old versions of curl --- what's unresolved is\nformalizing what the oldest curl version is that we want to support.\nAnd that doesn't need to hold this patch hostage.\n\nSo for what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThank you.\n"},{"id":"316627","messageId":"20170411194127.cfy2omkdwhbtkn63@sigill.intra.peff.net","threadId":"45665","inReplyTo":"20170411182740.GO8741@aiede.mtv.corp.google.com","subject":"Re: [PATCH v6] http.postbuffer: allow full range of ssize_t values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-11T19:41:27Z","receivedAt":"2017-04-11T19:41:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 11:27:40AM -0700, Jonathan Nieder wrote:\n\n> David Turner wrote:\n> \n> > Unfortunately, in order to push some large repos where a server does\n> > not support chunked encoding, the http postbuffer must sometimes\n> > exceed two gigabytes.  On a 64-bit system, this is OK: we just malloc\n> > a larger buffer.\n> >\n> > This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the\n> > buffer size.\n> >\n> > Signed-off-by: David Turner <dturner@twosigma.com>\n> > ---\n> >  cache.h       |  1 +\n> >  config.c      | 17 +++++++++++++++++\n> >  http.c        |  6 ++++--\n> >  http.h        |  2 +-\n> >  remote-curl.c | 12 +++++++++---\n> >  5 files changed, 32 insertions(+), 6 deletions(-)\n> \n> The only unresolved issue was whether we can count on curl being new\n> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say\n> \"unresolved\" but it is resolved in my mind since git doesn't build and\n> pass tests with such old versions of curl --- what's unresolved is\n> formalizing what the oldest curl version is that we want to support.\n> And that doesn't need to hold this patch hostage.\n\nIt could build on older curl with a minor fix; the regression is in\nv2.12. So if we did want to continue to support the same versions of\ncurl we did in v2.11, we could apply that fix and then we _would_ care\nabout #ifdef-ing this.\n\nThat isn't my preferred route; just pointing out that if the \"oldest\ncurl\" question isn't settled, that could still be relevant to this\npatch. It doesn't have to be held hostage to the fix, but we should be\naware we are digging the hole deeper.\n\n-Peff\n"},{"id":"316665","messageId":"xmqqa87mqrhc.fsf@gitster.mtv.corp.google.com","threadId":"45665","inReplyTo":"20170411194127.cfy2omkdwhbtkn63@sigill.intra.peff.net","subject":"Re: [PATCH v6] http.postbuffer: allow full range of ssize_t values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-12T02:39:27Z","receivedAt":"2017-04-12T02:39:37Z","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>> The only unresolved issue was whether we can count on curl being new\n>> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say\n>> \"unresolved\" but it is resolved in my mind since git doesn't build and\n>> pass tests with such old versions of curl --- what's unresolved is\n>> formalizing what the oldest curl version is that we want to support.\n>> And that doesn't need to hold this patch hostage.\n>\n> It could build on older curl with a minor fix; the regression is in\n> v2.12. So if we did want to continue to support the same versions of\n> curl we did in v2.11, we could apply that fix and then we _would_ care\n> about #ifdef-ing this.\n\nWhat would the fix be?  Have a code that notices that the value set\nto http.postbuffer is too large and ignore the request on the other\nside of #ifdef, i.e. when Git is built with older curl that lack\nCURLOPT_POSTFIELDSIZE_LARGE?\n\nSomething like that may be prudent for the 'maint' track.  But I\ntend to agree that for feature releases, we should revisit what the\noldest version we claim to support from time to time and raise the\nfloor when we notice nobody has even been attempting to build the\nother side of the #ifdef (and during that exercise, those who do\nwant to have older versions supported _can_ argue against removal of\n#ifdef with patch to keep both side of #ifdef working).  I fully\nagree with what you said earlier that it is irresponsible to the\nusers to keep #ifdef that gives a false impression that we are\nmaintaining both sides of them, when in reality the older side has\nbit-rotten without anybody even noticing.\n\nI suspect that it is a bit too late for the next release, but we can\ndecide by mid May for the one after 2.13 if we waned to.\n\n> That isn't my preferred route; just pointing out that if the \"oldest\n> curl\" question isn't settled, that could still be relevant to this\n> patch. It doesn't have to be held hostage to the fix, but we should be\n> aware we are digging the hole deeper.\n>\n> -Peff\n"},{"id":"316681","messageId":"20170412125214.uiex4ludmgahsvme@sigill.intra.peff.net","threadId":"45665","inReplyTo":"xmqqa87mqrhc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6] http.postbuffer: allow full range of ssize_t values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-12T12:52:14Z","receivedAt":"2017-04-12T12:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 11, 2017 at 07:39:27PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> The only unresolved issue was whether we can count on curl being new\n> >> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say\n> >> \"unresolved\" but it is resolved in my mind since git doesn't build and\n> >> pass tests with such old versions of curl --- what's unresolved is\n> >> formalizing what the oldest curl version is that we want to support.\n> >> And that doesn't need to hold this patch hostage.\n> >\n> > It could build on older curl with a minor fix; the regression is in\n> > v2.12. So if we did want to continue to support the same versions of\n> > curl we did in v2.11, we could apply that fix and then we _would_ care\n> > about #ifdef-ing this.\n> \n> What would the fix be?  Have a code that notices that the value set\n> to http.postbuffer is too large and ignore the request on the other\n> side of #ifdef, i.e. when Git is built with older curl that lack\n> CURLOPT_POSTFIELDSIZE_LARGE?\n\nThe fix I meant there is for a different spot. During the course of the\ndiscussion, somebody noticed that v2.12 does not compile using older\ncurls:\n\n  http://public-inbox.org/git/20170404133241.GA15588@gevaerts.be/\n\nSo _if_ we care about those older curls, then we should consider that a\nregression and fix it on the v2.12-maint track.\n\nAnd likewise, we should not accept this patch into master without a\nsimilar fix. Which is probably, yes, an ifdef for older curl that uses\nthe non-LARGE version of POSTFIELDSIZE and defines xcurl_off_t() to\ncheck against \"long\" and complain when it overflows.\n\n-Peff\n"}]}