Clone hangs when done over http with --reference

Clone hangs when done over http with --reference

[Konstantin Ryabitsev]

[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]

I have a reproducible case where git clone --reference hangs when
performed over http://, but not when performed over git://

The repository in question is very large, which possibly plays a role in
this. Unfortunately, I was not able to reproduce this with any other
repository, so if anyone wants to try this, they will have to suffer
through 2.5GB downloads.

To reproduce on the client:

git clone --mirror \
http://source.codeaurora.org/mirrors/chromium.googlesource.com/chromium/src
(completes after downloading ~2.5GB)

git clone --reference ./src.git --mirror \
http://source.codeaurora.org/quic/chrome4sdp/chromium/src.git foo.git
(this hangs forever)

If you do this over git:// protocol, it will work:

git clone --reference ./src.git --mirror \
git://source.codeaurora.org/quic/chrome4sdp/chromium/src.git foo.git
(completes after downloading ~100MB)

It will also work if you clone the same repo without --reference. Both
repositories pass fsck checks.

I reproduced it with git-2.4.0 on the server, and it appears to be
unrelated to the Apache/Nginx versions nor various httpd daemon
settings, at least not in my testing. No errors are generated in the
logs. The process just appears to be stuck not doing anything.

To reproduce locally, simply set up these two repositories in
/var/lib/git (one can be a --reference clone of the other -- it didn't
matter in my tests), and put this in /etc/httpd/conf.d/git.conf,
assuming Fedora or Centos7 system:

SetEnv GIT_PROJECT_ROOT /var/lib/git
SetEnv GIT_HTTP_EXPORT_ALL 1

AliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$             /var/lib/git/$1
AliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$    /var/lib/git/$1

ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/

<Directory "/var/lib/git">
    AllowOverride None
    Options None
    Require all granted
</Directory>
<Directory "/usr/libexec/git-core">
    AllowOverride None
    Options None
    Require all granted
</Directory>

Not sure what is going on, but it appears that the hang is on the
server. Hope someone can figure it out.

Best,
-- 
Konstantin Ryabitsev
Linux Foundation Collab Projects
Montréal, Québec

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: Clone hangs when done over http with --reference

[Jeff King]

On Wed, May 13, 2015 at 05:04:36PM -0400, Konstantin Ryabitsev wrote:

> I have a reproducible case where git clone --reference hangs when
> performed over http://, but not when performed over git://

Thanks for giving us a reproduction recipe. I was able to recreate the
problem on my machine.

There's a minor bug in git's error reporting that makes this a little
harder to examine, but isn't the root cause. I'll send a patch for that
momentarily.  But here's what I've found.

During the ref negotiation between the client and the server, there's so
much data that we end up making several POSTs to the server. In one of
them, we end up in a deadlock situation between the CGI and Apache. You
can get a full strace of the Apache side by cloning this:

  git://github.com/gist/9daf49aaaff16b4436f2

The interesting part is at 19:23:48.182380. Process 19891 is Apache,
reading from the client (on fd 8) and relaying the data to http-backend
over fd 10:

  19891 19:23:48.182331 read(8,  <unfinished ...>
  19891 19:23:48.182358 <... read resumed> "\320#\200*:x\26\355;t\205\36\v\361'}\304\24x\2502\327\247O\203\255\211\21e\324[_"..., 8000) = 5020
  19891 19:23:48.182365 write(10, "dy\206\327\256\311\230D\236\256\366\3639;\10\356\343%9R\3420\203~\200\256\276\250\332g7\331"..., 236 <unfinished ...>
  19891 19:23:48.182373 <... write resumed> ) = -1 EAGAIN (Resource temporarily unavailable)
  19891 19:23:48.182380 poll([{fd=10, events=POLLOUT}], 1, 60000 <unfinished ...>
  19891 19:24:48.242493 <... poll resumed> ) = 0 (Timeout)
  19891 19:24:48.242609 close(10)         = 0

During one of the writes we get EAGAIN; the buffer to http-backend is
full. So Apache calls poll() to give it 60 seconds. The http-backend
process never starts reading, so Apache gives up and closes the
descriptor, truncating the input to http-backend.

So what's http-backend doing? It's pid 2361 in this output (which you
can verify looking further back in the strace dump and finding the
clone/exec). It's busily relaying all the data to its child,
upload-pack:

  2361  19:23:48.182232 read(0,  <unfinished ...>
  2361  19:23:48.182246 <... read resumed> "\371r\324\366\3600\247\236\227\216i\376P\204R\32Op;\362\247\3573\227\2174\213\361k\5\300\\"..., 8192) = 8192
  2361  19:23:48.182324 write(4, "3435b8c152bec0c78998ff0445bd2c0f"..., 8192 <unfinished ...>
  2361  19:24:48.247291 <... write resumed> ) = 8192

but notice the minute-long jump in that write. It's not reading from
stdin because it's _also_ blocked on writing. So why isn't upload-pack
reading?

  2363  19:23:48.184370 read(0, "have 4ed051beb0ff979344c541ec659"..., 46) = 46
  2363  19:23:48.184381 alarm(0)          = 0
  2363  19:23:48.184398 write(1, "0038ACK 4ed051beb0ff979344c541ec"..., 56 <unfinished ...>
  2363  19:24:48.243565 <... write resumed> ) = 56

It's writing responses to its own stdout, of course! Which also blocks,
preventing us from reading during the crucial minute.  Now who's
supposed to be reading our stdout here?

It's rather difficult to tell from the strace output, as you have to
walk through the parents to see if anybody re-opened stdout over a pipe.
But the last one is in process 2361, just before we exec
git-http-backend. Apache has opened a pipe for the CGI stdout, and is
holding the other end of the pipe. But it's not reading, because it's
blocked trying to write the request body to us.

At 19:24:48, Apache gives up on writing and truncates the input to the
CGI. It starts reading the output, which lets Git start moving again.
But we hit this code:

  ssize_t n = xread(0, in_buf, sizeof(in_buf));
  if (n <= 0)
	die("request ended in the middle of the gzip stream");

because of the truncated input. At this point, everything is done on the
server. Presumably Apache sends us a reasonable content-length, so even
though our output isn't complete, we at least know when it ends. You can
see that in the client git-remote-http here (note this is from a
different run than above, so the timestamps are not the same; this is
a bit after the minute-long hang has completed):

  4453  20:19:32.557686 recvfrom(5,  <unfinished ...>
  4453  20:19:32.557695 <... recvfrom resumed> "4cea692129be9c003c33 common\n\r\n38"..., 16384, 0, NULL, NULL) = 9025
  4453  20:19:32.557703 write(6, "4cea692129be9c003c33 common\n", 28 <unfinished ...>
  4453  20:19:32.557714 <... write resumed> ) = 28 

This is the last bit of data we read from the server, so presumably we
believe we got it all. We relay it all to fd 6, which goes to our
git-fetch-pack sub-process. It in turn reads everything we give it, and
then waits for more. But we don't have any more. The two client
processes are deadlocked, each waiting for more data from the other. The
remote-http process has no way to tell fetch-pack "the server didn't
send any more data". It can't just close() the descriptor, because it's
expecting fetch-pack to tell it to make another POST request to the
server. And fetch-pack is waiting for more input, because what came from
the server was not enough to proceed.

This is a non-ideal way to handle the error, of course. But it's not the
fundamental problem; the best we could do is notice and say "hey, the
server didn't send us enough data".

The fundamental problem is the deadlock on the server side, which is
producing bogus protocol output. And that's a mismatch between what
Apache expects (that the CGI will read all of the input request and then
generate an output request) and what the CGI wants to do (stream output
as it reads the input).

I don't know if there's a way to convince Apache to be more interactive.
As a hacky workaround, we could basically spool all of the input into
memory (or a tempfile) and work from that. Or the output. Either way
would break the pipe deadlock. But we'd have to be sensitive to the type
of request (it's probably OK to spool ref negotiation, but not OK to
spool packfiles, which can be arbitrarily big).

