git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v3] http.postbuffer: allow full range of ssize_t values
@ 2017-03-31 17:26 David Turner
  2017-03-31 19:51 ` Junio C Hamano
  2017-04-01  6:01 ` Jeff King
  0 siblings, 2 replies; 10+ messages in thread
From: David Turner @ 2017-03-31 17:26 UTC (permalink / raw)
  To: git; +Cc: 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.

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

This version fixes the definition of git_parse_ssize_t to return int.

Sorry for the sloppiness.

 cache.h  |  1 +
 config.c | 17 +++++++++++++++++
 http.c   |  4 ++--
 http.h   |  2 +-
 4 files changed, 21 insertions(+), 3 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..de5b155a4e 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)
+{
+	ssize_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];
-- 
2.11.GIT


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-03-31 17:26 [PATCH v3] http.postbuffer: allow full range of ssize_t values David Turner
@ 2017-03-31 19:51 ` Junio C Hamano
  2017-04-01  6:01 ` Jeff King
  1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2017-03-31 19:51 UTC (permalink / raw)
  To: David Turner; +Cc: git

David Turner <dturner@twosigma.com> writes:

> +static int git_parse_ssize_t(const char *value, ssize_t *ret)
> +{
> +	ssize_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;
> +}
> +

Thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-03-31 17:26 [PATCH v3] http.postbuffer: allow full range of ssize_t values David Turner
  2017-03-31 19:51 ` Junio C Hamano
@ 2017-04-01  6:01 ` Jeff King
  2017-04-01 18:09   ` Junio C Hamano
  2017-04-03 17:03   ` David Turner
  1 sibling, 2 replies; 10+ messages in thread
From: Jeff King @ 2017-04-01  6:01 UTC (permalink / raw)
  To: David Turner; +Cc: git

On Fri, Mar 31, 2017 at 01:26:31PM -0400, 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.

I'm still not sure why a 2GB post-buffer is necessary. It sounds like
something is broken in your setup. Large pushes should be sent chunked.

I know broken setups are a fact of life, but this feels like a really
hacky work-around.

> 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 *);

For most of our other "big" values we use git_config_ulong(). E.g.,
core.bigfilethreshold. I suspect that would be fine for your purposes
here, though using size_t is more correct (on Windows "unsigned long" is
still only 32 bits, even on 64-bit systems).

The ultimate fate of this number, though, is to be handed to:

  curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);

where the final argument is interpreted as a long. So I suspect that on
64-bit Windows, setting http.postbuffer to "3G" would cause some kind of
weird error (either a truncated post or some internal curl error due to
the negative size, depending on how curl handles it). And in that sense
it's worse than the status quo, which rejects this at the config level.

I think a "git_config_long()" would probably do everything correctly.

> +static int git_parse_ssize_t(const char *value, ssize_t *ret)
> +{
> +	ssize_t tmp;
> +	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
> +		return 0;
> +	*ret = tmp;
> +	return 1;
> +}

I saw the earlier iteration used a size_t, but you switched it after the
compiler (rightfully) complained about the signedness. But I'm not sure
why we would want ssize_t here instead of just using git_parse_unsigned().

