{"thread":{"id":"45601","subject":"[PATCH v5] http.postbuffer: allow full range of ssize_t values","startedAt":"2017-04-03T23:53:24Z","lastAt":"2017-04-11T11:44:28Z","messageCount":5,"participants":["David Turner","Jonathan Nieder","Jeff King","Ramsay Jones","Lars Schneider"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"316123","messageId":"20170403235308.30102-1-dturner@twosigma.com","threadId":"45601","inReplyTo":null,"subject":"[PATCH v5] http.postbuffer: allow full range of ssize_t values","fromName":"David Turner","fromEmail":"dturner@twosigma.com","sentAt":"2017-04-03T23:53:08Z","receivedAt":"2017-04-03T23:53:24Z","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, the http postbuffer\nmust sometimes exceed two gigabytes.  On a 64-bit system, this is OK:\nwe just malloc a 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\nV5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since\nI don't have a 32-bit machine handy, it's difficult for me to check\nfor compiler warnings on 32-bit machines.  Hopefully my guess as\nto the solution to Ramsay's issue will be correct.\n\ncache.h       |  1 +\n config.c      | 17 +++++++++++++++++\n http.c        |  4 ++--\n http.h        |  2 +-\n remote-curl.c | 12 +++++++++---\n 5 files changed, 30 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..22f8167ba2 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,7 @@ 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 < 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..b7b69e096a 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 > (curl_off_t) len)\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":"316124","messageId":"20170404013210.GD8741@aiede.mtv.corp.google.com","threadId":"45601","inReplyTo":"20170403235308.30102-1-dturner@twosigma.com","subject":"Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-04T01:32:10Z","receivedAt":"2017-04-04T01:32:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Turner wrote:\n\n> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the\n> buffer size.\n\nNeat.\n\nFor completeness, it's useful to know this was added in curl 7.11.1,\nwhich is old enough for us to be able to count on users having it (in\nfact it was released >10 years ago).\n\n[...]\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 > (curl_off_t) len)\n> +\t\tdie(\"Cannot handle pushes this big\");\n\nnit: other calls to die() here and elsewhere tend to use a lowercase\nerror message.\n\nMore importantly, converting a value to a signed type when the value\ncannot be represented in it yields implementation-defined behavior\n(C99 section 6.3.1.3 \"signed and unsigned integers\").  That makes it\nfodder for over-eager optimizers.\n\nWould something like the following work?\n\nWith that change,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\ndiff --git i/remote-curl.c w/remote-curl.c\nindex b7b69e096a..cf171b1bc9 100644\n--- i/remote-curl.c\n+++ w/remote-curl.c\n@@ -532,8 +532,8 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)\n }\n \n static curl_off_t xcurl_off_t(ssize_t len) {\n-\tif (len > (curl_off_t) len)\n-\t\tdie(\"Cannot handle pushes this big\");\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"},{"id":"316129","messageId":"20170404023331.dqdndazmmkb24uvt@sigill.intra.peff.net","threadId":"45601","inReplyTo":"20170404013210.GD8741@aiede.mtv.corp.google.com","subject":"Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-04T02:33:31Z","receivedAt":"2017-04-04T02:33:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 03, 2017 at 06:32:10PM -0700, Jonathan Nieder wrote:\n\n> David Turner wrote:\n> \n> > This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the\n> > buffer size.\n> \n> Neat.\n> \n> For completeness, it's useful to know this was added in curl 7.11.1,\n> which is old enough for us to be able to count on users having it (in\n> fact it was released >10 years ago).\n\nWe have a number of LIBCURL_VERSION_NUM checks that are older than that.\nI'm totally OK with declaring a 13-year old version of curl as obsolete,\nbut we should probably consider dropping some of those ancient ifdefs. I\nwouldn't be surprised if some of them are buggy or even fail to compile\nat this point.\n\n-Peff\n"},{"id":"316159","messageId":"3d351f90-af4f-1ddb-c8a0-3e7926fd722d@ramsayjones.plus.com","threadId":"45601","inReplyTo":"20170403235308.30102-1-dturner@twosigma.com","subject":"Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-04-04T15:20:02Z","receivedAt":"2017-04-04T15:20:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/04/17 00:53, David Turner wrote:\n> Unfortunately, in order to push some large repos, the http postbuffer\n> must sometimes exceed two gigabytes.  On a 64-bit system, this is OK:\n> we just malloc 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> \n> V5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since\n> I don't have a 32-bit machine handy, it's difficult for me to check\n> for compiler warnings on 32-bit machines.  Hopefully my guess as\n> to the solution to Ramsay's issue will be correct.\n\nYep, this compiles without complaint.\n\nThanks!\n\nATB,\nRamsay Jones\n"},{"id":"316586","messageId":"62969B9F-F79E-4CBA-A343-BC8D9CDBADB6@gmail.com","threadId":"45601","inReplyTo":"3d351f90-af4f-1ddb-c8a0-3e7926fd722d@ramsayjones.plus.com","subject":"Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-04-11T11:44:19Z","receivedAt":"2017-04-11T11:44:28Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 04 Apr 2017, at 17:20, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:\n> \n> \n> \n> On 04/04/17 00:53, David Turner wrote:\n>> Unfortunately, in order to push some large repos, the http postbuffer\n>> must sometimes exceed two gigabytes.  On a 64-bit system, this is OK:\n>> we just malloc 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>> \n>> V5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since\n>> I don't have a 32-bit machine handy, it's difficult for me to check\n>> for compiler warnings on 32-bit machines.  Hopefully my guess as\n>> to the solution to Ramsay's issue will be correct.\n> \n> Yep, this compiles without complaint.\n\n\nHi David,\n\nYou can test the 32-bit build on your machine by ...\n\n1. Pulling this Docker image:\nhttps://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L58\n\n2. Starting the Docker image:\nhttps://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L72\n\n3. Running the linux32 build:\nhttps://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L73\n\n\nCheers,\nLars"}]}