The (horrible, should-not-be-applied) patch below makes your case work
for me:

diff --git a/http-backend.c b/http-backend.c
index 0670719..25471f6 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -269,21 +269,20 @@ static struct rpc_service *select_service(const char *name)
 static void inflate_request(const char *prog_name, int out)
 {
 	git_zstream stream;
-	unsigned char in_buf[8192];
+	struct strbuf in = STRBUF_INIT;
 	unsigned char out_buf[8192];
 	unsigned long cnt = 0;
 
 	memset(&stream, 0, sizeof(stream));
 	git_inflate_init_gzip_only(&stream);
 
-	while (1) {
-		ssize_t n = xread(0, in_buf, sizeof(in_buf));
-		if (n <= 0)
-			die("request ended in the middle of the gzip stream");
-
-		stream.next_in = in_buf;
-		stream.avail_in = n;
+	if (strbuf_read(&in, 0, 8192) < 0)
+		die("unable to read request");
+	stream.next_in = (unsigned char *)in.buf;
+	stream.avail_in = in.len;
 
+	{
+		unsigned long n;
 		while (0 < stream.avail_in) {
 			int ret;
 
@@ -304,6 +303,8 @@ static void inflate_request(const char *prog_name, int out)
 		}
 	}
 
+	die("request ended in the middle of the gzip stream");
+
 done:
 	git_inflate_end(&stream);
 	close(out);

[PATCH] http-backend: fix die recursion with custom handler

[Jeff King]

On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:

> There's a minor bug in git's error reporting that makes this a little
> harder to examine, but isn't the root cause. I'll send a patch for that
> momentarily.  But here's what I've found.

Here's that patch. You may have seen "recursion detected in die handler"
in your apache logs.  Basically we die(), try to write an HTTP error
response, and then die() trying to write it again. It happens reliably
here because this particular error happens _after_ we've already written
out the normal response header and closed stdout. So any die() we
encounter after that is going to try to write its own error header, and
will fail because of the closed stdout.

One way of avoiding this would obviously be to notice in the die()
handler that we have already written our header and closed stdout. That
would help this case, but it would not help any other case where writing
fails unexpectedly. So I'd rather solve the general recursion problem,
which covers all cases.

-- >8 --
Subject: http-backend: fix die recursion with custom handler

When we die() in http-backend, we call a custom handler that
writes an HTTP 500 response to stdout, then reports the
error to stderr. Our routines for writing out the HTTP
response may themselves die, leading to us entering die()
again.

When it was originally written, that was OK; our custom
handler keeps a variable to notice this and does not recurse
indefinitely. However, since cd163d4 (usage.c: detect
recursion in die routines and bail out immediately,
2012-11-14), the main die() implementation detects recursion
before we even get to our custom handler, and bails without
printing anything useful.

We can handle this case by doing two things:

  1. Installing a custom die_is_recursing handler that
     allows us to enter up to one level of recursion. Only
     the first call to our custom handler will try to write
     out the error response. So if we die again, that is OK.
     If we end up dying more than that, it is a sign that we
     have a bug and are in an infinite recursion (i.e., what
     cd163d4 was designed to protect against).

  2. Reporting the error to stderr before trying to write
     out the HTTP response. In the current code, if we do
     die() trying to write out the response, we'll exit
     immediately from this second die(), and never get a
     chance to output the original error (which is almost
     certainly the more interesting one; the second die is
     just going to be along the lines of "I tried to write
     to stdout but it was closed").

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index b6c0484..c210d4d 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -500,21 +500,25 @@ static void service_rpc(char *service_name)
 	strbuf_release(&buf);
 }
 
+static int dead;
 static NORETURN void die_webcgi(const char *err, va_list params)
 {
-	static int dead;
+	if (dead <= 1) {
+		vreportf("fatal: ", err, params);
 
-	if (!dead) {
-		dead = 1;
 		http_status(500, "Internal Server Error");
 		hdr_nocache();
 		end_headers();
-
-		vreportf("fatal: ", err, params);
 	}
 	exit(0); /* we successfully reported a failure ;-) */
 }
 
+static int die_webcgi_recursing(void)
+{
+	dead++;
+	return dead > 1;
+}
+
 static char* getdir(void)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -569,6 +573,7 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 	set_die_routine(die_webcgi);
+	set_die_is_recursing_routine(die_webcgi_recursing);
 
 	if (!method)
 		die("No REQUEST_METHOD from server");
-- 
2.4.0.327.ge28c153

Re: Clone hangs when done over http with --reference

[Konstantin Ryabitsev]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On 13/05/15 08:47 PM, Jeff King wrote:
> I don't know if there's a way to convince Apache to be more interactive.
> As a hacky workaround, we could basically spool all of the input into
> memory (or a tempfile) and work from that. Or the output. Either way
> would break the pipe deadlock. But we'd have to be sensitive to the type
> of request (it's probably OK to spool ref negotiation, but not OK to
> spool packfiles, which can be arbitrarily big).

Thanks for looking into this, Jeff.

Two questions:

1. Does this mean there are potential problems with other git operations
that involve ref negotiation, not just when doing git clone --reference?
Is there a chance to run in to this deadlock by doing an operation like
"git remote update"?

2. If we configure the webserver to serve some files directly, without
passing them to http-backend, e.g. doing the recommended apache magic:

> AliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$          /var/lib/git/$1
> AliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$ /var/lib/git/$1
> AliasMatch ^/git/(.*/refs/heads/.*)$                             /var/lib/git/$1

Will that make the spooling less of a problem, since it won't involve
the super-huge files?

Best,
-Konstantin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 538 bytes --]

Re: Clone hangs when done over http with --reference

[Jeff King]

On Thu, May 14, 2015 at 01:08:46PM -0400, Konstantin Ryabitsev wrote:

> Two questions:
> 
> 1. Does this mean there are potential problems with other git operations
> that involve ref negotiation, not just when doing git clone --reference?
> Is there a chance to run in to this deadlock by doing an operation like
> "git remote update"?

Yes. From the server's perspective, a "clone --reference" is really no
different than a fetch in which the client happened to have all of those
refs already. I didn't try it, but you should be able to reproduce the
problem with:

  cd first-repo.git
  git fetch https://.../second-repo.git refs/*:refs/remotes/foo/*

which should have to do the exact same ref negotiation ("I have these
commits, I want these other commits").

> 2. If we configure the webserver to serve some files directly, without
> passing them to http-backend, e.g. doing the recommended apache magic:
> 
> > AliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$          /var/lib/git/$1
> > AliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$ /var/lib/git/$1
> > AliasMatch ^/git/(.*/refs/heads/.*)$                             /var/lib/git/$1
> 
> Will that make the spooling less of a problem, since it won't involve
> the super-huge files?

No, that won't help. Once git is doing the smart protocol, it will never
ask for arbitrary files. So you would have to disable smart-http
entirely, which I don't recommend.

Besides which, it's not the size of the objects or packs that is an
issue here. It's the relationship of the tips in the second repo to the
tips in the first. That is, the "big" data here is the client and server
finding the common commits between the two (and it's not even _that_
big; it's just big by "stuffing into a pipe buffer" standards; as you
noticed, the git protocol handles it just fine).

-Peff

Re: [PATCH] http-backend: fix die recursion with custom handler

[Jeff King]

On Wed, May 13, 2015 at 09:02:33PM -0400, Jeff King wrote:

> +static int die_webcgi_recursing(void)
> +{
> +	dead++;
> +	return dead > 1;
> +}

Ugh, somehow I managed to introduce an off-by-one here while prettifying
the code before sending. It should be "> 2" (or "dead++ > 1" to get the
value before incrementing).

The series I'll post in a moment will have a fixed version.

-Peff

[PATCH 0/2] fix http deadlock on giant ref negotiations

[Jeff King]

On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:

> The fundamental problem is the deadlock on the server side, which is
> producing bogus protocol output. And that's a mismatch between what
> Apache expects (that the CGI will read all of the input request and then
> generate an output request) and what the CGI wants to do (stream output
> as it reads the input).

At first I was irritated with Apache for this. But thinking on it more,
it's really due to our shoe-horning of a full-duplex protocol into the
half-duplex HTTP protocol. Even if we could convince Apache to work in a
full-duplex way here, and even if our client is full-duplex (since
otherwise we are just trading pipe buffers for TCP buffers), we still
may face arbitrary HTTP proxies or other infrastructure in the middle.

So here's a series to try to address the issue. The first patch is a
fixed version of the die-recursion fixup I posted earlier. The second is
the interesting one.

  [1/2]: http-backend: fix die recursion with custom handler
  [2/2]: http-backend: spool ref negotiation requests to buffer

I have no clue how to write a test that would trigger this reliably
without requiring a gigantic test fixture. However, I did confirm that
it fixes the problem on the chromium case you provided (which otherwise
deadlocks reliably for me).

-Peff

[PATCH 1/2] http-backend: fix die recursion with custom handler

[Jeff King]

When we die() in http-backend, we call a custom handler that
writes an HTTP 500 response to stdout, then reports the
error to stderr. Our routines for writing out the HTTP
response may themselves die, leading to us entering die()
again.

When it was originally written, that was OK; our custom
handler keeps a variable to notice this and does not
recurse. However, since cd163d4 (usage.c: detect recursion
in die routines and bail out immediately, 2012-11-14), the
main die() implementation detects recursion before we even
get to our custom handler, and bails without printing
anything useful.

We can handle this case by doing two things:

  1. Installing a custom die_is_recursing handler that
     allows us to enter up to one level of recursion. Only
     the first call to our custom handler will try to write
     out the error response. So if we die again, that is OK.
     If we end up dying more than that, it is a sign that we
     are in an infinite recursion.

  2. Reporting the error to stderr before trying to write
     out the HTTP response. In the current code, if we do
     die() trying to write out the response, we'll exit
     immediately from this second die(), and never get a
     chance to output the original error (which is almost
     certainly the more interesting one; the second die is
     just going to be along the lines of "I tried to write
     to stdout but it was closed").

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index b6c0484..3ad82a8 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -500,21 +500,24 @@ static void service_rpc(char *service_name)
 	strbuf_release(&buf);
 }
 
+static int dead;
 static NORETURN void die_webcgi(const char *err, va_list params)
 {
-	static int dead;
+	if (dead <= 1) {
+		vreportf("fatal: ", err, params);
 
-	if (!dead) {
-		dead = 1;
 		http_status(500, "Internal Server Error");
 		hdr_nocache();
 		end_headers();
-
-		vreportf("fatal: ", err, params);
 	}
 	exit(0); /* we successfully reported a failure ;-) */
 }
 
+static int die_webcgi_recursing(void)
+{
+	return dead++ > 1;
+}
+
 static char* getdir(void)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -569,6 +572,7 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 	set_die_routine(die_webcgi);
+	set_die_is_recursing_routine(die_webcgi_recursing);
 
 	if (!method)
 		die("No REQUEST_METHOD from server");
-- 
2.4.1.396.g7ba6d7b

[PATCH 2/2] http-backend: spool ref negotiation requests to buffer

[Jeff King]

When http-backend spawns "upload-pack" to do ref
negotiation, it streams the http request body to
upload-pack, who then streams the http response back to the
client as it reads. In theory, git can go full-duplex; the
client can consume our response while it is still sending
the request.  In practice, however, HTTP is a half-duplex
protocol. Even if our client is ready to read and write
simultaneously, we may have other HTTP infrastructure in the
way, including the webserver that spawns our CGI, or any
intermediate proxies.

In at least one documented case[1], this leads to deadlock
when trying a fetch over http. What happens is basically:

  1. Apache proxies the request to the CGI, http-backend.

  2. http-backend gzip-inflates the data and sends
     the result to upload-pack.

  3. upload-pack acts on the data and generates output over
     the pipe back to Apache. Apache isn't reading because
     it's busy writing (step 1).

This works fine most of the time, because the upload-pack
output ends up in a system pipe buffer, and Apache reads
it as soon as it finishes writing. But if both the request
and the response exceed the system pipe buffer size, then we
deadlock (Apache blocks writing to http-backend,
http-backend blocks writing to upload-pack, and upload-pack
blocks writing to Apache).

We need to break the deadlock by spooling either the input
or the output. In this case, it's ideal to spool the input,
because Apache does not start reading either stdout _or_
stderr until we have consumed all of the input. So until we
do so, we cannot even get an error message out to the
client.

The solution is fairly straight-forward: we read the request
body into an in-memory buffer in http-backend, freeing up
Apache, and then feed the data ourselves to upload-pack. But
there are a few important things to note:

  1. We limit in-memory buffer to no larger than 1 megabyte
     to prevent an obvious denial-of-service attack. This
     is a new hard limit on requests, but it's likely that
     requests of this size didn't work before at all (i.e.,
     they would have run into the pipe buffer thing and
     deadlocked).

  2. We must take care only to buffer when we have to. For
     pushes, the incoming packfile may be of arbitrary
     size, and we should connect the input directly to
     receive-pack. There's no deadlock problem here, though,
     because we do not produce any output until the whole
     packfile has been read.

     For upload-pack's initial ref advertisement, we
     similarly do not need to buffer. Even though we may
     generate a lot of output, there is no request body at
     all (i.e., it is a GET, not a POST).

[1] http://article.gmane.org/gmane.comp.version-control.git/269020

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 11 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 3ad82a8..7cc2e8c 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -19,12 +19,13 @@ static struct string_list *query_params;
 struct rpc_service {
 	const char *name;
 	const char *config_name;
+	unsigned buffer_input : 1;
 	signed enabled : 2;
 };
 
 static struct rpc_service rpc_service[] = {
-	{ "upload-pack", "uploadpack", 1 },
-	{ "receive-pack", "receivepack", -1 },
+	{ "upload-pack", "uploadpack", 1, 1 },
+	{ "receive-pack", "receivepack", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -266,9 +267,49 @@ static struct rpc_service *select_service(const char *name)
 	return svc;
 }
 
-static void inflate_request(const char *prog_name, int out)
+/*
+ * This is basically strbuf_read(), except that if we
+ * hit MAX_REQUEST_BUFFER we die (we'd rather reject a
+ * maliciously large request than chew up infinite memory).
+ */
+#define MAX_REQUEST_BUFFER (1024 * 1024)
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	size_t len = 0, alloc = 8192;
+	unsigned char *buf = xmalloc(alloc);
+
+	while (1) {
+		ssize_t cnt;
+
+		cnt = read_in_full(fd, buf + len, alloc - len);
+		if (cnt < 0) {
+			free(buf);
+			return -1;
+		}
+
+		/* partial read from read_in_full means we hit EOF */
+		len += cnt;
+		if (len < alloc) {
+			*out = buf;
+			return len;
+		}
+
+		/* otherwise, grow and try again (if we can) */
+		if (alloc == MAX_REQUEST_BUFFER)
+			die("request was larger than our maximum size (%lu)",
+			    (unsigned long)(MAX_REQUEST_BUFFER-1));
+
+		alloc = alloc_nr(alloc);
+		if (alloc > MAX_REQUEST_BUFFER)
+			alloc = MAX_REQUEST_BUFFER;
+		REALLOC_ARRAY(buf, alloc);
+	}
+}
+
+static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
+	unsigned char *full_request = NULL;
 	unsigned char in_buf[8192];
 	unsigned char out_buf[8192];
 	unsigned long cnt = 0;
@@ -277,11 +318,21 @@ static void inflate_request(const char *prog_name, int out)
 	git_inflate_init_gzip_only(&stream);
 
 	while (1) {
-		ssize_t n = xread(0, in_buf, sizeof(in_buf));
+		ssize_t n;
+
+		if (buffer_input) {
+			if (full_request)
+				n = 0; /* nothing left to read */
+			else
+				n = read_request(0, &full_request);
+			stream.next_in = full_request;
+		} else {
+			n = xread(0, in_buf, sizeof(in_buf));
+			stream.next_in = in_buf;
+		}
+
 		if (n <= 0)
 			die("request ended in the middle of the gzip stream");
-
-		stream.next_in = in_buf;
 		stream.avail_in = n;
 
 		while (0 < stream.avail_in) {
@@ -307,9 +358,22 @@ static void inflate_request(const char *prog_name, int out)
 done:
 	git_inflate_end(&stream);
 	close(out);
+	free(full_request);
 }
 
-static void run_service(const char **argv)
+static void copy_request(const char *prog_name, int out)
+{
+	unsigned char *buf;
+	ssize_t n = read_request(0, &buf);
+	if (n < 0)
+		die_errno("error reading request body");
+	if (write_in_full(out, buf, n) != n)
+		die("%s aborted reading request", prog_name);
+	close(out);
+	free(buf);
+}
+
+static void run_service(const char **argv, int buffer_input)
 {
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
 	const char *user = getenv("REMOTE_USER");
@@ -334,7 +398,7 @@ static void run_service(const char **argv)
 				 "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
 
 	cld.argv = argv;
-	if (gzipped_request)
+	if (buffer_input || gzipped_request)
 		cld.in = -1;
 	cld.git_cmd = 1;
 	if (start_command(&cld))
@@ -342,7 +406,9 @@ static void run_service(const char **argv)
 
 	close(1);
 	if (gzipped_request)
-		inflate_request(argv[0], cld.in);
+		inflate_request(argv[0], cld.in, buffer_input);
+	else if (buffer_input)
+		copy_request(argv[0], cld.in);
 	else
 		close(0);
 
@@ -392,7 +458,7 @@ static void get_info_refs(char *arg)
 		packet_flush(1);
 
 		argv[0] = svc->name;
-		run_service(argv);
+		run_service(argv, 0);
 
 	} else {
 		select_getanyfile();
@@ -496,7 +562,7 @@ static void service_rpc(char *service_name)
 	end_headers();
 
 	argv[0] = svc->name;
-	run_service(argv);
+	run_service(argv, svc->buffer_input);
 	strbuf_release(&buf);
 }
 
-- 
2.4.1.396.g7ba6d7b

Re: [PATCH 0/2] fix http deadlock on giant ref negotiations

[Dennis Kaarsemaker]

On vr, 2015-05-15 at 02:29 -0400, Jeff King wrote:
> On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:
> 
> > The fundamental problem is the deadlock on the server side, which is
> > producing bogus protocol output. And that's a mismatch between what
> > Apache expects (that the CGI will read all of the input request and then
> > generate an output request) and what the CGI wants to do (stream output
> > as it reads the input).
> 
> At first I was irritated with Apache for this. But thinking on it more,
> it's really due to our shoe-horning of a full-duplex protocol into the
> half-duplex HTTP protocol. Even if we could convince Apache to work in a
> full-duplex way here, and even if our client is full-duplex (since
> otherwise we are just trading pipe buffers for TCP buffers), we still
> may face arbitrary HTTP proxies or other infrastructure in the middle.
> 
> So here's a series to try to address the issue. The first patch is a
> fixed version of the die-recursion fixup I posted earlier. The second is
> the interesting one.
> 
>   [1/2]: http-backend: fix die recursion with custom handler
>   [2/2]: http-backend: spool ref negotiation requests to buffer
> 
> I have no clue how to write a test that would trigger this reliably
> without requiring a gigantic test fixture. However, I did confirm that
> it fixes the problem on the chromium case you provided (which otherwise
> deadlocks reliably for me).

This looks similar to the failure I posted about alst year in
http://thread.gmane.org/gmane.comp.version-control.git/258514 

Though the issue is different, it has the same 'hanging git fetch'
symptom due to the deadlock between upload-pack and http-backend.

The patch I sent back then is suboptimal, as it can cause larger packs
than necessary (we still use it though, as the alternative is a
non-working git), but it does include a test you may be able to use to
verify your fix, if this is indeed the same issue.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH 0/2] fix http deadlock on giant ref negotiations

[Jeff King]

On Fri, May 15, 2015 at 09:41:20AM +0200, Dennis Kaarsemaker wrote:

> > I have no clue how to write a test that would trigger this reliably
> > without requiring a gigantic test fixture. However, I did confirm that
> > it fixes the problem on the chromium case you provided (which otherwise
> > deadlocks reliably for me).
> 
> This looks similar to the failure I posted about alst year in
> http://thread.gmane.org/gmane.comp.version-control.git/258514
> 
> Though the issue is different, it has the same 'hanging git fetch'
> symptom due to the deadlock between upload-pack and http-backend.

Thanks, I think it is the same issue (in the end I was replicating not
with `--reference`, but just by doing a fetch from the other
repository). And our solutions are essentially the same. I do prefer
mine because:

  1. It keeps the buffering logic in http-backend; the half-duplex
     nature is an http detail.

  2. I think it's better to buffer the request rather than the response,
     for the reasons I stated in the commit message.

> The patch I sent back then is suboptimal, as it can cause larger packs
> than necessary (we still use it though, as the alternative is a
> non-working git), but it does include a test you may be able to use to
> verify your fix, if this is indeed the same issue.

I applied the test from your patch, but couldn't get it to fail even
with stock git.  The test above it shrunk a bit, but I was able to tweak
yours to generate tags from 2001..100000, which I thought would have
worked.  I suspect it's something silly like the size not being quite
big enough for the pipe buffer on my system, or something like that.
Though I couldn't get it to fail even with 200,000 tags, so perhaps it's
something else.

-Peff

Re: [PATCH 0/2] fix http deadlock on giant ref negotiations

[Dennis Kaarsemaker]

On vr, 2015-05-15 at 04:38 -0400, Jeff King wrote:
> On Fri, May 15, 2015 at 09:41:20AM +0200, Dennis Kaarsemaker wrote:
> 
> > > I have no clue how to write a test that would trigger this reliably
> > > without requiring a gigantic test fixture. However, I did confirm that
> > > it fixes the problem on the chromium case you provided (which otherwise
> > > deadlocks reliably for me).
> > 
> > This looks similar to the failure I posted about alst year in
> > http://thread.gmane.org/gmane.comp.version-control.git/258514
> > 
> > Though the issue is different, it has the same 'hanging git fetch'
> > symptom due to the deadlock between upload-pack and http-backend.
> 
> Thanks, I think it is the same issue (in the end I was replicating not
> with `--reference`, but just by doing a fetch from the other
> repository). And our solutions are essentially the same. I do prefer
> mine because:
> 
>   1. It keeps the buffering logic in http-backend; the half-duplex
>      nature is an http detail.
> 
>   2. I think it's better to buffer the request rather than the response,
>      for the reasons I stated in the commit message.
> 
> > The patch I sent back then is suboptimal, as it can cause larger packs
> > than necessary (we still use it though, as the alternative is a
> > non-working git), but it does include a test you may be able to use to
> > verify your fix, if this is indeed the same issue.
> 
> I applied the test from your patch, but couldn't get it to fail even
> with stock git.  The test above it shrunk a bit, but I was able to tweak
> yours to generate tags from 2001..100000, which I thought would have
> worked.  I suspect it's something silly like the size not being quite
> big enough for the pipe buffer on my system, or something like that.
> Though I couldn't get it to fail even with 200,000 tags, so perhaps it's
> something else.

The shrinkage in the test above it will actually work around the issue,
as there are now fewer already-fetched tags to negotiate. Either
reverting that shrinkage or executing the new test twice should do the
trick.
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH 0/2] fix http deadlock on giant ref negotiations

[Jeff King]

On Fri, May 15, 2015 at 10:44:50AM +0200, Dennis Kaarsemaker wrote:

> > I applied the test from your patch, but couldn't get it to fail even
> > with stock git.  The test above it shrunk a bit, but I was able to tweak
> > yours to generate tags from 2001..100000, which I thought would have
> > worked.  I suspect it's something silly like the size not being quite
> > big enough for the pipe buffer on my system, or something like that.
> > Though I couldn't get it to fail even with 200,000 tags, so perhaps it's
> > something else.
> 
> The shrinkage in the test above it will actually work around the issue,
> as there are now fewer already-fetched tags to negotiate. Either
> reverting that shrinkage or executing the new test twice should do the
> trick.

Ah, right, that makes sense. I was creating the right number of tags,
but not with half of them already in the repo when I did the critical
fetch. I got it to fail by adding and fetching another 48,000, and then
adding and fetching another 50,000 on top of that.

Interestingly, with my patch the _first_ fetch fails, that otherwise
succeeds with stock git. My patch sets a maximum size on the spool
buffer, and we exceed it. I guess 1MB isn't enough for pathological
cases. I'm hesitant to let it expand indefinitely for security reasons,
but we could probably bump it to 10MB or something.

I dunno. I'm not excited about introducing new size restrictions that
were not there before.

Maybe it's time to implement git-over-websockets. ;)

-Peff

Re: [PATCH 0/2] fix http deadlock on giant ref negotiations

[Dennis Kaarsemaker]

On vr, 2015-05-15 at 04:53 -0400, Jeff King wrote:
> I guess 1MB isn't enough for pathological cases.

$ git for-each-ref | wc -l 
59658

I'm not saying it's sensible, but this is a real repo I have to work
with.
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The solution is fairly straight-forward: we read the request
> body into an in-memory buffer in http-backend, freeing up
> Apache, and then feed the data ourselves to upload-pack. But
> there are a few important things to note:
>
>   1. We limit in-memory buffer to no larger than 1 megabyte
>      to prevent an obvious denial-of-service attack. This
>      is a new hard limit on requests, but it's likely that
>      requests of this size didn't work before at all (i.e.,
>      they would have run into the pipe buffer thing and
>      deadlocked).
>
>   2. We must take care only to buffer when we have to. For
>      pushes, the incoming packfile may be of arbitrary
>      size, and we should connect the input directly to
>      receive-pack. There's no deadlock problem here, though,
>      because we do not produce any output until the whole
>      packfile has been read.
>
>      For upload-pack's initial ref advertisement, we
>      similarly do not need to buffer. Even though we may
>      generate a lot of output, there is no request body at
>      all (i.e., it is a GET, not a POST).

Thanks.

One unrelated thing I noticed was that three codepaths independently
have close(0) in run_service() now, and made me follow the two
helper functions to see they both do the close at the end.  It might
have made the flow easier to follow if run_service() were

    ...
    close(1);
    if (gzip)
    	inflate();
    else if (buffer)
        copy();
    close(0);
    ...

But that is minor.

Also, is it worth allocating small and then growing up to the maximum?
I think this only relays one request at a time anyway, and I suspect
that a single 1MB allocation at the first call kept getting reused
may be sufficient (and much simpler).

> [1] http://article.gmane.org/gmane.comp.version-control.git/269020
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http-backend.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 77 insertions(+), 11 deletions(-)
>
> diff --git a/http-backend.c b/http-backend.c
> index 3ad82a8..7cc2e8c 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -19,12 +19,13 @@ static struct string_list *query_params;
>  struct rpc_service {
>  	const char *name;
>  	const char *config_name;
> +	unsigned buffer_input : 1;
>  	signed enabled : 2;
>  };
>  
>  static struct rpc_service rpc_service[] = {
> -	{ "upload-pack", "uploadpack", 1 },
> -	{ "receive-pack", "receivepack", -1 },
> +	{ "upload-pack", "uploadpack", 1, 1 },
> +	{ "receive-pack", "receivepack", 0, -1 },
>  };
>  
>  static struct string_list *get_parameters(void)
> @@ -266,9 +267,49 @@ static struct rpc_service *select_service(const char *name)
>  	return svc;
>  }
>  
> -static void inflate_request(const char *prog_name, int out)
> +/*
> + * This is basically strbuf_read(), except that if we
> + * hit MAX_REQUEST_BUFFER we die (we'd rather reject a
> + * maliciously large request than chew up infinite memory).
> + */
> +#define MAX_REQUEST_BUFFER (1024 * 1024)
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +	size_t len = 0, alloc = 8192;
> +	unsigned char *buf = xmalloc(alloc);
> +
> +	while (1) {
> +		ssize_t cnt;
> +
> +		cnt = read_in_full(fd, buf + len, alloc - len);
> +		if (cnt < 0) {
> +			free(buf);
> +			return -1;
> +		}
> +
> +		/* partial read from read_in_full means we hit EOF */
> +		len += cnt;
> +		if (len < alloc) {
> +			*out = buf;
> +			return len;
> +		}
> +
> +		/* otherwise, grow and try again (if we can) */
> +		if (alloc == MAX_REQUEST_BUFFER)
> +			die("request was larger than our maximum size (%lu)",
> +			    (unsigned long)(MAX_REQUEST_BUFFER-1));
> +
> +		alloc = alloc_nr(alloc);
> +		if (alloc > MAX_REQUEST_BUFFER)
> +			alloc = MAX_REQUEST_BUFFER;
> +		REALLOC_ARRAY(buf, alloc);
> +	}
> +}
> +
> +static void inflate_request(const char *prog_name, int out, int buffer_input)
>  {
>  	git_zstream stream;
> +	unsigned char *full_request = NULL;
>  	unsigned char in_buf[8192];
>  	unsigned char out_buf[8192];
>  	unsigned long cnt = 0;
> @@ -277,11 +318,21 @@ static void inflate_request(const char *prog_name, int out)
>  	git_inflate_init_gzip_only(&stream);
>  
>  	while (1) {
> -		ssize_t n = xread(0, in_buf, sizeof(in_buf));
> +		ssize_t n;
> +
> +		if (buffer_input) {
> +			if (full_request)
> +				n = 0; /* nothing left to read */
> +			else
> +				n = read_request(0, &full_request);
> +			stream.next_in = full_request;
> +		} else {
> +			n = xread(0, in_buf, sizeof(in_buf));
> +			stream.next_in = in_buf;
> +		}
> +
>  		if (n <= 0)
>  			die("request ended in the middle of the gzip stream");
> -
> -		stream.next_in = in_buf;
>  		stream.avail_in = n;
>  
>  		while (0 < stream.avail_in) {
> @@ -307,9 +358,22 @@ static void inflate_request(const char *prog_name, int out)
>  done:
>  	git_inflate_end(&stream);
>  	close(out);
> +	free(full_request);
>  }
>  
> -static void run_service(const char **argv)
> +static void copy_request(const char *prog_name, int out)
> +{
> +	unsigned char *buf;
> +	ssize_t n = read_request(0, &buf);
> +	if (n < 0)
> +		die_errno("error reading request body");
> +	if (write_in_full(out, buf, n) != n)
> +		die("%s aborted reading request", prog_name);
> +	close(out);
> +	free(buf);
> +}
> +
> +static void run_service(const char **argv, int buffer_input)
>  {
>  	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
>  	const char *user = getenv("REMOTE_USER");
> @@ -334,7 +398,7 @@ static void run_service(const char **argv)
>  				 "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
>  
>  	cld.argv = argv;
> -	if (gzipped_request)
> +	if (buffer_input || gzipped_request)
>  		cld.in = -1;
>  	cld.git_cmd = 1;
>  	if (start_command(&cld))
> @@ -342,7 +406,9 @@ static void run_service(const char **argv)
>  
>  	close(1);
>  	if (gzipped_request)
> -		inflate_request(argv[0], cld.in);
> +		inflate_request(argv[0], cld.in, buffer_input);
> +	else if (buffer_input)
> +		copy_request(argv[0], cld.in);
>  	else
>  		close(0);
>  
> @@ -392,7 +458,7 @@ static void get_info_refs(char *arg)
>  		packet_flush(1);
>  
>  		argv[0] = svc->name;
> -		run_service(argv);
> +		run_service(argv, 0);
>  
>  	} else {
>  		select_getanyfile();
> @@ -496,7 +562,7 @@ static void service_rpc(char *service_name)
>  	end_headers();
>  
>  	argv[0] = svc->name;
> -	run_service(argv);
> +	run_service(argv, svc->buffer_input);
>  	strbuf_release(&buf);
>  }

Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer

[Konstantin Ryabitsev]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On 15/05/15 02:22 PM, Junio C Hamano wrote:
> Also, is it worth allocating small and then growing up to the maximum?
> I think this only relays one request at a time anyway, and I suspect
> that a single 1MB allocation at the first call kept getting reused
> may be sufficient (and much simpler).

Does it make sense to make that configurable via an env variable at all?
I suspect the vast majority of people would not hit this bug unless they
are dealing with repositories polluted with hundreds of refs created by
automation (like the codeaurora chromium repo).

E.g. can be set via an Apache directive like

SetEnv FOO_MAX_SIZE 2048

The backend can then be configured to emit an error message when the
spool size is exhausted saying "foo exhausted, increase FOO_MAX_SIZE to
allow for moar foo."

-K


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 538 bytes --]

Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer

[Jeff King]

On Fri, May 15, 2015 at 11:22:42AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The solution is fairly straight-forward: we read the request
> > body into an in-memory buffer in http-backend, freeing up
> > Apache, and then feed the data ourselves to upload-pack. But
> > there are a few important things to note:
> >
> >   1. We limit in-memory buffer to no larger than 1 megabyte
> >      to prevent an obvious denial-of-service attack. This
> >      is a new hard limit on requests, but it's likely that
> >      requests of this size didn't work before at all (i.e.,
> >      they would have run into the pipe buffer thing and
> >      deadlocked).

