threads / patch / 45665

v6http.postbuffer: allow full range of ssize_t values

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

## tl;dr

5 messages between Apr 11, 2017 and Apr 12, 2017. Diffs are folded; open one to read it.

replies: 4people: 4as markdown or json

David Turner· Apr 11, 2017, 18:13 UTC · lore

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(-)
Show changes to 5 files +32 −6

cache.h, config.c, http.c, http.h, remote-curl.c

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
Jonathan Nieder· Apr 11, 2017, 18:27 UTC · re: David Turner · lore

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

David Turner wrote:
Show 16 quoted lines
> 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.
Jeff King· Apr 11, 2017, 19:41 UTC · re: Jonathan Nieder · lore

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

On Tue, Apr 11, 2017 at 11:27:40AM -0700, Jonathan Nieder wrote:
Show 25 quoted lines
> 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
Junio C Hamano· Apr 12, 2017, 02:39 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
>> 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.

Show 6 quoted lines
> 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
Jeff King· Apr 12, 2017, 12:52 UTC · re: Junio C Hamano · lore

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

On Tue, Apr 11, 2017 at 07:39:27PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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

← back to recent threads