All of that's moot if we switch to parsing it as a "long" anyway,
though.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-01  6:01 ` Jeff King
@ 2017-04-01 18:09   ` Junio C Hamano
  2017-04-03 17:03   ` David Turner
  1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2017-04-01 18:09 UTC (permalink / raw)
  To: Jeff King; +Cc: David Turner, git

Jeff King <peff@peff.net> writes:

> On Fri, Mar 31, 2017 at 01:26:31PM -0400, 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.
>
> I'm still not sure why a 2GB post-buffer is necessary. It sounds like
> something is broken in your setup. Large pushes should be sent chunked.
>
> I know broken setups are a fact of life, but this feels like a really
> hacky work-around.

Yeah, I tend to share that "Huh? We do not want to do this, but what
forces us to?" puzzlement.

Tentatively demoted to 'pu' (out of 'jch'--candidates to go to 'next').

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-01  6:01 ` Jeff King
  2017-04-01 18:09   ` Junio C Hamano
@ 2017-04-03 17:03   ` David Turner
  2017-04-04  2:01     ` Jeff King
  1 sibling, 1 reply; 10+ messages in thread
From: David Turner @ 2017-04-03 17:03 UTC (permalink / raw)
  To: Jeff King; +Cc: git@vger.kernel.org



> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Saturday, April 1, 2017 2:01 AM
> To: David Turner <David.Turner@twosigma.com>
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
> 
> On Fri, Mar 31, 2017 at 01:26:31PM -0400, 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.
> 
> I'm still not sure why a 2GB post-buffer is necessary. It sounds like something
> is broken in your setup. Large pushes should be sent chunked.
> 
> I know broken setups are a fact of life, but this feels like a really hacky work-
> around.

I'm not sure what other workaround I should use.  I guess I could do multiple pushes, but only if individual objects are under the size limit, and I'm not sure all of mine are (maybe I'll get lucky tho).  I know that this is a configuration issue with gitlab: https://gitlab.com/gitlab-org/gitlab-ce/issues/30315 but I don't know when that will get fixed.  I could manually copy the repo to the server and do a local push, but I don't know that I have the necessary permissions to do that. Or I could do this, which would hopefully actually solve the problem.

> > 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 *);
> 
> For most of our other "big" values we use git_config_ulong(). E.g.,
> core.bigfilethreshold. I suspect that would be fine for your purposes here,
> though using size_t is more correct (on Windows "unsigned long" is still only
> 32 bits, even on 64-bit systems).
> 
> The ultimate fate of this number, though, is to be handed to:
> 
>   curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);
> 
> where the final argument is interpreted as a long. So I suspect that on 64-bit
> Windows, setting http.postbuffer to "3G" would cause some kind of weird
> error (either a truncated post or some internal curl error due to the negative
> size, depending on how curl handles it).

Ah, so we would need to use CURLOPT_POSTFIELDSIZE_LARGE.  Will re-roll.

> 
> I think a "git_config_long()" would probably do everything correctly.
> 
> > +static int git_parse_ssize_t(const char *value, ssize_t *ret) {
> > +	ssize_t tmp;
> > +	if (!git_parse_signed(value, &tmp,
> maximum_signed_value_of_type(ssize_t)))
> > +		return 0;
> > +	*ret = tmp;
> > +	return 1;
> > +}
> 
> I saw the earlier iteration used a size_t, but you switched it after the compiler
> (rightfully) complained about the signedness. But I'm not sure why we would
> want ssize_t here instead of just using git_parse_unsigned().

It was originally signed.  I'm not sure why that was, but I figured it would be simpler to save the extra bit just in case.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-03 17:03   ` David Turner
@ 2017-04-04  2:01     ` Jeff King
  2017-04-04 18:42       ` David Turner
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2017-04-04  2:01 UTC (permalink / raw)
  To: David Turner; +Cc: git@vger.kernel.org

On Mon, Apr 03, 2017 at 05:03:49PM +0000, 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.
> > 
> > I'm still not sure why a 2GB post-buffer is necessary. It sounds like something
> > is broken in your setup. Large pushes should be sent chunked.
> > 
> > I know broken setups are a fact of life, but this feels like a really hacky work-
> > around.
> 
> I'm not sure what other workaround I should use.  I guess I could do
> multiple pushes, but only if individual objects are under the size
> limit, and I'm not sure all of mine are (maybe I'll get lucky tho).  I
> know that this is a configuration issue with gitlab:
> https://gitlab.com/gitlab-org/gitlab-ce/issues/30315 but I don't know
> when that will get fixed.  I could manually copy the repo to the
> server and do a local push, but I don't know that I have the necessary
> permissions to do that. Or I could do this, which would hopefully
> actually solve the problem.

I didn't think we had gotten details on what was actually broken. Is it
really that GitLab does not support chunked transfers? I know that's
what the issue above says, but it sounds like it is just assuming that
is the problem based on the recent messages to the list.

