[PATCH v5] http.postbuffer: allow full range of ssize_t values

[PATCH v5] http.postbuffer: allow full range of ssize_t values

[David Turner]

Unfortunately, in order to push some large repos, the http postbuffer
must sometimes exceed two gigabytes.  On a 64-bit system, this is OK:
we just malloc a larger buffer.

This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
buffer size.

Signed-off-by: David Turner <dturner@twosigma.com>
---

V5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since
I don't have a 32-bit machine handy, it's difficult for me to check
for compiler warnings on 32-bit machines.  Hopefully my guess as
to the solution to Ramsay's issue will be correct.

cache.h       |  1 +
 config.c      | 17 +++++++++++++++++
 http.c        |  4 ++--
 http.h        |  2 +-
 remote-curl.c | 12 +++++++++---
 5 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index fbdf7a815a..5e6747dbb4 100644
--- a/cache.h
+++ b/cache.h
@@ -1900,6 +1900,7 @@ extern int git_parse_maybe_bool(const char *);
 extern int git_config_int(const char *, const char *);
 extern int64_t git_config_int64(const char *, const char *);
 extern unsigned long git_config_ulong(const char *, const char *);
+extern ssize_t git_config_ssize_t(const char *, const char *);
 extern int git_config_bool_or_int(const char *, const char *, int *);
 extern int git_config_bool(const char *, const char *);
 extern int git_config_maybe_bool(const char *, const char *);
diff --git a/config.c b/config.c
index 1a4d85537b..aae6dcc34e 100644
--- a/config.c
+++ b/config.c
@@ -834,6 +834,15 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 	return 1;
 }
 
+static int git_parse_ssize_t(const char *value, ssize_t *ret)
+{
+	intmax_t tmp;
+	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
+		return 0;
+	*ret = tmp;
+	return 1;
+}
+
 NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
@@ -892,6 +901,14 @@ unsigned long git_config_ulong(const char *name, const char *value)
 	return ret;
 }
 
+ssize_t git_config_ssize_t(const char *name, const char *value)
+{
+	ssize_t ret;
+	if (!git_parse_ssize_t(value, &ret))
+		die_bad_number(name, value);
+	return ret;
+}
+
 int git_parse_maybe_bool(const char *value)
 {
 	if (!value)
diff --git a/http.c b/http.c
index 96d84bbed3..22f8167ba2 100644
--- a/http.c
+++ b/http.c
@@ -19,7 +19,7 @@ long int git_curl_ipresolve;
 #endif
 int active_requests;
 int http_is_verbose;
-size_t http_post_buffer = 16 * LARGE_PACKET_MAX;
+ssize_t http_post_buffer = 16 * LARGE_PACKET_MAX;
 
 #if LIBCURL_VERSION_NUM >= 0x070a06
 #define LIBCURL_CAN_HANDLE_AUTH_ANY
@@ -331,7 +331,7 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp("http.postbuffer", var)) {
-		http_post_buffer = git_config_int(var, value);
+		http_post_buffer = git_config_ssize_t(var, value);
 		if (http_post_buffer < LARGE_PACKET_MAX)
 			http_post_buffer = LARGE_PACKET_MAX;
 		return 0;
diff --git a/http.h b/http.h
index 02bccb7b0c..f7bd3b26b0 100644
--- a/http.h
+++ b/http.h
@@ -111,7 +111,7 @@ extern struct curl_slist *http_copy_default_headers(void);
 extern long int git_curl_ipresolve;
 extern int active_requests;
 extern int http_is_verbose;
-extern size_t http_post_buffer;
+extern ssize_t http_post_buffer;
 extern struct credential http_auth;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
diff --git a/remote-curl.c b/remote-curl.c
index e953d06f66..b7b69e096a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -531,6 +531,12 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 	return err;
 }
 
+static curl_off_t xcurl_off_t(ssize_t len) {
+	if (len > (curl_off_t) len)
+		die("Cannot handle pushes this big");
+	return (curl_off_t) len;
+}
+
 static int post_rpc(struct rpc_state *rpc)
 {
 	struct active_request_slot *slot;
@@ -614,7 +620,7 @@ static int post_rpc(struct rpc_state *rpc)
 		 * and we just need to send it.
 		 */
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
-		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));
 
 	} else if (use_gzip && 1024 < rpc->len) {
 		/* The client backend isn't giving us compressed data so
@@ -645,7 +651,7 @@ static int post_rpc(struct rpc_state *rpc)
 
 		headers = curl_slist_append(headers, "Content-Encoding: gzip");
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
-		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, gzip_size);
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));
 
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (gzip %lu to %lu bytes)\n",
@@ -658,7 +664,7 @@ static int post_rpc(struct rpc_state *rpc)
 		 * more normal Content-Length approach.
 		 */
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, rpc->buf);
-		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);
+		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(rpc->len));
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (%lu bytes)\n",
 				rpc->service_name, (unsigned long)rpc->len);
