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

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

[David Turner]

Unfortunately, in order to push some large repos where a server does
not support chunked encoding, 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>
---
 cache.h       |  1 +
 config.c      | 17 +++++++++++++++++
 http.c        |  6 ++++--
 http.h        |  2 +-
 remote-curl.c | 12 +++++++++---
 5 files changed, 32 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..7bccb36459 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,9 @@ 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 < 0)
+			warning(_("negative value for http.postbuffer; defaulting to %d"), LARGE_PACKET_MAX);
 		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..cf171b1bc9 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 > maximum_signed_value_of_type(curl_off_t))
+		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 v6] http.postbuffer: allow full range of ssize_t values

[Jonathan Nieder]

David Turner wrote:

> Unfortunately, in order to push some large repos where a server does
> not support chunked encoding, 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>
> ---
>  cache.h       |  1 +
>  config.c      | 17 +++++++++++++++++
>  http.c        |  6 ++++--
>  http.h        |  2 +-
>  remote-curl.c | 12 +++++++++---
>  5 files changed, 32 insertions(+), 6 deletions(-)

The only unresolved issue was whether we can count on curl being new
enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say
"unresolved" but it is resolved in my mind since git doesn't build and
pass tests with such old versions of curl --- what's unresolved is
formalizing what the oldest curl version is that we want to support.
And that doesn't need to hold this patch hostage.

So for what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thank you.

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

[Jeff King]

On Tue, Apr 11, 2017 at 11:27:40AM -0700, Jonathan Nieder wrote:

> David Turner wrote:
> 
> > Unfortunately, in order to push some large repos where a server does
> > not support chunked encoding, 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>
> > ---
> >  cache.h       |  1 +
> >  config.c      | 17 +++++++++++++++++
> >  http.c        |  6 ++++--
> >  http.h        |  2 +-
> >  remote-curl.c | 12 +++++++++---
> >  5 files changed, 32 insertions(+), 6 deletions(-)
> 
> The only unresolved issue was whether we can count on curl being new
> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say
> "unresolved" but it is resolved in my mind since git doesn't build and
> pass tests with such old versions of curl --- what's unresolved is
> formalizing what the oldest curl version is that we want to support.
> And that doesn't need to hold this patch hostage.

It could build on older curl with a minor fix; the regression is in
v2.12. So if we did want to continue to support the same versions of
curl we did in v2.11, we could apply that fix and then we _would_ care
about #ifdef-ing this.

That isn't my preferred route; just pointing out that if the "oldest
curl" question isn't settled, that could still be relevant to this
patch. It doesn't have to be held hostage to the fix, but we should be
aware we are digging the hole deeper.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> The only unresolved issue was whether we can count on curl being new
>> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say
>> "unresolved" but it is resolved in my mind since git doesn't build and
>> pass tests with such old versions of curl --- what's unresolved is
>> formalizing what the oldest curl version is that we want to support.
>> And that doesn't need to hold this patch hostage.
>
> It could build on older curl with a minor fix; the regression is in
> v2.12. So if we did want to continue to support the same versions of
> curl we did in v2.11, we could apply that fix and then we _would_ care
> about #ifdef-ing this.

What would the fix be?  Have a code that notices that the value set
to http.postbuffer is too large and ignore the request on the other
side of #ifdef, i.e. when Git is built with older curl that lack
CURLOPT_POSTFIELDSIZE_LARGE?

Something like that may be prudent for the 'maint' track.  But I
tend to agree that for feature releases, we should revisit what the
oldest version we claim to support from time to time and raise the
floor when we notice nobody has even been attempting to build the
other side of the #ifdef (and during that exercise, those who do
want to have older versions supported _can_ argue against removal of
#ifdef with patch to keep both side of #ifdef working).  I fully
agree with what you said earlier that it is irresponsible to the
users to keep #ifdef that gives a false impression that we are
maintaining both sides of them, when in reality the older side has
bit-rotten without anybody even noticing.

I suspect that it is a bit too late for the next release, but we can
decide by mid May for the one after 2.13 if we waned to.

> That isn't my preferred route; just pointing out that if the "oldest
> curl" question isn't settled, that could still be relevant to this
> patch. It doesn't have to be held hostage to the fix, but we should be
> aware we are digging the hole deeper.
>
> -Peff

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

[Jeff King]

On Tue, Apr 11, 2017 at 07:39:27PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> The only unresolved issue was whether we can count on curl being new
> >> enough for CURLOPT_POSTFIELDSIZE_LARGE to be present.  I say
> >> "unresolved" but it is resolved in my mind since git doesn't build and
> >> pass tests with such old versions of curl --- what's unresolved is
> >> formalizing what the oldest curl version is that we want to support.
> >> And that doesn't need to hold this patch hostage.
> >
> > It could build on older curl with a minor fix; the regression is in
> > v2.12. So if we did want to continue to support the same versions of
> > curl we did in v2.11, we could apply that fix and then we _would_ care
> > about #ifdef-ing this.
> 
> What would the fix be?  Have a code that notices that the value set
> to http.postbuffer is too large and ignore the request on the other
> side of #ifdef, i.e. when Git is built with older curl that lack
> CURLOPT_POSTFIELDSIZE_LARGE?

The fix I meant there is for a different spot. During the course of the
discussion, somebody noticed that v2.12 does not compile using older
curls:

  http://public-inbox.org/git/20170404133241.GA15588@gevaerts.be/

So _if_ we care about those older curls, then we should consider that a
regression and fix it on the v2.12-maint track.

And likewise, we should not accept this patch into master without a
similar fix. Which is probably, yes, an ifdef for older curl that uses
the non-LARGE version of POSTFIELDSIZE and defines xcurl_off_t() to
check against "long" and complain when it overflows.

-Peff