If that's really the issue, then OK. That's lame, but something the
client has to work around. It seems like a pretty big gap, though (and
one I'd think people would have hit before; the default post-buffer is
only 1MB. Surely people routinely push more than that to GitLab servers?
So I'm really wondering if there is something else going on here.

What does it look like when it fails? What does GIT_TRACE_CURL look like
(or GIT_CURL_VERBOSE if your client is older, but remember to sanitize
any auth lines)?

> > The ultimate fate of this number, though, is to be handed to:
> > 
> >   curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);
> > 
> > where the final argument is interpreted as a long. So I suspect that on 64-bit
> > Windows, setting http.postbuffer to "3G" would cause some kind of weird
> > error (either a truncated post or some internal curl error due to the negative
> > size, depending on how curl handles it).
> 
> Ah, so we would need to use CURLOPT_POSTFIELDSIZE_LARGE.  Will re-roll.

Ah, neat. I didn't even know about CURLOPT_POSTFIELDSIZE_LARGE, and
thought we'd have to just limit 32-bit platforms. That's a much better
solution.

> > I saw the earlier iteration used a size_t, but you switched it after the compiler
> > (rightfully) complained about the signedness. But I'm not sure why we would
> > want ssize_t here instead of just using git_parse_unsigned().
> 
> It was originally signed.  I'm not sure why that was, but I figured it
> would be simpler to save the extra bit just in case.

I think it was simply because git_config_int() is the generic "number"
parser, and nobody ever thought about it.

In fact, passing a negative value to curl would be disastrous, as it
would use strlen(). I don't think a negative value could ever get that
far, though. It looks like the config code would silently turn a
negative value into LARGE_PACKET_MAX.