So this 1MB limit is clearly a problem, and the reasoning above is not
right. The case we are helping is when a large amount of input creates a
large amount of output. But we're _hurting_ the case where there's just
a large amount of input (as shown by the Dennis's test case).

What do we want to do about that? We can switch to streaming after
hitting our limit (so opening the opportunity for deadlock again in some
cases, but making sure we do no harm to cases that currently work). Or
we can just bump the input size and say "you'd be crazy to send more
than 10MB" (or 50, or whatever). We could make a configuration knob,
too, I guess.

> One unrelated thing I noticed was that three codepaths independently
> have close(0) in run_service() now, and made me follow the two
> helper functions to see they both do the close at the end.  It might
> have made the flow easier to follow if run_service() were
> 
>     ...
>     close(1);
>     if (gzip)
>     	inflate();
>     else if (buffer)
>         copy();
>     close(0);
>     ...
> 
> But that is minor.

I don't see the close(0) in the other (buffered) code paths. We close
the _output_ to the child, but of course we have to do that to tell it
we're done sending (I actually forgot it in an earlier version of
copy_request(), and things hang :) ).

I don't think there's any need to close(0) in the buffered cases. We
read until EOF in the copy() case. For gzip, we read until the end of
the gzipped data. I guess it would be better to close if we're not
expecting more input, as otherwise Apache might block trying to write to
us if the client sends bogus input (i.e., a zlib stream with more cruft
at the end).

