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

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:

index 0670719..25471f6 100644
@@ -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);

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").

---
 http-backend.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

index b6c0484..c210d4d 100644
@@ -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

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:

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

Best,
-Konstantin

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").

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

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

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

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.

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

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

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

$ 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

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").

---
 http-backend.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index b6c0484..3ad82a8 100644
@@ -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

---
 http-backend.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 11 deletions(-)

index 3ad82a8..7cc2e8c 100644
@@ -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

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

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.

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

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

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

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:

index e2a2fa1..1fff812 100755
@@ -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

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").

---
 http-backend.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index b6c0484..3ad82a8 100644
@@ -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

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.

---
 t/t5551-http-fetch-smart.sh | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

index 66439e5..c83cdf4 100755
@@ -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 <[email protected]> $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 <[email protected]> $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

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 <[email protected]>

---
 Documentation/git-http-backend.txt |  9 ++++
 http-backend.c                     | 97 +++++++++++++++++++++++++++++++++-----
 t/t5551-http-fetch-smart.sh        | 15 ++++++
 3 files changed, 110 insertions(+), 11 deletions(-)

index d422ba4..8c6acbe 100644
@@ -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

index c83cdf4..a540c6d 100755
@@ -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

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.

index d1333b8..6bf139b 100644
@@ -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

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

Thanks.