IMHO, complaining about the negative number to the user would be an
improvement.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-04  2:01     ` Jeff King
@ 2017-04-04 18:42       ` David Turner
  2017-04-04 20:40         ` Jeff King
  0 siblings, 1 reply; 10+ messages in thread
From: David Turner @ 2017-04-04 18:42 UTC (permalink / raw)
  To: Jeff King; +Cc: git@vger.kernel.org

> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Monday, April 3, 2017 10:02 PM
> To: David Turner <David.Turner@twosigma.com>
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
> 
> On Mon, Apr 03, 2017 at 05:03:49PM +0000, 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.
> > >
> > > I'm still not sure why a 2GB post-buffer is necessary. It sounds
> > > like something is broken in your setup. Large pushes should be sent
> chunked.
> > >
> > > I know broken setups are a fact of life, but this feels like a
> > > really hacky work- around.
> >
> > I'm not sure what other workaround I should use.  I guess I could do
> > multiple pushes, but only if individual objects are under the size
> > limit, and I'm not sure all of mine are (maybe I'll get lucky tho).  I
> > know that this is a configuration issue with gitlab:
> > https://gitlab.com/gitlab-org/gitlab-ce/issues/30315 but I don't know
> > when that will get fixed.  I could manually copy the repo to the
> > server and do a local push, but I don't know that I have the necessary
> > permissions to do that. Or I could do this, which would hopefully
> > actually solve the problem.
> 
> I didn't think we had gotten details on what was actually broken. Is it really
> that GitLab does not support chunked transfers? I know that's what the issue
> above says, but it sounds like it is just assuming that is the problem based on
> the recent messages to the list.

I agree that we don't know for sure what's actually broken.  I think probably the
GitLab bug tracker might be the better place to figure that out, unless GitLab 
thinks they're hitting a bug in git.

> If that's really the issue, then OK. That's lame, but something the client has to
> work around. It seems like a pretty big gap, though (and one I'd think people
> would have hit before; the default post-buffer is only 1MB. Surely people
> routinely push more than that to GitLab servers?
> So I'm really wondering if there is something else going on here.
> 
> What does it look like when it fails? What does GIT_TRACE_CURL look like (or
> GIT_CURL_VERBOSE if your client is older, but remember to sanitize any auth
> lines)?

Unfortunately, we've already worked around the problem by pushing over SSH, 
so I no longer have a failing case to examine. Last time I tried, I actually did some 
hackery to create a push smaller than 2GB, but it still failed (this time, with 
"502 Bad Gateway").  So, something is clearly weird in GitLab land.

I did see "Transfer-Encoding: chunked" in one of the responses from the server,
 but not in the request (not sure if that's normal). The smaller push had: 
Content-Length: 1048908476

(For me to publish longer log traces requires a level of security review which is 
probably too much of a hassle unless you think it will be really useful).

> > > The ultimate fate of this number, though, is to be handed to:
> > >
> > >   curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, rpc->len);
> > >
> > > where the final argument is interpreted as a long. So I suspect that
> > > on 64-bit Windows, setting http.postbuffer to "3G" would cause some
> > > kind of weird error (either a truncated post or some internal curl
> > > error due to the negative size, depending on how curl handles it).
> >
> > Ah, so we would need to use CURLOPT_POSTFIELDSIZE_LARGE.  Will re-roll.
> 
> Ah, neat. I didn't even know about CURLOPT_POSTFIELDSIZE_LARGE, and
> thought we'd have to just limit 32-bit platforms. That's a much better
> solution.
> 
> > > I saw the earlier iteration used a size_t, but you switched it after
> > > the compiler
> > > (rightfully) complained about the signedness. But I'm not sure why
> > > we would want ssize_t here instead of just using git_parse_unsigned().
> >
> > It was originally signed.  I'm not sure why that was, but I figured it
> > would be simpler to save the extra bit just in case.
> 
> I think it was simply because git_config_int() is the generic "number"
> parser, and nobody ever thought about it.
> 
> In fact, passing a negative value to curl would be disastrous, as it would use
> strlen(). I don't think a negative value could ever get that far, though. It looks
> like the config code would silently turn a negative value into
> LARGE_PACKET_MAX.

I would still prefer to preserve the bit just in case we ever decide that a negative 
value should have some special meaning later.  Of course, that special meaning 
wouldn't be "pass directly to curl".  (As I think about git's http protocol, and how 
hard it is to change it, I always want to leave the maximal number of extra bits 
free possible for general future usage).

> IMHO, complaining about the negative number to the user would be an
> improvement.

That seems reasonable.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-04 18:42       ` David Turner
@ 2017-04-04 20:40         ` Jeff King
  2017-04-06 17:24           ` Christian Couder
  0 siblings, 1 reply; 10+ messages in thread
From: Jeff King @ 2017-04-04 20:40 UTC (permalink / raw)
  To: David Turner; +Cc: git@vger.kernel.org

On Tue, Apr 04, 2017 at 06:42:23PM +0000, David Turner wrote:

> > What does it look like when it fails? What does GIT_TRACE_CURL look like (or
> > GIT_CURL_VERBOSE if your client is older, but remember to sanitize any auth
> > lines)?
> 
> Unfortunately, we've already worked around the problem by pushing over SSH, 
> so I no longer have a failing case to examine. Last time I tried, I actually did some 
> hackery to create a push smaller than 2GB, but it still failed (this time, with 
> "502 Bad Gateway").  So, something is clearly weird in GitLab land.
> 
> I did see "Transfer-Encoding: chunked" in one of the responses from the server,
>  but not in the request (not sure if that's normal). The smaller push had: 
> Content-Length: 1048908476

The 502 makes me think it's a problem in the GitLab reverse-proxy layer
(and also my experience debugging Git-over-HTTP weirdness on GitHub's reverse
proxy layer, which had a number of pitfalls ;) ).

You should be able to do a synthetic test like:

  git init
  dd if=/dev/urandom of=foo.rand bs=1k count=1024
  git add .
  git commit -m 'random megabyte'
  GIT_TRACE_CURL=/tmp/foo.out \
    git -c http.postbuffer=0 push https://...