> Also, is it worth allocating small and then growing up to the maximum?
> I think this only relays one request at a time anyway, and I suspect
> that a single 1MB allocation at the first call kept getting reused
> may be sufficient (and much simpler).

My initial attempt did exactly that, but I had a much smaller buffer. I
started to get worried around 1MB. If we bump it to 10MB (or make it
configurable), I get more so. I dunno. It is not _that_ much memory, but
it is per-request we are serving, so it might add up on a busy server.
OTOH, pack-objects thinks nothing of allocating 800MB just for the
book-keeping to serve a clone of torvalds/linux.

-Peff

[PATCH v2 0/3] fix http deadlock on giant ref negotiations

[Jeff King]

On Fri, May 15, 2015 at 02:28:37PM -0400, Konstantin Ryabitsev wrote:

> On 15/05/15 02:22 PM, Junio C Hamano wrote:
> > Also, is it worth allocating small and then growing up to the maximum?
> > I think this only relays one request at a time anyway, and I suspect
> > that a single 1MB allocation at the first call kept getting reused
> > may be sufficient (and much simpler).
> 
> Does it make sense to make that configurable via an env variable at all?
> I suspect the vast majority of people would not hit this bug unless they
> are dealing with repositories polluted with hundreds of refs created by
> automation (like the codeaurora chromium repo).
> 
> E.g. can be set via an Apache directive like
> 
> SetEnv FOO_MAX_SIZE 2048
> 
> The backend can then be configured to emit an error message when the
> spool size is exhausted saying "foo exhausted, increase FOO_MAX_SIZE to
> allow for moar foo."