-- 
2.11.GIT

Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values

[Jonathan Nieder]

David Turner wrote:

> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
> buffer size.

Neat.

For completeness, it's useful to know this was added in curl 7.11.1,
which is old enough for us to be able to count on users having it (in
fact it was released >10 years ago).

[...]
> +++ b/remote-curl.c
> @@ -531,6 +531,12 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  	return err;
>  }
>  
> +static curl_off_t xcurl_off_t(ssize_t len) {
> +	if (len > (curl_off_t) len)
> +		die("Cannot handle pushes this big");

nit: other calls to die() here and elsewhere tend to use a lowercase
error message.

More importantly, converting a value to a signed type when the value
cannot be represented in it yields implementation-defined behavior
(C99 section 6.3.1.3 "signed and unsigned integers").  That makes it
fodder for over-eager optimizers.

Would something like the following work?

With that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/remote-curl.c w/remote-curl.c
index b7b69e096a..cf171b1bc9 100644
--- i/remote-curl.c
+++ w/remote-curl.c
@@ -532,8 +532,8 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 }
 
 static curl_off_t xcurl_off_t(ssize_t len) {
-	if (len > (curl_off_t) len)
-		die("Cannot handle pushes this big");
+	if (len > maximum_signed_value_of_type(curl_off_t))
+		die("cannot handle pushes this big");
 	return (curl_off_t) len;
 }
 

Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values

[Jeff King]

On Mon, Apr 03, 2017 at 06:32:10PM -0700, Jonathan Nieder wrote:

> David Turner wrote:
> 
> > This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
> > buffer size.
> 
> Neat.
> 
> For completeness, it's useful to know this was added in curl 7.11.1,
> which is old enough for us to be able to count on users having it (in
> fact it was released >10 years ago).

We have a number of LIBCURL_VERSION_NUM checks that are older than that.
I'm totally OK with declaring a 13-year old version of curl as obsolete,
but we should probably consider dropping some of those ancient ifdefs. I
wouldn't be surprised if some of them are buggy or even fail to compile
at this point.

-Peff

Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values

[Ramsay Jones]

On 04/04/17 00:53, David Turner wrote:
> Unfortunately, in order to push some large repos, the http postbuffer
> must sometimes exceed two gigabytes.  On a 64-bit system, this is OK:
> we just malloc a larger buffer.
> 
> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
> buffer size.
> 
> Signed-off-by: David Turner <dturner@twosigma.com>
> ---
> 
> V5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since
> I don't have a 32-bit machine handy, it's difficult for me to check
> for compiler warnings on 32-bit machines.  Hopefully my guess as
> to the solution to Ramsay's issue will be correct.

Yep, this compiles without complaint.

Thanks!

ATB,
Ramsay Jones

Re: [PATCH v5] http.postbuffer: allow full range of ssize_t values

[Lars Schneider]

> On 04 Apr 2017, at 17:20, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> 
> On 04/04/17 00:53, David Turner wrote:
>> Unfortunately, in order to push some large repos, the http postbuffer
>> must sometimes exceed two gigabytes.  On a 64-bit system, this is OK:
>> we just malloc a larger buffer.
>> 
>> This means that we need to use CURLOPT_POSTFIELDSIZE_LARGE to set the
>> buffer size.
>> 
>> Signed-off-by: David Turner <dturner@twosigma.com>
>> ---
>> 
>> V5 addresses comments from Torsten Boegershausen and Ramsay Jones.  Since
>> I don't have a 32-bit machine handy, it's difficult for me to check
>> for compiler warnings on 32-bit machines.  Hopefully my guess as
>> to the solution to Ramsay's issue will be correct.
> 
> Yep, this compiles without complaint.


Hi David,

You can test the 32-bit build on your machine by ...

1. Pulling this Docker image:
https://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L58

2. Starting the Docker image:
https://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L72

3. Running the linux32 build:
https://github.com/git/git/blob/cf11a67975b057a144618badf16dc4e3d25b9407/.travis.yml#L73


Cheers,
Lars