You should see two POSTs to /git-receive-pack, like this:

  Send header: POST /peff/test.git/git-receive-pack HTTP/1.1
  Send header: Host: github.com
  Send header: Authorization: Basic <redacted>
  Send header: User-Agent: git/2.12.2.952.g759391acc
  Send header: Content-Type: application/x-git-receive-pack-request
  Send header: Accept: application/x-git-receive-pack-result
  Send header: Content-Length: 4

  Send header: POST /peff/test.git/git-receive-pack HTTP/1.1
  Send header: Host: github.com
  Send header: Authorization: Basic <redacted>
  Send header: User-Agent: git/2.12.2.952.g759391acc
  Send header: Accept-Encoding: gzip
  Send header: Content-Type: application/x-git-receive-pack-request
  Send header: Accept: application/x-git-receive-pack-result
  Send header: Transfer-Encoding: chunked

The first is a probe to make sure we can hit the endpoint without
sending the whole payload. And the second should pass up the 1MB
packfile in chunks.

That would at least tell you if the problem is the chunked encoding, or
if it's related to the size.

> (For me to publish longer log traces requires a level of security review which is 
> probably too much of a hassle unless you think it will be really useful).

Nah, I doubt there's much to see except "did a small chunked transfer
work", and anything relevant you can pick out of the server response
(but probably "502" is the extent of it).

> > IMHO, complaining about the negative number to the user would be an
> > improvement.
> 
> That seems reasonable.

You can do that with:

   if (http_post_buffer < 0)
	die("negative http.postBuffer not allowed");

but I was trying to suggest that using git_parse_unsigned() should
detect that error for you. It doesn't seem to, though! The strtoumax()
function happily converts negatives into their twos-complement
wraparounds. We could detect it by looking for a leading "-" ourselves,
though I wonder if anybody is relying on the "-1" behavior.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-04 20:40         ` Jeff King
@ 2017-04-06 17:24           ` Christian Couder
  2017-04-07  4:48             ` Jeff King
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Couder @ 2017-04-06 17:24 UTC (permalink / raw)
  To: Jeff King; +Cc: David Turner, git@vger.kernel.org

On Tue, Apr 4, 2017 at 10:40 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Apr 04, 2017 at 06:42:23PM +0000, David Turner wrote:
>
>> > What does it look like when it fails? What does GIT_TRACE_CURL look like (or
>> > GIT_CURL_VERBOSE if your client is older, but remember to sanitize any auth
>> > lines)?
>>
>> Unfortunately, we've already worked around the problem by pushing over SSH,
>> so I no longer have a failing case to examine. Last time I tried, I actually did some
>> hackery to create a push smaller than 2GB, but it still failed (this time, with
>> "502 Bad Gateway").  So, something is clearly weird in GitLab land.
>>
>> I did see "Transfer-Encoding: chunked" in one of the responses from the server,
>>  but not in the request (not sure if that's normal). The smaller push had:
>> Content-Length: 1048908476
>
> The 502 makes me think it's a problem in the GitLab reverse-proxy layer
> (and also my experience debugging Git-over-HTTP weirdness on GitHub's reverse
> proxy layer, which had a number of pitfalls ;) ).

Yeah, maybe.

> You should be able to do a synthetic test like:
>
>   git init
>   dd if=/dev/urandom of=foo.rand bs=1k count=1024
>   git add .
>   git commit -m 'random megabyte'
>   GIT_TRACE_CURL=/tmp/foo.out \
>     git -c http.postbuffer=0 push https://...
>
> You should see two POSTs to /git-receive-pack, like this:
>
>   Send header: POST /peff/test.git/git-receive-pack HTTP/1.1
>   Send header: Host: github.com
>   Send header: Authorization: Basic <redacted>
>   Send header: User-Agent: git/2.12.2.952.g759391acc
>   Send header: Content-Type: application/x-git-receive-pack-request
>   Send header: Accept: application/x-git-receive-pack-result
>   Send header: Content-Length: 4
>
>   Send header: POST /peff/test.git/git-receive-pack HTTP/1.1
>   Send header: Host: github.com
>   Send header: Authorization: Basic <redacted>
>   Send header: User-Agent: git/2.12.2.952.g759391acc
>   Send header: Accept-Encoding: gzip
>   Send header: Content-Type: application/x-git-receive-pack-request
>   Send header: Accept: application/x-git-receive-pack-result
>   Send header: Transfer-Encoding: chunked
>
> The first is a probe to make sure we can hit the endpoint without
> sending the whole payload. And the second should pass up the 1MB
> packfile in chunks.
>
> That would at least tell you if the problem is the chunked encoding, or
> if it's related to the size.

The above commands work for me using gitlab.com and the log shows:

Send header, 0000000309 bytes (0x00000135)
Send header: POST
/chriscool/yet-another-test-project.git/git-receive-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Send header: User-Agent: git/2.12.2.625.g14da1346c9.dirty
Send header: Host: gitlab.com
Send header: Content-Type: application/x-git-receive-pack-request
Send header: Accept: application/x-git-receive-pack-result
Send header: Content-Length: 4

Send header, 0000000341 bytes (0x00000155)
Send header: POST
/chriscool/yet-another-test-project.git/git-receive-pack HTTP/1.1
Send header: Authorization: Basic <redacted>
Send header: User-Agent: git/2.12.2.625.g14da1346c9.dirty
Send header: Host: gitlab.com
Send header: Accept-Encoding: gzip
Send header: Content-Type: application/x-git-receive-pack-request
Send header: Accept: application/x-git-receive-pack-result
Send header: Transfer-Encoding: chunked

Maybe the reverse proxy doesn't like it when the push is really big.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] http.postbuffer: allow full range of ssize_t values
  2017-04-06 17:24           ` Christian Couder