Yeah, that was the same conclusion I came to elsewhere in the thread.
Here's a re-roll:

  [1/3]: http-backend: fix die recursion with custom handler
  [2/3]: t5551: factor out tag creation
  [3/3]: http-backend: spool ref negotiation requests to buffer

It makes the size configurable (either through the environment, which is
convenient for setting via Apache; or through the config, which is
convenient if you have one absurdly-sized repo). It mentions the
variable name when it barfs into the Apache log. I also bumped the
default to 10MB, which I think should be enough to handle even
ridiculous cases.

I also adapted Dennis's test into the third patch. Beware that it's
quite slow to run (and is protected by the "EXPENSIVE" prerequisite).
Patch 2 is new, and just refactors the script to make adding the new
test easier.

I really wanted to add a test like:

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index e2a2fa1..1fff812 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -273,6 +273,16 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
 	test_line_count = 2 posts
 '
 
+test_expect_success 'http-backend does not buffer arbitrarily large requests' '
+	test_when_finished "(
+		cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&
+		test_unconfig http.maxrequestbuffer
+	)" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.maxrequestbuffer 100 &&
+	test_must_fail git clone $HTTPD_URL/smart/repo.git foo.git
+'
+
 test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
 	(
 		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&

to test that the maxRequestBuffer code does indeed work. Unfortunately,
even though the server behaves reasonably in this case, the client ends
up hanging forever. I'm not sure there is a simple solution to that; I
think it is a protocol issue where remote-http is waiting for fetch-pack
to speak, but fetch-pack is waiting for more data from the remote.

Personally, I think I'd be much more interested in pursuing a saner,
full duplex http solution like git-over-websockets than trying to iron
out all of the corner cases in the stateless-rpc protocol.

-Peff

[PATCH v2 1/3] http-backend: fix die recursion with custom handler

[Jeff King]

When we die() in http-backend, we call a custom handler that
writes an HTTP 500 response to stdout, then reports the
error to stderr. Our routines for writing out the HTTP
response may themselves die, leading to us entering die()
again.

When it was originally written, that was OK; our custom
handler keeps a variable to notice this and does not
recurse. However, since cd163d4 (usage.c: detect recursion
in die routines and bail out immediately, 2012-11-14), the
main die() implementation detects recursion before we even
get to our custom handler, and bails without printing
anything useful.

We can handle this case by doing two things:

  1. Installing a custom die_is_recursing handler that
     allows us to enter up to one level of recursion. Only
     the first call to our custom handler will try to write
     out the error response. So if we die again, that is OK.
     If we end up dying more than that, it is a sign that we
     are in an infinite recursion.

  2. Reporting the error to stderr before trying to write
     out the HTTP response. In the current code, if we do
     die() trying to write out the response, we'll exit
     immediately from this second die(), and never get a
     chance to output the original error (which is almost
     certainly the more interesting one; the second die is
     just going to be along the lines of "I tried to write
     to stdout but it was closed").

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index b6c0484..3ad82a8 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -500,21 +500,24 @@ static void service_rpc(char *service_name)
 	strbuf_release(&buf);
 }
 
+static int dead;
 static NORETURN void die_webcgi(const char *err, va_list params)
 {
-	static int dead;
+	if (dead <= 1) {
+		vreportf("fatal: ", err, params);
 
-	if (!dead) {
-		dead = 1;
 		http_status(500, "Internal Server Error");
 		hdr_nocache();
 		end_headers();
-
-		vreportf("fatal: ", err, params);
 	}
 	exit(0); /* we successfully reported a failure ;-) */
 }
 
+static int die_webcgi_recursing(void)
+{
+	return dead++ > 1;
+}
+
 static char* getdir(void)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -569,6 +572,7 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 	set_die_routine(die_webcgi);
+	set_die_is_recursing_routine(die_webcgi_recursing);
 
 	if (!method)
 		die("No REQUEST_METHOD from server");
-- 
2.4.1.396.g7ba6d7b

[PATCH v2 2/3] t5551: factor out tag creation

[Jeff King]

One of our tests in t5551 creates a large number of tags,
and jumps through some hoops to do it efficiently. Let's
factor that out into a function so we can make other similar
tests.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5551-http-fetch-smart.sh | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 66439e5..c83cdf4 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -224,27 +224,35 @@ test_expect_success 'transfer.hiderefs works over smart-http' '
 	git -C hidden.git rev-parse --verify b
 '
 
-test_expect_success 'create 2,000 tags in the repo' '
-	(
-	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-	for i in $(test_seq 2000)
+# create an arbitrary number of tags, numbered from tag-$1 to tag-$2
+create_tags () {
+	rm -f marks &&
+	for i in $(test_seq "$1" "$2")
 	do
-		echo "commit refs/heads/too-many-refs"
-		echo "mark :$i"
-		echo "committer git <git@example.com> $i +0000"
-		echo "data 0"
-		echo "M 644 inline bla.txt"
-		echo "data 4"
-		echo "bla"
+		# don't use here-doc, because it requires a process
+		# per loop iteration
+		echo "commit refs/heads/too-many-refs-$1" &&
+		echo "mark :$i" &&
+		echo "committer git <git@example.com> $i +0000" &&
+		echo "data 0" &&
+		echo "M 644 inline bla.txt" &&
+		echo "data 4" &&
+		echo "bla" &&
 		# make every commit dangling by always
 		# rewinding the branch after each commit
-		echo "reset refs/heads/too-many-refs"
-		echo "from :1"
+		echo "reset refs/heads/too-many-refs-$1" &&
+		echo "from :$1"
 	done | git fast-import --export-marks=marks &&
 
 	# now assign tags to all the dangling commits we created above
 	tag=$(perl -e "print \"bla\" x 30") &&
 	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
+}
+
+test_expect_success 'create 2,000 tags in the repo' '
+	(
+		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		create_tags 1 2000
 	)
 '
 
-- 
2.4.1.396.g7ba6d7b

[PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer

[Jeff King]

When http-backend spawns "upload-pack" to do ref
negotiation, it streams the http request body to
upload-pack, who then streams the http response back to the
client as it reads. In theory, git can go full-duplex; the
client can consume our response while it is still sending
the request.  In practice, however, HTTP is a half-duplex
protocol. Even if our client is ready to read and write
simultaneously, we may have other HTTP infrastructure in the
way, including the webserver that spawns our CGI, or any
intermediate proxies.

In at least one documented case[1], this leads to deadlock
when trying a fetch over http. What happens is basically:

  1. Apache proxies the request to the CGI, http-backend.

  2. http-backend gzip-inflates the data and sends
     the result to upload-pack.

  3. upload-pack acts on the data and generates output over
     the pipe back to Apache. Apache isn't reading because
     it's busy writing (step 1).

This works fine most of the time, because the upload-pack
output ends up in a system pipe buffer, and Apache reads
it as soon as it finishes writing. But if both the request
and the response exceed the system pipe buffer size, then we
deadlock (Apache blocks writing to http-backend,
http-backend blocks writing to upload-pack, and upload-pack
blocks writing to Apache).

We need to break the deadlock by spooling either the input
or the output. In this case, it's ideal to spool the input,
because Apache does not start reading either stdout _or_
stderr until we have consumed all of the input. So until we
do so, we cannot even get an error message out to the
client.

The solution is fairly straight-forward: we read the request
body into an in-memory buffer in http-backend, freeing up
Apache, and then feed the data ourselves to upload-pack. But
there are a few important things to note:

  1. We limit the in-memory buffer to prevent an obvious
     denial-of-service attack. This is a new hard limit on
     requests, but it's unlikely to come into play. The
     default value is 10MB, which covers even the ridiculous
     100,000-ref negotation in the included test (that
     actually caps out just over 5MB). But it's configurable
     on the off chance that you don't mind spending some
     extra memory to make even ridiculous requests work.

  2. We must take care only to buffer when we have to. For
     pushes, the incoming packfile may be of arbitrary
     size, and we should connect the input directly to
     receive-pack. There's no deadlock problem here, though,
     because we do not produce any output until the whole
     packfile has been read.

     For upload-pack's initial ref advertisement, we
     similarly do not need to buffer. Even though we may
     generate a lot of output, there is no request body at
     all (i.e., it is a GET, not a POST).

[1] http://article.gmane.org/gmane.comp.version-control.git/269020

Test-adapted-from: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-http-backend.txt |  9 ++++
 http-backend.c                     | 97 +++++++++++++++++++++++++++++++++-----
 t/t5551-http-fetch-smart.sh        | 15 ++++++
 3 files changed, 110 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt
index d422ba4..8c6acbe 100644
--- a/Documentation/git-http-backend.txt
+++ b/Documentation/git-http-backend.txt
@@ -255,6 +255,15 @@ The GIT_HTTP_EXPORT_ALL environmental variable may be passed to
 'git-http-backend' to bypass the check for the "git-daemon-export-ok"
 file in each repository before allowing export of that repository.
 
+The `GIT_HTTP_MAX_REQUEST_BUFFER` environment variable (or the
+`http.maxRequestBuffer` config variable) may be set to change the
+largest ref negotiation request that git will handle during a fetch; any
+fetch requiring a larger buffer will not succeed.  This value should not
+normally need to be changed, but may be helpful if you are fetching from
+a repository with an extremely large number of refs.  The value can be
+specified with a unit (e.g., `100M` for 100 megabytes). The default is
+10 megabytes.
+
 The backend process sets GIT_COMMITTER_NAME to '$REMOTE_USER' and
 GIT_COMMITTER_EMAIL to '$\{REMOTE_USER}@http.$\{REMOTE_ADDR\}',
 ensuring that any reflogs created by 'git-receive-pack' contain some
diff --git a/http-backend.c b/http-backend.c
index 3ad82a8..d1333b8 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -13,18 +13,20 @@ static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
 static const char last_modified[] = "Last-Modified";
 static int getanyfile = 1;
+static unsigned long max_request_buffer = 10 * 1024 * 1024;
 
 static struct string_list *query_params;
 
 struct rpc_service {
 	const char *name;
 	const char *config_name;
+	unsigned buffer_input : 1;
 	signed enabled : 2;
 };
 
 static struct rpc_service rpc_service[] = {
-	{ "upload-pack", "uploadpack", 1 },
-	{ "receive-pack", "receivepack", -1 },
+	{ "upload-pack", "uploadpack", 1, 1 },
+	{ "receive-pack", "receivepack", 0, -1 },
 };
 
 static struct string_list *get_parameters(void)
@@ -225,6 +227,7 @@ static void http_config(void)
 	struct strbuf var = STRBUF_INIT;
 
 	git_config_get_bool("http.getanyfile", &getanyfile);
+	git_config_get_ulong("http.maxrequestbuffer", &max_request_buffer);
 
 	for (i = 0; i < ARRAY_SIZE(rpc_service); i++) {
 		struct rpc_service *svc = &rpc_service[i];
@@ -266,9 +269,53 @@ static struct rpc_service *select_service(const char *name)
 	return svc;
 }
 
-static void inflate_request(const char *prog_name, int out)
+/*
+ * This is basically strbuf_read(), except that if we
+ * hit max_request_buffer we die (we'd rather reject a
+ * maliciously large request than chew up infinite memory).
+ */
+static ssize_t read_request(int fd, unsigned char **out)
+{
+	size_t len = 0, alloc = 8192;
+	unsigned char *buf = xmalloc(alloc);
+
+	if (max_request_buffer < alloc)
+		max_request_buffer = alloc;
+
+	while (1) {
+		ssize_t cnt;
+
+		cnt = read_in_full(fd, buf + len, alloc - len);
+		if (cnt < 0) {
+			free(buf);
+			return -1;
+		}
+
+		/* partial read from read_in_full means we hit EOF */
+		len += cnt;
+		if (len < alloc) {
+			*out = buf;
+			warning("request size was %lu", (unsigned long)len);
+			return len;
+		}
+
+		/* otherwise, grow and try again (if we can) */
+		if (alloc == max_request_buffer)
+			die("request was larger than our maximum size (%lu);"
+			    " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
+			    max_request_buffer);
+
+		alloc = alloc_nr(alloc);
+		if (alloc > max_request_buffer)
+			alloc = max_request_buffer;
+		REALLOC_ARRAY(buf, alloc);
+	}
+}
+
+static void inflate_request(const char *prog_name, int out, int buffer_input)
 {
 	git_zstream stream;
+	unsigned char *full_request = NULL;
 	unsigned char in_buf[8192];
 	unsigned char out_buf[8192];
 	unsigned long cnt = 0;
@@ -277,11 +324,21 @@ static void inflate_request(const char *prog_name, int out)
 	git_inflate_init_gzip_only(&stream);
 
 	while (1) {
-		ssize_t n = xread(0, in_buf, sizeof(in_buf));
+		ssize_t n;
+
+		if (buffer_input) {
+			if (full_request)
+				n = 0; /* nothing left to read */
+			else
+				n = read_request(0, &full_request);
+			stream.next_in = full_request;
+		} else {
+			n = xread(0, in_buf, sizeof(in_buf));
+			stream.next_in = in_buf;
+		}
+
 		if (n <= 0)
 			die("request ended in the middle of the gzip stream");
-
-		stream.next_in = in_buf;
 		stream.avail_in = n;
 
 		while (0 < stream.avail_in) {
@@ -307,9 +364,22 @@ static void inflate_request(const char *prog_name, int out)
 done:
 	git_inflate_end(&stream);
 	close(out);
+	free(full_request);
+}
+
+static void copy_request(const char *prog_name, int out)
+{
+	unsigned char *buf;
+	ssize_t n = read_request(0, &buf);
+	if (n < 0)
+		die_errno("error reading request body");
+	if (write_in_full(out, buf, n) != n)
+		die("%s aborted reading request", prog_name);
+	close(out);
+	free(buf);
 }
 
-static void run_service(const char **argv)
+static void run_service(const char **argv, int buffer_input)
 {
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
 	const char *user = getenv("REMOTE_USER");
@@ -334,7 +404,7 @@ static void run_service(const char **argv)
 				 "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
 
 	cld.argv = argv;
-	if (gzipped_request)
+	if (buffer_input || gzipped_request)
 		cld.in = -1;
 	cld.git_cmd = 1;
 	if (start_command(&cld))
@@ -342,7 +412,9 @@ static void run_service(const char **argv)
 
 	close(1);
 	if (gzipped_request)
-		inflate_request(argv[0], cld.in);
+		inflate_request(argv[0], cld.in, buffer_input);
+	else if (buffer_input)
+		copy_request(argv[0], cld.in);
 	else
 		close(0);
 
@@ -392,7 +464,7 @@ static void get_info_refs(char *arg)
 		packet_flush(1);
 
 		argv[0] = svc->name;
-		run_service(argv);
+		run_service(argv, 0);
 
 	} else {
 		select_getanyfile();
@@ -496,7 +568,7 @@ static void service_rpc(char *service_name)
 	end_headers();
 
 	argv[0] = svc->name;
-	run_service(argv);
+	run_service(argv, svc->buffer_input);
 	strbuf_release(&buf);
 }
 
@@ -623,6 +695,9 @@ int main(int argc, char **argv)
 		not_found("Repository not exported: '%s'", dir);
 
 	http_config();
+	max_request_buffer = git_env_ulong("GIT_HTTP_MAX_REQUEST_BUFFER",
+					   max_request_buffer);
+
 	cmd->imp(cmd_arg);
 	return 0;
 }
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index c83cdf4..a540c6d 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -273,5 +273,20 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '
 	test_line_count = 2 posts
 '
 
+test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
+	(
+		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		create_tags 2001 50000
+	) &&
+	git -C too-many-refs fetch -q --tags &&
+	(
+		cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+		create_tags 50001 100000
+	) &&
+	git -C too-many-refs fetch -q --tags &&
+	git -C too-many-refs for-each-ref refs/tags >tags &&
+	test_line_count = 100000 tags
+'
+
 stop_httpd
 test_done
-- 
2.4.1.396.g7ba6d7b

Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer

[Konstantin Ryabitsev]

On 20 May 2015 at 03:37, Jeff King <peff@peff.net> wrote:
> +               /* partial read from read_in_full means we hit EOF */
> +               len += cnt;
> +               if (len < alloc) {
> +                       *out = buf;
> +                       warning("request size was %lu", (unsigned long)len);
> +                       return len;
> +               }

Jeff:

This patch appears to work well -- the only complaint I have is that I
now have "warning: request size was NNN" all over my error logs. :) Is
it supposed to convey an actual warning message, or is it merely a
debug statement?

Best,
-- 
Konstantin Ryabitsev
Sr. Systems Administrator
Linux Foundation Collab Projects
541-224-6067
Montréal, Québec

Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer

[Jeff King]

On Mon, May 25, 2015 at 10:07:50PM -0400, Konstantin Ryabitsev wrote:

> On 20 May 2015 at 03:37, Jeff King <peff@peff.net> wrote:
> > +               /* partial read from read_in_full means we hit EOF */
> > +               len += cnt;
> > +               if (len < alloc) {
> > +                       *out = buf;
> > +                       warning("request size was %lu", (unsigned long)len);
> > +                       return len;
> > +               }
> 
> Jeff:
> 
> This patch appears to work well -- the only complaint I have is that I
> now have "warning: request size was NNN" all over my error logs. :) Is
> it supposed to convey an actual warning message, or is it merely a
> debug statement?

Whoops, yeah, it was just for debugging. I missed that one when sending
out the patch.

Junio, the squashable patch is below (on jk/http-backend-deadlock-2.2),
and it looks like nothing has hit "next" yet. But you did do some
up-merging of the topic. Let me know if you would prefer to just have a
patch on top.

diff --git a/http-backend.c b/http-backend.c
index d1333b8..6bf139b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -295,7 +295,6 @@ static ssize_t read_request(int fd, unsigned char **out)
 		len += cnt;
 		if (len < alloc) {
 			*out = buf;
-			warning("request size was %lu", (unsigned long)len);
 			return len;
 		}
 

-Peff

Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Whoops, yeah, it was just for debugging. I missed that one when sending
> out the patch.
>
> Junio, the squashable patch is below (on jk/http-backend-deadlock-2.2),
> and it looks like nothing has hit "next" yet. But you did do some
> up-merging of the topic. Let me know if you would prefer to just have a
> patch on top.

Sorry, I missed that, too (actually I saw warning() and said "Heh,
OK", without realizing that this is a normal return path).

Thanks.

>
> diff --git a/http-backend.c b/http-backend.c
> index d1333b8..6bf139b 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -295,7 +295,6 @@ static ssize_t read_request(int fd, unsigned char **out)
>  		len += cnt;
>  		if (len < alloc) {
>  			*out = buf;
> -			warning("request size was %lu", (unsigned long)len);
>  			return len;
>  		}
>  
>
> -Peff