Clone hangs when done over http with --reference

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

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

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 <redacted>
---
 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

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

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:

quoted

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

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

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

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 <redacted>
---
 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

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 <redacted>
---
 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

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:

quoted

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

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

quoted

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

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:

quoted

quoted

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.

quoted

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

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

quoted

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

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