@ 2017-04-07  4:48             ` Jeff King
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff King @ 2017-04-07  4:48 UTC (permalink / raw)
  To: Christian Couder; +Cc: David Turner, git@vger.kernel.org

On Thu, Apr 06, 2017 at 07:24:54PM +0200, Christian Couder wrote:

> > That would at least tell you if the problem is the chunked encoding, or
> > if it's related to the size.
> 
> The above commands work for me using gitlab.com and the log shows:
> 
> Send header, 0000000309 bytes (0x00000135)
> Send header: POST
> /chriscool/yet-another-test-project.git/git-receive-pack HTTP/1.1
> Send header: Authorization: Basic <redacted>
> Send header: User-Agent: git/2.12.2.625.g14da1346c9.dirty
> Send header: Host: gitlab.com
> Send header: Content-Type: application/x-git-receive-pack-request
> Send header: Accept: application/x-git-receive-pack-result
> Send header: Content-Length: 4
> 
> Send header, 0000000341 bytes (0x00000155)
> Send header: POST
> /chriscool/yet-another-test-project.git/git-receive-pack HTTP/1.1
> Send header: Authorization: Basic <redacted>
> Send header: User-Agent: git/2.12.2.625.g14da1346c9.dirty
> Send header: Host: gitlab.com
> Send header: Accept-Encoding: gzip
> Send header: Content-Type: application/x-git-receive-pack-request
> Send header: Accept: application/x-git-receive-pack-result
> Send header: Transfer-Encoding: chunked
> 
> Maybe the reverse proxy doesn't like it when the push is really big.

Interesting. So it is OK with the chunked encoding. It seems odd that it
would complain about a bigger chunked encoding, but then work correctly
with a single big buffer. But I guess it would all depend on what kind
of buffering logic the reverse proxy uses.

-Peff

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-04-07  4:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-31 17:26 [PATCH v3] http.postbuffer: allow full range of ssize_t values David Turner
2017-03-31 19:51 ` Junio C Hamano
2017-04-01  6:01 ` Jeff King
2017-04-01 18:09   ` Junio C Hamano
2017-04-03 17:03   ` David Turner
2017-04-04  2:01     ` Jeff King
2017-04-04 18:42       ` David Turner
2017-04-04 20:40         ` Jeff King
2017-04-06 17:24           ` Christian Couder
2017-04-07  4:48             ` Jeff King

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).