{"thread":{"id":"39330","subject":"Clone hangs when done over http with --reference","startedAt":"2015-05-13T21:04:36Z","lastAt":"2015-05-26T03:43:08Z","messageCount":24,"participants":["Konstantin Ryabitsev","Jeff King","Dennis Kaarsemaker","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"261204","messageId":"20150513210436.GA7407@gmail.com","threadId":"39330","inReplyTo":null,"subject":"Clone hangs when done over http with --reference","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2015-05-13T21:04:36Z","receivedAt":"2015-05-13T21:04:36Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"I have a reproducible case where git clone --reference hangs when\nperformed over http://, but not when performed over git://\n\nThe repository in question is very large, which possibly plays a role in\nthis. Unfortunately, I was not able to reproduce this with any other\nrepository, so if anyone wants to try this, they will have to suffer\nthrough 2.5GB downloads.\n\nTo reproduce on the client:\n\ngit clone --mirror \\\nhttp://source.codeaurora.org/mirrors/chromium.googlesource.com/chromium/src\n(completes after downloading ~2.5GB)\n\ngit clone --reference ./src.git --mirror \\\nhttp://source.codeaurora.org/quic/chrome4sdp/chromium/src.git foo.git\n(this hangs forever)\n\nIf you do this over git:// protocol, it will work:\n\ngit clone --reference ./src.git --mirror \\\ngit://source.codeaurora.org/quic/chrome4sdp/chromium/src.git foo.git\n(completes after downloading ~100MB)\n\nIt will also work if you clone the same repo without --reference. Both\nrepositories pass fsck checks.\n\nI reproduced it with git-2.4.0 on the server, and it appears to be\nunrelated to the Apache/Nginx versions nor various httpd daemon\nsettings, at least not in my testing. No errors are generated in the\nlogs. The process just appears to be stuck not doing anything.\n\nTo reproduce locally, simply set up these two repositories in\n/var/lib/git (one can be a --reference clone of the other -- it didn't\nmatter in my tests), and put this in /etc/httpd/conf.d/git.conf,\nassuming Fedora or Centos7 system:\n\nSetEnv GIT_PROJECT_ROOT /var/lib/git\nSetEnv GIT_HTTP_EXPORT_ALL 1\n\nAliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$             /var/lib/git/$1\nAliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$    /var/lib/git/$1\n\nScriptAlias /git/ /usr/libexec/git-core/git-http-backend/\n\n<Directory \"/var/lib/git\">\n    AllowOverride None\n    Options None\n    Require all granted\n</Directory>\n<Directory \"/usr/libexec/git-core\">\n    AllowOverride None\n    Options None\n    Require all granted\n</Directory>\n\nNot sure what is going on, but it appears that the hang is on the\nserver. Hope someone can figure it out.\n\nBest,\n-- \nKonstantin Ryabitsev\nLinux Foundation Collab Projects\nMontréal, Québec\n"},{"id":"261215","messageId":"20150514004724.GA3059@peff.net","threadId":"39330","inReplyTo":"20150513210436.GA7407@gmail.com","subject":"Re: Clone hangs when done over http with --reference","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T00:47:25Z","receivedAt":"2015-05-14T00:47:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 05:04:36PM -0400, Konstantin Ryabitsev wrote:\n\n> I have a reproducible case where git clone --reference hangs when\n> performed over http://, but not when performed over git://\n\nThanks for giving us a reproduction recipe. I was able to recreate the\nproblem on my machine.\n\nThere's a minor bug in git's error reporting that makes this a little\nharder to examine, but isn't the root cause. I'll send a patch for that\nmomentarily.  But here's what I've found.\n\nDuring the ref negotiation between the client and the server, there's so\nmuch data that we end up making several POSTs to the server. In one of\nthem, we end up in a deadlock situation between the CGI and Apache. You\ncan get a full strace of the Apache side by cloning this:\n\n  git://github.com/gist/9daf49aaaff16b4436f2\n\nThe interesting part is at 19:23:48.182380. Process 19891 is Apache,\nreading from the client (on fd 8) and relaying the data to http-backend\nover fd 10:\n\n  19891 19:23:48.182331 read(8,  <unfinished ...>\n  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\n  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 ...>\n  19891 19:23:48.182373 <... write resumed> ) = -1 EAGAIN (Resource temporarily unavailable)\n  19891 19:23:48.182380 poll([{fd=10, events=POLLOUT}], 1, 60000 <unfinished ...>\n  19891 19:24:48.242493 <... poll resumed> ) = 0 (Timeout)\n  19891 19:24:48.242609 close(10)         = 0\n\nDuring one of the writes we get EAGAIN; the buffer to http-backend is\nfull. So Apache calls poll() to give it 60 seconds. The http-backend\nprocess never starts reading, so Apache gives up and closes the\ndescriptor, truncating the input to http-backend.\n\nSo what's http-backend doing? It's pid 2361 in this output (which you\ncan verify looking further back in the strace dump and finding the\nclone/exec). It's busily relaying all the data to its child,\nupload-pack:\n\n  2361  19:23:48.182232 read(0,  <unfinished ...>\n  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\n  2361  19:23:48.182324 write(4, \"3435b8c152bec0c78998ff0445bd2c0f\"..., 8192 <unfinished ...>\n  2361  19:24:48.247291 <... write resumed> ) = 8192\n\nbut notice the minute-long jump in that write. It's not reading from\nstdin because it's _also_ blocked on writing. So why isn't upload-pack\nreading?\n\n  2363  19:23:48.184370 read(0, \"have 4ed051beb0ff979344c541ec659\"..., 46) = 46\n  2363  19:23:48.184381 alarm(0)          = 0\n  2363  19:23:48.184398 write(1, \"0038ACK 4ed051beb0ff979344c541ec\"..., 56 <unfinished ...>\n  2363  19:24:48.243565 <... write resumed> ) = 56\n\nIt's writing responses to its own stdout, of course! Which also blocks,\npreventing us from reading during the crucial minute.  Now who's\nsupposed to be reading our stdout here?\n\nIt's rather difficult to tell from the strace output, as you have to\nwalk through the parents to see if anybody re-opened stdout over a pipe.\nBut the last one is in process 2361, just before we exec\ngit-http-backend. Apache has opened a pipe for the CGI stdout, and is\nholding the other end of the pipe. But it's not reading, because it's\nblocked trying to write the request body to us.\n\nAt 19:24:48, Apache gives up on writing and truncates the input to the\nCGI. It starts reading the output, which lets Git start moving again.\nBut we hit this code:\n\n  ssize_t n = xread(0, in_buf, sizeof(in_buf));\n  if (n <= 0)\n\tdie(\"request ended in the middle of the gzip stream\");\n\nbecause of the truncated input. At this point, everything is done on the\nserver. Presumably Apache sends us a reasonable content-length, so even\nthough our output isn't complete, we at least know when it ends. You can\nsee that in the client git-remote-http here (note this is from a\ndifferent run than above, so the timestamps are not the same; this is\na bit after the minute-long hang has completed):\n\n  4453  20:19:32.557686 recvfrom(5,  <unfinished ...>\n  4453  20:19:32.557695 <... recvfrom resumed> \"4cea692129be9c003c33 common\\n\\r\\n38\"..., 16384, 0, NULL, NULL) = 9025\n  4453  20:19:32.557703 write(6, \"4cea692129be9c003c33 common\\n\", 28 <unfinished ...>\n  4453  20:19:32.557714 <... write resumed> ) = 28 \n\nThis is the last bit of data we read from the server, so presumably we\nbelieve we got it all. We relay it all to fd 6, which goes to our\ngit-fetch-pack sub-process. It in turn reads everything we give it, and\nthen waits for more. But we don't have any more. The two client\nprocesses are deadlocked, each waiting for more data from the other. The\nremote-http process has no way to tell fetch-pack \"the server didn't\nsend any more data\". It can't just close() the descriptor, because it's\nexpecting fetch-pack to tell it to make another POST request to the\nserver. And fetch-pack is waiting for more input, because what came from\nthe server was not enough to proceed.\n\nThis is a non-ideal way to handle the error, of course. But it's not the\nfundamental problem; the best we could do is notice and say \"hey, the\nserver didn't send us enough data\".\n\nThe fundamental problem is the deadlock on the server side, which is\nproducing bogus protocol output. And that's a mismatch between what\nApache expects (that the CGI will read all of the input request and then\ngenerate an output request) and what the CGI wants to do (stream output\nas it reads the input).\n\nI don't know if there's a way to convince Apache to be more interactive.\nAs a hacky workaround, we could basically spool all of the input into\nmemory (or a tempfile) and work from that. Or the output. Either way\nwould break the pipe deadlock. But we'd have to be sensitive to the type\nof request (it's probably OK to spool ref negotiation, but not OK to\nspool packfiles, which can be arbitrarily big).\n\nThe (horrible, should-not-be-applied) patch below makes your case work\nfor me:\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 0670719..25471f6 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -269,21 +269,20 @@ static struct rpc_service *select_service(const char *name)\n static void inflate_request(const char *prog_name, int out)\n {\n \tgit_zstream stream;\n-\tunsigned char in_buf[8192];\n+\tstruct strbuf in = STRBUF_INIT;\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n \n \tmemset(&stream, 0, sizeof(stream));\n \tgit_inflate_init_gzip_only(&stream);\n \n-\twhile (1) {\n-\t\tssize_t n = xread(0, in_buf, sizeof(in_buf));\n-\t\tif (n <= 0)\n-\t\t\tdie(\"request ended in the middle of the gzip stream\");\n-\n-\t\tstream.next_in = in_buf;\n-\t\tstream.avail_in = n;\n+\tif (strbuf_read(&in, 0, 8192) < 0)\n+\t\tdie(\"unable to read request\");\n+\tstream.next_in = (unsigned char *)in.buf;\n+\tstream.avail_in = in.len;\n \n+\t{\n+\t\tunsigned long n;\n \t\twhile (0 < stream.avail_in) {\n \t\t\tint ret;\n \n@@ -304,6 +303,8 @@ static void inflate_request(const char *prog_name, int out)\n \t\t}\n \t}\n \n+\tdie(\"request ended in the middle of the gzip stream\");\n+\n done:\n \tgit_inflate_end(&stream);\n \tclose(out);\n"},{"id":"261216","messageId":"20150514010233.GA7808@peff.net","threadId":"39330","inReplyTo":"20150514004724.GA3059@peff.net","subject":"[PATCH] http-backend: fix die recursion with custom handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T01:02:34Z","receivedAt":"2015-05-14T01:02:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:\n\n> There's a minor bug in git's error reporting that makes this a little\n> harder to examine, but isn't the root cause. I'll send a patch for that\n> momentarily.  But here's what I've found.\n\nHere's that patch. You may have seen \"recursion detected in die handler\"\nin your apache logs.  Basically we die(), try to write an HTTP error\nresponse, and then die() trying to write it again. It happens reliably\nhere because this particular error happens _after_ we've already written\nout the normal response header and closed stdout. So any die() we\nencounter after that is going to try to write its own error header, and\nwill fail because of the closed stdout.\n\nOne way of avoiding this would obviously be to notice in the die()\nhandler that we have already written our header and closed stdout. That\nwould help this case, but it would not help any other case where writing\nfails unexpectedly. So I'd rather solve the general recursion problem,\nwhich covers all cases.\n\n-- >8 --\nSubject: http-backend: fix die recursion with custom handler\n\nWhen we die() in http-backend, we call a custom handler that\nwrites an HTTP 500 response to stdout, then reports the\nerror to stderr. Our routines for writing out the HTTP\nresponse may themselves die, leading to us entering die()\nagain.\n\nWhen it was originally written, that was OK; our custom\nhandler keeps a variable to notice this and does not recurse\nindefinitely. However, since cd163d4 (usage.c: detect\nrecursion in die routines and bail out immediately,\n2012-11-14), the main die() implementation detects recursion\nbefore we even get to our custom handler, and bails without\nprinting anything useful.\n\nWe can handle this case by doing two things:\n\n  1. Installing a custom die_is_recursing handler that\n     allows us to enter up to one level of recursion. Only\n     the first call to our custom handler will try to write\n     out the error response. So if we die again, that is OK.\n     If we end up dying more than that, it is a sign that we\n     have a bug and are in an infinite recursion (i.e., what\n     cd163d4 was designed to protect against).\n\n  2. Reporting the error to stderr before trying to write\n     out the HTTP response. In the current code, if we do\n     die() trying to write out the response, we'll exit\n     immediately from this second die(), and never get a\n     chance to output the original error (which is almost\n     certainly the more interesting one; the second die is\n     just going to be along the lines of \"I tried to write\n     to stdout but it was closed\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-backend.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex b6c0484..c210d4d 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -500,21 +500,25 @@ static void service_rpc(char *service_name)\n \tstrbuf_release(&buf);\n }\n \n+static int dead;\n static NORETURN void die_webcgi(const char *err, va_list params)\n {\n-\tstatic int dead;\n+\tif (dead <= 1) {\n+\t\tvreportf(\"fatal: \", err, params);\n \n-\tif (!dead) {\n-\t\tdead = 1;\n \t\thttp_status(500, \"Internal Server Error\");\n \t\thdr_nocache();\n \t\tend_headers();\n-\n-\t\tvreportf(\"fatal: \", err, params);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\n \n+static int die_webcgi_recursing(void)\n+{\n+\tdead++;\n+\treturn dead > 1;\n+}\n+\n static char* getdir(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -569,6 +573,7 @@ int main(int argc, char **argv)\n \n \tgit_extract_argv0_path(argv[0]);\n \tset_die_routine(die_webcgi);\n+\tset_die_is_recursing_routine(die_webcgi_recursing);\n \n \tif (!method)\n \t\tdie(\"No REQUEST_METHOD from server\");\n-- \n2.4.0.327.ge28c153\n"},{"id":"261242","messageId":"5554D69E.20307@linuxfoundation.org","threadId":"39330","inReplyTo":"20150514004724.GA3059@peff.net","subject":"Re: Clone hangs when done over http with --reference","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2015-05-14T17:08:46Z","receivedAt":"2015-05-14T17:08:46Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On 13/05/15 08:47 PM, Jeff King wrote:\n> I don't know if there's a way to convince Apache to be more interactive.\n> As a hacky workaround, we could basically spool all of the input into\n> memory (or a tempfile) and work from that. Or the output. Either way\n> would break the pipe deadlock. But we'd have to be sensitive to the type\n> of request (it's probably OK to spool ref negotiation, but not OK to\n> spool packfiles, which can be arbitrarily big).\n\nThanks for looking into this, Jeff.\n\nTwo questions:\n\n1. Does this mean there are potential problems with other git operations\nthat involve ref negotiation, not just when doing git clone --reference?\nIs there a chance to run in to this deadlock by doing an operation like\n\"git remote update\"?\n\n2. If we configure the webserver to serve some files directly, without\npassing them to http-backend, e.g. doing the recommended apache magic:\n\n> AliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$          /var/lib/git/$1\n> AliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$ /var/lib/git/$1\n> AliasMatch ^/git/(.*/refs/heads/.*)$                             /var/lib/git/$1\n\nWill that make the spooling less of a problem, since it won't involve\nthe super-huge files?\n\nBest,\n-Konstantin\n\n"},{"id":"261272","messageId":"20150514195226.GA10205@peff.net","threadId":"39330","inReplyTo":"5554D69E.20307@linuxfoundation.org","subject":"Re: Clone hangs when done over http with --reference","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-14T19:52:26Z","receivedAt":"2015-05-14T19:52:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 14, 2015 at 01:08:46PM -0400, Konstantin Ryabitsev wrote:\n\n> Two questions:\n> \n> 1. Does this mean there are potential problems with other git operations\n> that involve ref negotiation, not just when doing git clone --reference?\n> Is there a chance to run in to this deadlock by doing an operation like\n> \"git remote update\"?\n\nYes. From the server's perspective, a \"clone --reference\" is really no\ndifferent than a fetch in which the client happened to have all of those\nrefs already. I didn't try it, but you should be able to reproduce the\nproblem with:\n\n  cd first-repo.git\n  git fetch https://.../second-repo.git refs/*:refs/remotes/foo/*\n\nwhich should have to do the exact same ref negotiation (\"I have these\ncommits, I want these other commits\").\n\n> 2. If we configure the webserver to serve some files directly, without\n> passing them to http-backend, e.g. doing the recommended apache magic:\n> \n> > AliasMatch ^/git/(.*/objects/[0-9a-f]{2}/[0-9a-f]{38})$          /var/lib/git/$1\n> > AliasMatch ^/git/(.*/objects/pack/pack-[0-9a-f]{40}.(pack|idx))$ /var/lib/git/$1\n> > AliasMatch ^/git/(.*/refs/heads/.*)$                             /var/lib/git/$1\n> \n> Will that make the spooling less of a problem, since it won't involve\n> the super-huge files?\n\nNo, that won't help. Once git is doing the smart protocol, it will never\nask for arbitrary files. So you would have to disable smart-http\nentirely, which I don't recommend.\n\nBesides which, it's not the size of the objects or packs that is an\nissue here. It's the relationship of the tips in the second repo to the\ntips in the first. That is, the \"big\" data here is the client and server\nfinding the common commits between the two (and it's not even _that_\nbig; it's just big by \"stuffing into a pipe buffer\" standards; as you\nnoticed, the git protocol handles it just fine).\n\n-Peff\n"},{"id":"261322","messageId":"20150515062034.GA30741@peff.net","threadId":"39330","inReplyTo":"20150514010233.GA7808@peff.net","subject":"Re: [PATCH] http-backend: fix die recursion with custom handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T06:20:35Z","receivedAt":"2015-05-15T06:20:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 09:02:33PM -0400, Jeff King wrote:\n\n> +static int die_webcgi_recursing(void)\n> +{\n> +\tdead++;\n> +\treturn dead > 1;\n> +}\n\nUgh, somehow I managed to introduce an off-by-one here while prettifying\nthe code before sending. It should be \"> 2\" (or \"dead++ > 1\" to get the\nvalue before incrementing).\n\nThe series I'll post in a moment will have a fixed version.\n\n-Peff\n"},{"id":"261323","messageId":"20150515062901.GA30768@peff.net","threadId":"39330","inReplyTo":"20150514004724.GA3059@peff.net","subject":"[PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T06:29:01Z","receivedAt":"2015-05-15T06:29:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:\n\n> The fundamental problem is the deadlock on the server side, which is\n> producing bogus protocol output. And that's a mismatch between what\n> Apache expects (that the CGI will read all of the input request and then\n> generate an output request) and what the CGI wants to do (stream output\n> as it reads the input).\n\nAt first I was irritated with Apache for this. But thinking on it more,\nit's really due to our shoe-horning of a full-duplex protocol into the\nhalf-duplex HTTP protocol. Even if we could convince Apache to work in a\nfull-duplex way here, and even if our client is full-duplex (since\notherwise we are just trading pipe buffers for TCP buffers), we still\nmay face arbitrary HTTP proxies or other infrastructure in the middle.\n\nSo here's a series to try to address the issue. The first patch is a\nfixed version of the die-recursion fixup I posted earlier. The second is\nthe interesting one.\n\n  [1/2]: http-backend: fix die recursion with custom handler\n  [2/2]: http-backend: spool ref negotiation requests to buffer\n\nI have no clue how to write a test that would trigger this reliably\nwithout requiring a gigantic test fixture. However, I did confirm that\nit fixes the problem on the chromium case you provided (which otherwise\ndeadlocks reliably for me).\n\n-Peff\n"},{"id":"261324","messageId":"20150515062926.GA30890@peff.net","threadId":"39330","inReplyTo":"20150515062901.GA30768@peff.net","subject":"[PATCH 1/2] http-backend: fix die recursion with custom handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T06:29:27Z","receivedAt":"2015-05-15T06:29:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we die() in http-backend, we call a custom handler that\nwrites an HTTP 500 response to stdout, then reports the\nerror to stderr. Our routines for writing out the HTTP\nresponse may themselves die, leading to us entering die()\nagain.\n\nWhen it was originally written, that was OK; our custom\nhandler keeps a variable to notice this and does not\nrecurse. However, since cd163d4 (usage.c: detect recursion\nin die routines and bail out immediately, 2012-11-14), the\nmain die() implementation detects recursion before we even\nget to our custom handler, and bails without printing\nanything useful.\n\nWe can handle this case by doing two things:\n\n  1. Installing a custom die_is_recursing handler that\n     allows us to enter up to one level of recursion. Only\n     the first call to our custom handler will try to write\n     out the error response. So if we die again, that is OK.\n     If we end up dying more than that, it is a sign that we\n     are in an infinite recursion.\n\n  2. Reporting the error to stderr before trying to write\n     out the HTTP response. In the current code, if we do\n     die() trying to write out the response, we'll exit\n     immediately from this second die(), and never get a\n     chance to output the original error (which is almost\n     certainly the more interesting one; the second die is\n     just going to be along the lines of \"I tried to write\n     to stdout but it was closed\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-backend.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex b6c0484..3ad82a8 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -500,21 +500,24 @@ static void service_rpc(char *service_name)\n \tstrbuf_release(&buf);\n }\n \n+static int dead;\n static NORETURN void die_webcgi(const char *err, va_list params)\n {\n-\tstatic int dead;\n+\tif (dead <= 1) {\n+\t\tvreportf(\"fatal: \", err, params);\n \n-\tif (!dead) {\n-\t\tdead = 1;\n \t\thttp_status(500, \"Internal Server Error\");\n \t\thdr_nocache();\n \t\tend_headers();\n-\n-\t\tvreportf(\"fatal: \", err, params);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\n \n+static int die_webcgi_recursing(void)\n+{\n+\treturn dead++ > 1;\n+}\n+\n static char* getdir(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -569,6 +572,7 @@ int main(int argc, char **argv)\n \n \tgit_extract_argv0_path(argv[0]);\n \tset_die_routine(die_webcgi);\n+\tset_die_is_recursing_routine(die_webcgi_recursing);\n \n \tif (!method)\n \t\tdie(\"No REQUEST_METHOD from server\");\n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"261325","messageId":"20150515063339.GB30890@peff.net","threadId":"39330","inReplyTo":"20150515062901.GA30768@peff.net","subject":"[PATCH 2/2] http-backend: spool ref negotiation requests to buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T06:33:39Z","receivedAt":"2015-05-15T06:33:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When http-backend spawns \"upload-pack\" to do ref\nnegotiation, it streams the http request body to\nupload-pack, who then streams the http response back to the\nclient as it reads. In theory, git can go full-duplex; the\nclient can consume our response while it is still sending\nthe request.  In practice, however, HTTP is a half-duplex\nprotocol. Even if our client is ready to read and write\nsimultaneously, we may have other HTTP infrastructure in the\nway, including the webserver that spawns our CGI, or any\nintermediate proxies.\n\nIn at least one documented case[1], this leads to deadlock\nwhen trying a fetch over http. What happens is basically:\n\n  1. Apache proxies the request to the CGI, http-backend.\n\n  2. http-backend gzip-inflates the data and sends\n     the result to upload-pack.\n\n  3. upload-pack acts on the data and generates output over\n     the pipe back to Apache. Apache isn't reading because\n     it's busy writing (step 1).\n\nThis works fine most of the time, because the upload-pack\noutput ends up in a system pipe buffer, and Apache reads\nit as soon as it finishes writing. But if both the request\nand the response exceed the system pipe buffer size, then we\ndeadlock (Apache blocks writing to http-backend,\nhttp-backend blocks writing to upload-pack, and upload-pack\nblocks writing to Apache).\n\nWe need to break the deadlock by spooling either the input\nor the output. In this case, it's ideal to spool the input,\nbecause Apache does not start reading either stdout _or_\nstderr until we have consumed all of the input. So until we\ndo so, we cannot even get an error message out to the\nclient.\n\nThe solution is fairly straight-forward: we read the request\nbody into an in-memory buffer in http-backend, freeing up\nApache, and then feed the data ourselves to upload-pack. But\nthere are a few important things to note:\n\n  1. We limit in-memory buffer to no larger than 1 megabyte\n     to prevent an obvious denial-of-service attack. This\n     is a new hard limit on requests, but it's likely that\n     requests of this size didn't work before at all (i.e.,\n     they would have run into the pipe buffer thing and\n     deadlocked).\n\n  2. We must take care only to buffer when we have to. For\n     pushes, the incoming packfile may be of arbitrary\n     size, and we should connect the input directly to\n     receive-pack. There's no deadlock problem here, though,\n     because we do not produce any output until the whole\n     packfile has been read.\n\n     For upload-pack's initial ref advertisement, we\n     similarly do not need to buffer. Even though we may\n     generate a lot of output, there is no request body at\n     all (i.e., it is a GET, not a POST).\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/269020\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-backend.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 77 insertions(+), 11 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 3ad82a8..7cc2e8c 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -19,12 +19,13 @@ static struct string_list *query_params;\n struct rpc_service {\n \tconst char *name;\n \tconst char *config_name;\n+\tunsigned buffer_input : 1;\n \tsigned enabled : 2;\n };\n \n static struct rpc_service rpc_service[] = {\n-\t{ \"upload-pack\", \"uploadpack\", 1 },\n-\t{ \"receive-pack\", \"receivepack\", -1 },\n+\t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n+\t{ \"receive-pack\", \"receivepack\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -266,9 +267,49 @@ static struct rpc_service *select_service(const char *name)\n \treturn svc;\n }\n \n-static void inflate_request(const char *prog_name, int out)\n+/*\n+ * This is basically strbuf_read(), except that if we\n+ * hit MAX_REQUEST_BUFFER we die (we'd rather reject a\n+ * maliciously large request than chew up infinite memory).\n+ */\n+#define MAX_REQUEST_BUFFER (1024 * 1024)\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tsize_t len = 0, alloc = 8192;\n+\tunsigned char *buf = xmalloc(alloc);\n+\n+\twhile (1) {\n+\t\tssize_t cnt;\n+\n+\t\tcnt = read_in_full(fd, buf + len, alloc - len);\n+\t\tif (cnt < 0) {\n+\t\t\tfree(buf);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\t/* partial read from read_in_full means we hit EOF */\n+\t\tlen += cnt;\n+\t\tif (len < alloc) {\n+\t\t\t*out = buf;\n+\t\t\treturn len;\n+\t\t}\n+\n+\t\t/* otherwise, grow and try again (if we can) */\n+\t\tif (alloc == MAX_REQUEST_BUFFER)\n+\t\t\tdie(\"request was larger than our maximum size (%lu)\",\n+\t\t\t    (unsigned long)(MAX_REQUEST_BUFFER-1));\n+\n+\t\talloc = alloc_nr(alloc);\n+\t\tif (alloc > MAX_REQUEST_BUFFER)\n+\t\t\talloc = MAX_REQUEST_BUFFER;\n+\t\tREALLOC_ARRAY(buf, alloc);\n+\t}\n+}\n+\n+static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n+\tunsigned char *full_request = NULL;\n \tunsigned char in_buf[8192];\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n@@ -277,11 +318,21 @@ static void inflate_request(const char *prog_name, int out)\n \tgit_inflate_init_gzip_only(&stream);\n \n \twhile (1) {\n-\t\tssize_t n = xread(0, in_buf, sizeof(in_buf));\n+\t\tssize_t n;\n+\n+\t\tif (buffer_input) {\n+\t\t\tif (full_request)\n+\t\t\t\tn = 0; /* nothing left to read */\n+\t\t\telse\n+\t\t\t\tn = read_request(0, &full_request);\n+\t\t\tstream.next_in = full_request;\n+\t\t} else {\n+\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n+\t\t\tstream.next_in = in_buf;\n+\t\t}\n+\n \t\tif (n <= 0)\n \t\t\tdie(\"request ended in the middle of the gzip stream\");\n-\n-\t\tstream.next_in = in_buf;\n \t\tstream.avail_in = n;\n \n \t\twhile (0 < stream.avail_in) {\n@@ -307,9 +358,22 @@ static void inflate_request(const char *prog_name, int out)\n done:\n \tgit_inflate_end(&stream);\n \tclose(out);\n+\tfree(full_request);\n }\n \n-static void run_service(const char **argv)\n+static void copy_request(const char *prog_name, int out)\n+{\n+\tunsigned char *buf;\n+\tssize_t n = read_request(0, &buf);\n+\tif (n < 0)\n+\t\tdie_errno(\"error reading request body\");\n+\tif (write_in_full(out, buf, n) != n)\n+\t\tdie(\"%s aborted reading request\", prog_name);\n+\tclose(out);\n+\tfree(buf);\n+}\n+\n+static void run_service(const char **argv, int buffer_input)\n {\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n \tconst char *user = getenv(\"REMOTE_USER\");\n@@ -334,7 +398,7 @@ static void run_service(const char **argv)\n \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tif (gzipped_request)\n+\tif (buffer_input || gzipped_request)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n \tif (start_command(&cld))\n@@ -342,7 +406,9 @@ static void run_service(const char **argv)\n \n \tclose(1);\n \tif (gzipped_request)\n-\t\tinflate_request(argv[0], cld.in);\n+\t\tinflate_request(argv[0], cld.in, buffer_input);\n+\telse if (buffer_input)\n+\t\tcopy_request(argv[0], cld.in);\n \telse\n \t\tclose(0);\n \n@@ -392,7 +458,7 @@ static void get_info_refs(char *arg)\n \t\tpacket_flush(1);\n \n \t\targv[0] = svc->name;\n-\t\trun_service(argv);\n+\t\trun_service(argv, 0);\n \n \t} else {\n \t\tselect_getanyfile();\n@@ -496,7 +562,7 @@ static void service_rpc(char *service_name)\n \tend_headers();\n \n \targv[0] = svc->name;\n-\trun_service(argv);\n+\trun_service(argv, svc->buffer_input);\n \tstrbuf_release(&buf);\n }\n \n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"261326","messageId":"1431675680.14042.39.camel@kaarsemaker.net","threadId":"39330","inReplyTo":"20150515062901.GA30768@peff.net","subject":"Re: [PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-05-15T07:41:20Z","receivedAt":"2015-05-15T07:41:20Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On vr, 2015-05-15 at 02:29 -0400, Jeff King wrote:\n> On Wed, May 13, 2015 at 08:47:24PM -0400, Jeff King wrote:\n> \n> > The fundamental problem is the deadlock on the server side, which is\n> > producing bogus protocol output. And that's a mismatch between what\n> > Apache expects (that the CGI will read all of the input request and then\n> > generate an output request) and what the CGI wants to do (stream output\n> > as it reads the input).\n> \n> At first I was irritated with Apache for this. But thinking on it more,\n> it's really due to our shoe-horning of a full-duplex protocol into the\n> half-duplex HTTP protocol. Even if we could convince Apache to work in a\n> full-duplex way here, and even if our client is full-duplex (since\n> otherwise we are just trading pipe buffers for TCP buffers), we still\n> may face arbitrary HTTP proxies or other infrastructure in the middle.\n> \n> So here's a series to try to address the issue. The first patch is a\n> fixed version of the die-recursion fixup I posted earlier. The second is\n> the interesting one.\n> \n>   [1/2]: http-backend: fix die recursion with custom handler\n>   [2/2]: http-backend: spool ref negotiation requests to buffer\n> \n> I have no clue how to write a test that would trigger this reliably\n> without requiring a gigantic test fixture. However, I did confirm that\n> it fixes the problem on the chromium case you provided (which otherwise\n> deadlocks reliably for me).\n\nThis looks similar to the failure I posted about alst year in\nhttp://thread.gmane.org/gmane.comp.version-control.git/258514 \n\nThough the issue is different, it has the same 'hanging git fetch'\nsymptom due to the deadlock between upload-pack and http-backend.\n\nThe patch I sent back then is suboptimal, as it can cause larger packs\nthan necessary (we still use it though, as the alternative is a\nnon-working git), but it does include a test you may be able to use to\nverify your fix, if this is indeed the same issue.\n\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"261328","messageId":"20150515083843.GA16910@peff.net","threadId":"39330","inReplyTo":"1431675680.14042.39.camel@kaarsemaker.net","subject":"Re: [PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T08:38:43Z","receivedAt":"2015-05-15T08:38:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2015 at 09:41:20AM +0200, Dennis Kaarsemaker wrote:\n\n> > I have no clue how to write a test that would trigger this reliably\n> > without requiring a gigantic test fixture. However, I did confirm that\n> > it fixes the problem on the chromium case you provided (which otherwise\n> > deadlocks reliably for me).\n> \n> This looks similar to the failure I posted about alst year in\n> http://thread.gmane.org/gmane.comp.version-control.git/258514\n> \n> Though the issue is different, it has the same 'hanging git fetch'\n> symptom due to the deadlock between upload-pack and http-backend.\n\nThanks, I think it is the same issue (in the end I was replicating not\nwith `--reference`, but just by doing a fetch from the other\nrepository). And our solutions are essentially the same. I do prefer\nmine because:\n\n  1. It keeps the buffering logic in http-backend; the half-duplex\n     nature is an http detail.\n\n  2. I think it's better to buffer the request rather than the response,\n     for the reasons I stated in the commit message.\n\n> The patch I sent back then is suboptimal, as it can cause larger packs\n> than necessary (we still use it though, as the alternative is a\n> non-working git), but it does include a test you may be able to use to\n> verify your fix, if this is indeed the same issue.\n\nI applied the test from your patch, but couldn't get it to fail even\nwith stock git.  The test above it shrunk a bit, but I was able to tweak\nyours to generate tags from 2001..100000, which I thought would have\nworked.  I suspect it's something silly like the size not being quite\nbig enough for the pipe buffer on my system, or something like that.\nThough I couldn't get it to fail even with 200,000 tags, so perhaps it's\nsomething else.\n\n-Peff\n"},{"id":"261329","messageId":"1431679490.14042.42.camel@kaarsemaker.net","threadId":"39330","inReplyTo":"20150515083843.GA16910@peff.net","subject":"Re: [PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-05-15T08:44:50Z","receivedAt":"2015-05-15T08:44:50Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On vr, 2015-05-15 at 04:38 -0400, Jeff King wrote:\n> On Fri, May 15, 2015 at 09:41:20AM +0200, Dennis Kaarsemaker wrote:\n> \n> > > I have no clue how to write a test that would trigger this reliably\n> > > without requiring a gigantic test fixture. However, I did confirm that\n> > > it fixes the problem on the chromium case you provided (which otherwise\n> > > deadlocks reliably for me).\n> > \n> > This looks similar to the failure I posted about alst year in\n> > http://thread.gmane.org/gmane.comp.version-control.git/258514\n> > \n> > Though the issue is different, it has the same 'hanging git fetch'\n> > symptom due to the deadlock between upload-pack and http-backend.\n> \n> Thanks, I think it is the same issue (in the end I was replicating not\n> with `--reference`, but just by doing a fetch from the other\n> repository). And our solutions are essentially the same. I do prefer\n> mine because:\n> \n>   1. It keeps the buffering logic in http-backend; the half-duplex\n>      nature is an http detail.\n> \n>   2. I think it's better to buffer the request rather than the response,\n>      for the reasons I stated in the commit message.\n> \n> > The patch I sent back then is suboptimal, as it can cause larger packs\n> > than necessary (we still use it though, as the alternative is a\n> > non-working git), but it does include a test you may be able to use to\n> > verify your fix, if this is indeed the same issue.\n> \n> I applied the test from your patch, but couldn't get it to fail even\n> with stock git.  The test above it shrunk a bit, but I was able to tweak\n> yours to generate tags from 2001..100000, which I thought would have\n> worked.  I suspect it's something silly like the size not being quite\n> big enough for the pipe buffer on my system, or something like that.\n> Though I couldn't get it to fail even with 200,000 tags, so perhaps it's\n> something else.\n\nThe shrinkage in the test above it will actually work around the issue,\nas there are now fewer already-fetched tags to negotiate. Either\nreverting that shrinkage or executing the new test twice should do the\ntrick.\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"261330","messageId":"20150515085355.GA18890@peff.net","threadId":"39330","inReplyTo":"1431679490.14042.42.camel@kaarsemaker.net","subject":"Re: [PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T08:53:55Z","receivedAt":"2015-05-15T08:53:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2015 at 10:44:50AM +0200, Dennis Kaarsemaker wrote:\n\n> > I applied the test from your patch, but couldn't get it to fail even\n> > with stock git.  The test above it shrunk a bit, but I was able to tweak\n> > yours to generate tags from 2001..100000, which I thought would have\n> > worked.  I suspect it's something silly like the size not being quite\n> > big enough for the pipe buffer on my system, or something like that.\n> > Though I couldn't get it to fail even with 200,000 tags, so perhaps it's\n> > something else.\n> \n> The shrinkage in the test above it will actually work around the issue,\n> as there are now fewer already-fetched tags to negotiate. Either\n> reverting that shrinkage or executing the new test twice should do the\n> trick.\n\nAh, right, that makes sense. I was creating the right number of tags,\nbut not with half of them already in the repo when I did the critical\nfetch. I got it to fail by adding and fetching another 48,000, and then\nadding and fetching another 50,000 on top of that.\n\nInterestingly, with my patch the _first_ fetch fails, that otherwise\nsucceeds with stock git. My patch sets a maximum size on the spool\nbuffer, and we exceed it. I guess 1MB isn't enough for pathological\ncases. I'm hesitant to let it expand indefinitely for security reasons,\nbut we could probably bump it to 10MB or something.\n\nI dunno. I'm not excited about introducing new size restrictions that\nwere not there before.\n\nMaybe it's time to implement git-over-websockets. ;)\n\n-Peff\n"},{"id":"261332","messageId":"1431681077.14042.44.camel@kaarsemaker.net","threadId":"39330","inReplyTo":"20150515085355.GA18890@peff.net","subject":"Re: [PATCH 0/2] fix http deadlock on giant ref negotiations","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2015-05-15T09:11:17Z","receivedAt":"2015-05-15T09:11:17Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On vr, 2015-05-15 at 04:53 -0400, Jeff King wrote:\n> I guess 1MB isn't enough for pathological cases.\n\n$ git for-each-ref | wc -l \n59658\n\nI'm not saying it's sensible, but this is a real repo I have to work\nwith.\n-- \nDennis Kaarsemaker\nhttp://www.kaarsemaker.net\n"},{"id":"261358","messageId":"xmqqegmhhf9p.fsf@gitster.dls.corp.google.com","threadId":"39330","inReplyTo":"20150515063339.GB30890@peff.net","subject":"Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-15T18:22:42Z","receivedAt":"2015-05-15T18:22:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The solution is fairly straight-forward: we read the request\n> body into an in-memory buffer in http-backend, freeing up\n> Apache, and then feed the data ourselves to upload-pack. But\n> there are a few important things to note:\n>\n>   1. We limit in-memory buffer to no larger than 1 megabyte\n>      to prevent an obvious denial-of-service attack. This\n>      is a new hard limit on requests, but it's likely that\n>      requests of this size didn't work before at all (i.e.,\n>      they would have run into the pipe buffer thing and\n>      deadlocked).\n>\n>   2. We must take care only to buffer when we have to. For\n>      pushes, the incoming packfile may be of arbitrary\n>      size, and we should connect the input directly to\n>      receive-pack. There's no deadlock problem here, though,\n>      because we do not produce any output until the whole\n>      packfile has been read.\n>\n>      For upload-pack's initial ref advertisement, we\n>      similarly do not need to buffer. Even though we may\n>      generate a lot of output, there is no request body at\n>      all (i.e., it is a GET, not a POST).\n\nThanks.\n\nOne unrelated thing I noticed was that three codepaths independently\nhave close(0) in run_service() now, and made me follow the two\nhelper functions to see they both do the close at the end.  It might\nhave made the flow easier to follow if run_service() were\n\n    ...\n    close(1);\n    if (gzip)\n    \tinflate();\n    else if (buffer)\n        copy();\n    close(0);\n    ...\n\nBut that is minor.\n\nAlso, is it worth allocating small and then growing up to the maximum?\nI think this only relays one request at a time anyway, and I suspect\nthat a single 1MB allocation at the first call kept getting reused\nmay be sufficient (and much simpler).\n\n> [1] http://article.gmane.org/gmane.comp.version-control.git/269020\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http-backend.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++--------\n>  1 file changed, 77 insertions(+), 11 deletions(-)\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index 3ad82a8..7cc2e8c 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -19,12 +19,13 @@ static struct string_list *query_params;\n>  struct rpc_service {\n>  \tconst char *name;\n>  \tconst char *config_name;\n> +\tunsigned buffer_input : 1;\n>  \tsigned enabled : 2;\n>  };\n>  \n>  static struct rpc_service rpc_service[] = {\n> -\t{ \"upload-pack\", \"uploadpack\", 1 },\n> -\t{ \"receive-pack\", \"receivepack\", -1 },\n> +\t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n> +\t{ \"receive-pack\", \"receivepack\", 0, -1 },\n>  };\n>  \n>  static struct string_list *get_parameters(void)\n> @@ -266,9 +267,49 @@ static struct rpc_service *select_service(const char *name)\n>  \treturn svc;\n>  }\n>  \n> -static void inflate_request(const char *prog_name, int out)\n> +/*\n> + * This is basically strbuf_read(), except that if we\n> + * hit MAX_REQUEST_BUFFER we die (we'd rather reject a\n> + * maliciously large request than chew up infinite memory).\n> + */\n> +#define MAX_REQUEST_BUFFER (1024 * 1024)\n> +static ssize_t read_request(int fd, unsigned char **out)\n> +{\n> +\tsize_t len = 0, alloc = 8192;\n> +\tunsigned char *buf = xmalloc(alloc);\n> +\n> +\twhile (1) {\n> +\t\tssize_t cnt;\n> +\n> +\t\tcnt = read_in_full(fd, buf + len, alloc - len);\n> +\t\tif (cnt < 0) {\n> +\t\t\tfree(buf);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\t/* partial read from read_in_full means we hit EOF */\n> +\t\tlen += cnt;\n> +\t\tif (len < alloc) {\n> +\t\t\t*out = buf;\n> +\t\t\treturn len;\n> +\t\t}\n> +\n> +\t\t/* otherwise, grow and try again (if we can) */\n> +\t\tif (alloc == MAX_REQUEST_BUFFER)\n> +\t\t\tdie(\"request was larger than our maximum size (%lu)\",\n> +\t\t\t    (unsigned long)(MAX_REQUEST_BUFFER-1));\n> +\n> +\t\talloc = alloc_nr(alloc);\n> +\t\tif (alloc > MAX_REQUEST_BUFFER)\n> +\t\t\talloc = MAX_REQUEST_BUFFER;\n> +\t\tREALLOC_ARRAY(buf, alloc);\n> +\t}\n> +}\n> +\n> +static void inflate_request(const char *prog_name, int out, int buffer_input)\n>  {\n>  \tgit_zstream stream;\n> +\tunsigned char *full_request = NULL;\n>  \tunsigned char in_buf[8192];\n>  \tunsigned char out_buf[8192];\n>  \tunsigned long cnt = 0;\n> @@ -277,11 +318,21 @@ static void inflate_request(const char *prog_name, int out)\n>  \tgit_inflate_init_gzip_only(&stream);\n>  \n>  \twhile (1) {\n> -\t\tssize_t n = xread(0, in_buf, sizeof(in_buf));\n> +\t\tssize_t n;\n> +\n> +\t\tif (buffer_input) {\n> +\t\t\tif (full_request)\n> +\t\t\t\tn = 0; /* nothing left to read */\n> +\t\t\telse\n> +\t\t\t\tn = read_request(0, &full_request);\n> +\t\t\tstream.next_in = full_request;\n> +\t\t} else {\n> +\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n> +\t\t\tstream.next_in = in_buf;\n> +\t\t}\n> +\n>  \t\tif (n <= 0)\n>  \t\t\tdie(\"request ended in the middle of the gzip stream\");\n> -\n> -\t\tstream.next_in = in_buf;\n>  \t\tstream.avail_in = n;\n>  \n>  \t\twhile (0 < stream.avail_in) {\n> @@ -307,9 +358,22 @@ static void inflate_request(const char *prog_name, int out)\n>  done:\n>  \tgit_inflate_end(&stream);\n>  \tclose(out);\n> +\tfree(full_request);\n>  }\n>  \n> -static void run_service(const char **argv)\n> +static void copy_request(const char *prog_name, int out)\n> +{\n> +\tunsigned char *buf;\n> +\tssize_t n = read_request(0, &buf);\n> +\tif (n < 0)\n> +\t\tdie_errno(\"error reading request body\");\n> +\tif (write_in_full(out, buf, n) != n)\n> +\t\tdie(\"%s aborted reading request\", prog_name);\n> +\tclose(out);\n> +\tfree(buf);\n> +}\n> +\n> +static void run_service(const char **argv, int buffer_input)\n>  {\n>  \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n>  \tconst char *user = getenv(\"REMOTE_USER\");\n> @@ -334,7 +398,7 @@ static void run_service(const char **argv)\n>  \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n>  \n>  \tcld.argv = argv;\n> -\tif (gzipped_request)\n> +\tif (buffer_input || gzipped_request)\n>  \t\tcld.in = -1;\n>  \tcld.git_cmd = 1;\n>  \tif (start_command(&cld))\n> @@ -342,7 +406,9 @@ static void run_service(const char **argv)\n>  \n>  \tclose(1);\n>  \tif (gzipped_request)\n> -\t\tinflate_request(argv[0], cld.in);\n> +\t\tinflate_request(argv[0], cld.in, buffer_input);\n> +\telse if (buffer_input)\n> +\t\tcopy_request(argv[0], cld.in);\n>  \telse\n>  \t\tclose(0);\n>  \n> @@ -392,7 +458,7 @@ static void get_info_refs(char *arg)\n>  \t\tpacket_flush(1);\n>  \n>  \t\targv[0] = svc->name;\n> -\t\trun_service(argv);\n> +\t\trun_service(argv, 0);\n>  \n>  \t} else {\n>  \t\tselect_getanyfile();\n> @@ -496,7 +562,7 @@ static void service_rpc(char *service_name)\n>  \tend_headers();\n>  \n>  \targv[0] = svc->name;\n> -\trun_service(argv);\n> +\trun_service(argv, svc->buffer_input);\n>  \tstrbuf_release(&buf);\n>  }\n"},{"id":"261360","messageId":"55563AD5.4090105@linuxfoundation.org","threadId":"39330","inReplyTo":"xmqqegmhhf9p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2015-05-15T18:28:37Z","receivedAt":"2015-05-15T18:28:37Z","isPatch":true,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On 15/05/15 02:22 PM, Junio C Hamano wrote:\n> Also, is it worth allocating small and then growing up to the maximum?\n> I think this only relays one request at a time anyway, and I suspect\n> that a single 1MB allocation at the first call kept getting reused\n> may be sufficient (and much simpler).\n\nDoes it make sense to make that configurable via an env variable at all?\nI suspect the vast majority of people would not hit this bug unless they\nare dealing with repositories polluted with hundreds of refs created by\nautomation (like the codeaurora chromium repo).\n\nE.g. can be set via an Apache directive like\n\nSetEnv FOO_MAX_SIZE 2048\n\nThe backend can then be configured to emit an error message when the\nspool size is exhausted saying \"foo exhausted, increase FOO_MAX_SIZE to\nallow for moar foo.\"\n\n-K\n\n"},{"id":"261368","messageId":"20150515191646.GA29934@peff.net","threadId":"39330","inReplyTo":"xmqqegmhhf9p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] http-backend: spool ref negotiation requests to buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-15T19:16:46Z","receivedAt":"2015-05-15T19:16:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2015 at 11:22:42AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The solution is fairly straight-forward: we read the request\n> > body into an in-memory buffer in http-backend, freeing up\n> > Apache, and then feed the data ourselves to upload-pack. But\n> > there are a few important things to note:\n> >\n> >   1. We limit in-memory buffer to no larger than 1 megabyte\n> >      to prevent an obvious denial-of-service attack. This\n> >      is a new hard limit on requests, but it's likely that\n> >      requests of this size didn't work before at all (i.e.,\n> >      they would have run into the pipe buffer thing and\n> >      deadlocked).\n\nSo this 1MB limit is clearly a problem, and the reasoning above is not\nright. The case we are helping is when a large amount of input creates a\nlarge amount of output. But we're _hurting_ the case where there's just\na large amount of input (as shown by the Dennis's test case).\n\nWhat do we want to do about that? We can switch to streaming after\nhitting our limit (so opening the opportunity for deadlock again in some\ncases, but making sure we do no harm to cases that currently work). Or\nwe can just bump the input size and say \"you'd be crazy to send more\nthan 10MB\" (or 50, or whatever). We could make a configuration knob,\ntoo, I guess.\n\n> One unrelated thing I noticed was that three codepaths independently\n> have close(0) in run_service() now, and made me follow the two\n> helper functions to see they both do the close at the end.  It might\n> have made the flow easier to follow if run_service() were\n> \n>     ...\n>     close(1);\n>     if (gzip)\n>     \tinflate();\n>     else if (buffer)\n>         copy();\n>     close(0);\n>     ...\n> \n> But that is minor.\n\nI don't see the close(0) in the other (buffered) code paths. We close\nthe _output_ to the child, but of course we have to do that to tell it\nwe're done sending (I actually forgot it in an earlier version of\ncopy_request(), and things hang :) ).\n\nI don't think there's any need to close(0) in the buffered cases. We\nread until EOF in the copy() case. For gzip, we read until the end of\nthe gzipped data. I guess it would be better to close if we're not\nexpecting more input, as otherwise Apache might block trying to write to\nus if the client sends bogus input (i.e., a zlib stream with more cruft\nat the end).\n\n> Also, is it worth allocating small and then growing up to the maximum?\n> I think this only relays one request at a time anyway, and I suspect\n> that a single 1MB allocation at the first call kept getting reused\n> may be sufficient (and much simpler).\n\nMy initial attempt did exactly that, but I had a much smaller buffer. I\nstarted to get worried around 1MB. If we bump it to 10MB (or make it\nconfigurable), I get more so. I dunno. It is not _that_ much memory, but\nit is per-request we are serving, so it might add up on a busy server.\nOTOH, pack-objects thinks nothing of allocating 800MB just for the\nbook-keeping to serve a clone of torvalds/linux.\n\n-Peff\n"},{"id":"261636","messageId":"20150520073526.GA16784@peff.net","threadId":"39330","inReplyTo":"55563AD5.4090105@linuxfoundation.org","subject":"[PATCH v2 0/3] fix http deadlock on giant ref negotiations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-20T07:35:26Z","receivedAt":"2015-05-20T07:35:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 15, 2015 at 02:28:37PM -0400, Konstantin Ryabitsev wrote:\n\n> On 15/05/15 02:22 PM, Junio C Hamano wrote:\n> > Also, is it worth allocating small and then growing up to the maximum?\n> > I think this only relays one request at a time anyway, and I suspect\n> > that a single 1MB allocation at the first call kept getting reused\n> > may be sufficient (and much simpler).\n> \n> Does it make sense to make that configurable via an env variable at all?\n> I suspect the vast majority of people would not hit this bug unless they\n> are dealing with repositories polluted with hundreds of refs created by\n> automation (like the codeaurora chromium repo).\n> \n> E.g. can be set via an Apache directive like\n> \n> SetEnv FOO_MAX_SIZE 2048\n> \n> The backend can then be configured to emit an error message when the\n> spool size is exhausted saying \"foo exhausted, increase FOO_MAX_SIZE to\n> allow for moar foo.\"\n\nYeah, that was the same conclusion I came to elsewhere in the thread.\nHere's a re-roll:\n\n  [1/3]: http-backend: fix die recursion with custom handler\n  [2/3]: t5551: factor out tag creation\n  [3/3]: http-backend: spool ref negotiation requests to buffer\n\nIt makes the size configurable (either through the environment, which is\nconvenient for setting via Apache; or through the config, which is\nconvenient if you have one absurdly-sized repo). It mentions the\nvariable name when it barfs into the Apache log. I also bumped the\ndefault to 10MB, which I think should be enough to handle even\nridiculous cases.\n\nI also adapted Dennis's test into the third patch. Beware that it's\nquite slow to run (and is protected by the \"EXPENSIVE\" prerequisite).\nPatch 2 is new, and just refactors the script to make adding the new\ntest easier.\n\nI really wanted to add a test like:\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex e2a2fa1..1fff812 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -273,6 +273,16 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '\n \ttest_line_count = 2 posts\n '\n \n+test_expect_success 'http-backend does not buffer arbitrarily large requests' '\n+\ttest_when_finished \"(\n+\t\tcd \\\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\\\" &&\n+\t\ttest_unconfig http.maxrequestbuffer\n+\t)\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n+\t\tconfig http.maxrequestbuffer 100 &&\n+\ttest_must_fail git clone $HTTPD_URL/smart/repo.git foo.git\n+'\n+\n test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '\n \t(\n \t\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n\nto test that the maxRequestBuffer code does indeed work. Unfortunately,\neven though the server behaves reasonably in this case, the client ends\nup hanging forever. I'm not sure there is a simple solution to that; I\nthink it is a protocol issue where remote-http is waiting for fetch-pack\nto speak, but fetch-pack is waiting for more data from the remote.\n\nPersonally, I think I'd be much more interested in pursuing a saner,\nfull duplex http solution like git-over-websockets than trying to iron\nout all of the corner cases in the stateless-rpc protocol.\n\n-Peff\n"},{"id":"261637","messageId":"20150520073635.GA9818@peff.net","threadId":"39330","inReplyTo":"20150520073526.GA16784@peff.net","subject":"[PATCH v2 1/3] http-backend: fix die recursion with custom handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-20T07:36:35Z","receivedAt":"2015-05-20T07:36:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we die() in http-backend, we call a custom handler that\nwrites an HTTP 500 response to stdout, then reports the\nerror to stderr. Our routines for writing out the HTTP\nresponse may themselves die, leading to us entering die()\nagain.\n\nWhen it was originally written, that was OK; our custom\nhandler keeps a variable to notice this and does not\nrecurse. However, since cd163d4 (usage.c: detect recursion\nin die routines and bail out immediately, 2012-11-14), the\nmain die() implementation detects recursion before we even\nget to our custom handler, and bails without printing\nanything useful.\n\nWe can handle this case by doing two things:\n\n  1. Installing a custom die_is_recursing handler that\n     allows us to enter up to one level of recursion. Only\n     the first call to our custom handler will try to write\n     out the error response. So if we die again, that is OK.\n     If we end up dying more than that, it is a sign that we\n     are in an infinite recursion.\n\n  2. Reporting the error to stderr before trying to write\n     out the HTTP response. In the current code, if we do\n     die() trying to write out the response, we'll exit\n     immediately from this second die(), and never get a\n     chance to output the original error (which is almost\n     certainly the more interesting one; the second die is\n     just going to be along the lines of \"I tried to write\n     to stdout but it was closed\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-backend.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex b6c0484..3ad82a8 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -500,21 +500,24 @@ static void service_rpc(char *service_name)\n \tstrbuf_release(&buf);\n }\n \n+static int dead;\n static NORETURN void die_webcgi(const char *err, va_list params)\n {\n-\tstatic int dead;\n+\tif (dead <= 1) {\n+\t\tvreportf(\"fatal: \", err, params);\n \n-\tif (!dead) {\n-\t\tdead = 1;\n \t\thttp_status(500, \"Internal Server Error\");\n \t\thdr_nocache();\n \t\tend_headers();\n-\n-\t\tvreportf(\"fatal: \", err, params);\n \t}\n \texit(0); /* we successfully reported a failure ;-) */\n }\n \n+static int die_webcgi_recursing(void)\n+{\n+\treturn dead++ > 1;\n+}\n+\n static char* getdir(void)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -569,6 +572,7 @@ int main(int argc, char **argv)\n \n \tgit_extract_argv0_path(argv[0]);\n \tset_die_routine(die_webcgi);\n+\tset_die_is_recursing_routine(die_webcgi_recursing);\n \n \tif (!method)\n \t\tdie(\"No REQUEST_METHOD from server\");\n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"261638","messageId":"20150520073643.GB9818@peff.net","threadId":"39330","inReplyTo":"20150520073526.GA16784@peff.net","subject":"[PATCH v2 2/3] t5551: factor out tag creation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-20T07:36:43Z","receivedAt":"2015-05-20T07:36:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of our tests in t5551 creates a large number of tags,\nand jumps through some hoops to do it efficiently. Let's\nfactor that out into a function so we can make other similar\ntests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5551-http-fetch-smart.sh | 34 +++++++++++++++++++++-------------\n 1 file changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 66439e5..c83cdf4 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -224,27 +224,35 @@ test_expect_success 'transfer.hiderefs works over smart-http' '\n \tgit -C hidden.git rev-parse --verify b\n '\n \n-test_expect_success 'create 2,000 tags in the repo' '\n-\t(\n-\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in $(test_seq 2000)\n+# create an arbitrary number of tags, numbered from tag-$1 to tag-$2\n+create_tags () {\n+\trm -f marks &&\n+\tfor i in $(test_seq \"$1\" \"$2\")\n \tdo\n-\t\techo \"commit refs/heads/too-many-refs\"\n-\t\techo \"mark :$i\"\n-\t\techo \"committer git <git@example.com> $i +0000\"\n-\t\techo \"data 0\"\n-\t\techo \"M 644 inline bla.txt\"\n-\t\techo \"data 4\"\n-\t\techo \"bla\"\n+\t\t# don't use here-doc, because it requires a process\n+\t\t# per loop iteration\n+\t\techo \"commit refs/heads/too-many-refs-$1\" &&\n+\t\techo \"mark :$i\" &&\n+\t\techo \"committer git <git@example.com> $i +0000\" &&\n+\t\techo \"data 0\" &&\n+\t\techo \"M 644 inline bla.txt\" &&\n+\t\techo \"data 4\" &&\n+\t\techo \"bla\" &&\n \t\t# make every commit dangling by always\n \t\t# rewinding the branch after each commit\n-\t\techo \"reset refs/heads/too-many-refs\"\n-\t\techo \"from :1\"\n+\t\techo \"reset refs/heads/too-many-refs-$1\" &&\n+\t\techo \"from :$1\"\n \tdone | git fast-import --export-marks=marks &&\n \n \t# now assign tags to all the dangling commits we created above\n \ttag=$(perl -e \"print \\\"bla\\\" x 30\") &&\n \tsed -e \"s|^:\\([^ ]*\\) \\(.*\\)$|\\2 refs/tags/$tag-\\1|\" <marks >>packed-refs\n+}\n+\n+test_expect_success 'create 2,000 tags in the repo' '\n+\t(\n+\t\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t\tcreate_tags 1 2000\n \t)\n '\n \n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"261639","messageId":"20150520073709.GC9818@peff.net","threadId":"39330","inReplyTo":"20150520073526.GA16784@peff.net","subject":"[PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-20T07:37:09Z","receivedAt":"2015-05-20T07:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When http-backend spawns \"upload-pack\" to do ref\nnegotiation, it streams the http request body to\nupload-pack, who then streams the http response back to the\nclient as it reads. In theory, git can go full-duplex; the\nclient can consume our response while it is still sending\nthe request.  In practice, however, HTTP is a half-duplex\nprotocol. Even if our client is ready to read and write\nsimultaneously, we may have other HTTP infrastructure in the\nway, including the webserver that spawns our CGI, or any\nintermediate proxies.\n\nIn at least one documented case[1], this leads to deadlock\nwhen trying a fetch over http. What happens is basically:\n\n  1. Apache proxies the request to the CGI, http-backend.\n\n  2. http-backend gzip-inflates the data and sends\n     the result to upload-pack.\n\n  3. upload-pack acts on the data and generates output over\n     the pipe back to Apache. Apache isn't reading because\n     it's busy writing (step 1).\n\nThis works fine most of the time, because the upload-pack\noutput ends up in a system pipe buffer, and Apache reads\nit as soon as it finishes writing. But if both the request\nand the response exceed the system pipe buffer size, then we\ndeadlock (Apache blocks writing to http-backend,\nhttp-backend blocks writing to upload-pack, and upload-pack\nblocks writing to Apache).\n\nWe need to break the deadlock by spooling either the input\nor the output. In this case, it's ideal to spool the input,\nbecause Apache does not start reading either stdout _or_\nstderr until we have consumed all of the input. So until we\ndo so, we cannot even get an error message out to the\nclient.\n\nThe solution is fairly straight-forward: we read the request\nbody into an in-memory buffer in http-backend, freeing up\nApache, and then feed the data ourselves to upload-pack. But\nthere are a few important things to note:\n\n  1. We limit the in-memory buffer to prevent an obvious\n     denial-of-service attack. This is a new hard limit on\n     requests, but it's unlikely to come into play. The\n     default value is 10MB, which covers even the ridiculous\n     100,000-ref negotation in the included test (that\n     actually caps out just over 5MB). But it's configurable\n     on the off chance that you don't mind spending some\n     extra memory to make even ridiculous requests work.\n\n  2. We must take care only to buffer when we have to. For\n     pushes, the incoming packfile may be of arbitrary\n     size, and we should connect the input directly to\n     receive-pack. There's no deadlock problem here, though,\n     because we do not produce any output until the whole\n     packfile has been read.\n\n     For upload-pack's initial ref advertisement, we\n     similarly do not need to buffer. Even though we may\n     generate a lot of output, there is no request body at\n     all (i.e., it is a GET, not a POST).\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/269020\n\nTest-adapted-from: Dennis Kaarsemaker <dennis@kaarsemaker.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-http-backend.txt |  9 ++++\n http-backend.c                     | 97 +++++++++++++++++++++++++++++++++-----\n t/t5551-http-fetch-smart.sh        | 15 ++++++\n 3 files changed, 110 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt\nindex d422ba4..8c6acbe 100644\n--- a/Documentation/git-http-backend.txt\n+++ b/Documentation/git-http-backend.txt\n@@ -255,6 +255,15 @@ The GIT_HTTP_EXPORT_ALL environmental variable may be passed to\n 'git-http-backend' to bypass the check for the \"git-daemon-export-ok\"\n file in each repository before allowing export of that repository.\n \n+The `GIT_HTTP_MAX_REQUEST_BUFFER` environment variable (or the\n+`http.maxRequestBuffer` config variable) may be set to change the\n+largest ref negotiation request that git will handle during a fetch; any\n+fetch requiring a larger buffer will not succeed.  This value should not\n+normally need to be changed, but may be helpful if you are fetching from\n+a repository with an extremely large number of refs.  The value can be\n+specified with a unit (e.g., `100M` for 100 megabytes). The default is\n+10 megabytes.\n+\n The backend process sets GIT_COMMITTER_NAME to '$REMOTE_USER' and\n GIT_COMMITTER_EMAIL to '$\\{REMOTE_USER}@http.$\\{REMOTE_ADDR\\}',\n ensuring that any reflogs created by 'git-receive-pack' contain some\ndiff --git a/http-backend.c b/http-backend.c\nindex 3ad82a8..d1333b8 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -13,18 +13,20 @@ static const char content_type[] = \"Content-Type\";\n static const char content_length[] = \"Content-Length\";\n static const char last_modified[] = \"Last-Modified\";\n static int getanyfile = 1;\n+static unsigned long max_request_buffer = 10 * 1024 * 1024;\n \n static struct string_list *query_params;\n \n struct rpc_service {\n \tconst char *name;\n \tconst char *config_name;\n+\tunsigned buffer_input : 1;\n \tsigned enabled : 2;\n };\n \n static struct rpc_service rpc_service[] = {\n-\t{ \"upload-pack\", \"uploadpack\", 1 },\n-\t{ \"receive-pack\", \"receivepack\", -1 },\n+\t{ \"upload-pack\", \"uploadpack\", 1, 1 },\n+\t{ \"receive-pack\", \"receivepack\", 0, -1 },\n };\n \n static struct string_list *get_parameters(void)\n@@ -225,6 +227,7 @@ static void http_config(void)\n \tstruct strbuf var = STRBUF_INIT;\n \n \tgit_config_get_bool(\"http.getanyfile\", &getanyfile);\n+\tgit_config_get_ulong(\"http.maxrequestbuffer\", &max_request_buffer);\n \n \tfor (i = 0; i < ARRAY_SIZE(rpc_service); i++) {\n \t\tstruct rpc_service *svc = &rpc_service[i];\n@@ -266,9 +269,53 @@ static struct rpc_service *select_service(const char *name)\n \treturn svc;\n }\n \n-static void inflate_request(const char *prog_name, int out)\n+/*\n+ * This is basically strbuf_read(), except that if we\n+ * hit max_request_buffer we die (we'd rather reject a\n+ * maliciously large request than chew up infinite memory).\n+ */\n+static ssize_t read_request(int fd, unsigned char **out)\n+{\n+\tsize_t len = 0, alloc = 8192;\n+\tunsigned char *buf = xmalloc(alloc);\n+\n+\tif (max_request_buffer < alloc)\n+\t\tmax_request_buffer = alloc;\n+\n+\twhile (1) {\n+\t\tssize_t cnt;\n+\n+\t\tcnt = read_in_full(fd, buf + len, alloc - len);\n+\t\tif (cnt < 0) {\n+\t\t\tfree(buf);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\t/* partial read from read_in_full means we hit EOF */\n+\t\tlen += cnt;\n+\t\tif (len < alloc) {\n+\t\t\t*out = buf;\n+\t\t\twarning(\"request size was %lu\", (unsigned long)len);\n+\t\t\treturn len;\n+\t\t}\n+\n+\t\t/* otherwise, grow and try again (if we can) */\n+\t\tif (alloc == max_request_buffer)\n+\t\t\tdie(\"request was larger than our maximum size (%lu);\"\n+\t\t\t    \" try setting GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t    max_request_buffer);\n+\n+\t\talloc = alloc_nr(alloc);\n+\t\tif (alloc > max_request_buffer)\n+\t\t\talloc = max_request_buffer;\n+\t\tREALLOC_ARRAY(buf, alloc);\n+\t}\n+}\n+\n+static void inflate_request(const char *prog_name, int out, int buffer_input)\n {\n \tgit_zstream stream;\n+\tunsigned char *full_request = NULL;\n \tunsigned char in_buf[8192];\n \tunsigned char out_buf[8192];\n \tunsigned long cnt = 0;\n@@ -277,11 +324,21 @@ static void inflate_request(const char *prog_name, int out)\n \tgit_inflate_init_gzip_only(&stream);\n \n \twhile (1) {\n-\t\tssize_t n = xread(0, in_buf, sizeof(in_buf));\n+\t\tssize_t n;\n+\n+\t\tif (buffer_input) {\n+\t\t\tif (full_request)\n+\t\t\t\tn = 0; /* nothing left to read */\n+\t\t\telse\n+\t\t\t\tn = read_request(0, &full_request);\n+\t\t\tstream.next_in = full_request;\n+\t\t} else {\n+\t\t\tn = xread(0, in_buf, sizeof(in_buf));\n+\t\t\tstream.next_in = in_buf;\n+\t\t}\n+\n \t\tif (n <= 0)\n \t\t\tdie(\"request ended in the middle of the gzip stream\");\n-\n-\t\tstream.next_in = in_buf;\n \t\tstream.avail_in = n;\n \n \t\twhile (0 < stream.avail_in) {\n@@ -307,9 +364,22 @@ static void inflate_request(const char *prog_name, int out)\n done:\n \tgit_inflate_end(&stream);\n \tclose(out);\n+\tfree(full_request);\n+}\n+\n+static void copy_request(const char *prog_name, int out)\n+{\n+\tunsigned char *buf;\n+\tssize_t n = read_request(0, &buf);\n+\tif (n < 0)\n+\t\tdie_errno(\"error reading request body\");\n+\tif (write_in_full(out, buf, n) != n)\n+\t\tdie(\"%s aborted reading request\", prog_name);\n+\tclose(out);\n+\tfree(buf);\n }\n \n-static void run_service(const char **argv)\n+static void run_service(const char **argv, int buffer_input)\n {\n \tconst char *encoding = getenv(\"HTTP_CONTENT_ENCODING\");\n \tconst char *user = getenv(\"REMOTE_USER\");\n@@ -334,7 +404,7 @@ static void run_service(const char **argv)\n \t\t\t\t \"GIT_COMMITTER_EMAIL=%s@http.%s\", user, host);\n \n \tcld.argv = argv;\n-\tif (gzipped_request)\n+\tif (buffer_input || gzipped_request)\n \t\tcld.in = -1;\n \tcld.git_cmd = 1;\n \tif (start_command(&cld))\n@@ -342,7 +412,9 @@ static void run_service(const char **argv)\n \n \tclose(1);\n \tif (gzipped_request)\n-\t\tinflate_request(argv[0], cld.in);\n+\t\tinflate_request(argv[0], cld.in, buffer_input);\n+\telse if (buffer_input)\n+\t\tcopy_request(argv[0], cld.in);\n \telse\n \t\tclose(0);\n \n@@ -392,7 +464,7 @@ static void get_info_refs(char *arg)\n \t\tpacket_flush(1);\n \n \t\targv[0] = svc->name;\n-\t\trun_service(argv);\n+\t\trun_service(argv, 0);\n \n \t} else {\n \t\tselect_getanyfile();\n@@ -496,7 +568,7 @@ static void service_rpc(char *service_name)\n \tend_headers();\n \n \targv[0] = svc->name;\n-\trun_service(argv);\n+\trun_service(argv, svc->buffer_input);\n \tstrbuf_release(&buf);\n }\n \n@@ -623,6 +695,9 @@ int main(int argc, char **argv)\n \t\tnot_found(\"Repository not exported: '%s'\", dir);\n \n \thttp_config();\n+\tmax_request_buffer = git_env_ulong(\"GIT_HTTP_MAX_REQUEST_BUFFER\",\n+\t\t\t\t\t   max_request_buffer);\n+\n \tcmd->imp(cmd_arg);\n \treturn 0;\n }\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex c83cdf4..a540c6d 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -273,5 +273,20 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' '\n \ttest_line_count = 2 posts\n '\n \n+test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '\n+\t(\n+\t\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t\tcreate_tags 2001 50000\n+\t) &&\n+\tgit -C too-many-refs fetch -q --tags &&\n+\t(\n+\t\tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t\tcreate_tags 50001 100000\n+\t) &&\n+\tgit -C too-many-refs fetch -q --tags &&\n+\tgit -C too-many-refs for-each-ref refs/tags >tags &&\n+\ttest_line_count = 100000 tags\n+'\n+\n stop_httpd\n test_done\n-- \n2.4.1.396.g7ba6d7b\n"},{"id":"262106","messageId":"CAMwyc-Tn+Sw67z9zTPsPU7SQP7PDzJhoJn30utQ0OWhKe_xbuA@mail.gmail.com","threadId":"39330","inReplyTo":"20150520073709.GC9818@peff.net","subject":"Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2015-05-26T02:07:50Z","receivedAt":"2015-05-26T02:07:50Z","isPatch":true,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On 20 May 2015 at 03:37, Jeff King <peff@peff.net> wrote:\n> +               /* partial read from read_in_full means we hit EOF */\n> +               len += cnt;\n> +               if (len < alloc) {\n> +                       *out = buf;\n> +                       warning(\"request size was %lu\", (unsigned long)len);\n> +                       return len;\n> +               }\n\nJeff:\n\nThis patch appears to work well -- the only complaint I have is that I\nnow have \"warning: request size was NNN\" all over my error logs. :) Is\nit supposed to convey an actual warning message, or is it merely a\ndebug statement?\n\nBest,\n-- \nKonstantin Ryabitsev\nSr. Systems Administrator\nLinux Foundation Collab Projects\n541-224-6067\nMontréal, Québec\n"},{"id":"262108","messageId":"20150526022401.GA1725@peff.net","threadId":"39330","inReplyTo":"CAMwyc-Tn+Sw67z9zTPsPU7SQP7PDzJhoJn30utQ0OWhKe_xbuA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-26T02:24:01Z","receivedAt":"2015-05-26T02:24:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 25, 2015 at 10:07:50PM -0400, Konstantin Ryabitsev wrote:\n\n> On 20 May 2015 at 03:37, Jeff King <peff@peff.net> wrote:\n> > +               /* partial read from read_in_full means we hit EOF */\n> > +               len += cnt;\n> > +               if (len < alloc) {\n> > +                       *out = buf;\n> > +                       warning(\"request size was %lu\", (unsigned long)len);\n> > +                       return len;\n> > +               }\n> \n> Jeff:\n> \n> This patch appears to work well -- the only complaint I have is that I\n> now have \"warning: request size was NNN\" all over my error logs. :) Is\n> it supposed to convey an actual warning message, or is it merely a\n> debug statement?\n\nWhoops, yeah, it was just for debugging. I missed that one when sending\nout the patch.\n\nJunio, the squashable patch is below (on jk/http-backend-deadlock-2.2),\nand it looks like nothing has hit \"next\" yet. But you did do some\nup-merging of the topic. Let me know if you would prefer to just have a\npatch on top.\n\ndiff --git a/http-backend.c b/http-backend.c\nindex d1333b8..6bf139b 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -295,7 +295,6 @@ static ssize_t read_request(int fd, unsigned char **out)\n \t\tlen += cnt;\n \t\tif (len < alloc) {\n \t\t\t*out = buf;\n-\t\t\twarning(\"request size was %lu\", (unsigned long)len);\n \t\t\treturn len;\n \t\t}\n \n\n-Peff\n"},{"id":"262113","messageId":"xmqqk2vw80mr.fsf@gitster.dls.corp.google.com","threadId":"39330","inReplyTo":"20150526022401.GA1725@peff.net","subject":"Re: [PATCH v2 3/3] http-backend: spool ref negotiation requests to buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-26T03:43:08Z","receivedAt":"2015-05-26T03:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Whoops, yeah, it was just for debugging. I missed that one when sending\n> out the patch.\n>\n> Junio, the squashable patch is below (on jk/http-backend-deadlock-2.2),\n> and it looks like nothing has hit \"next\" yet. But you did do some\n> up-merging of the topic. Let me know if you would prefer to just have a\n> patch on top.\n\nSorry, I missed that, too (actually I saw warning() and said \"Heh,\nOK\", without realizing that this is a normal return path).\n\nThanks.\n\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index d1333b8..6bf139b 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -295,7 +295,6 @@ static ssize_t read_request(int fd, unsigned char **out)\n>  \t\tlen += cnt;\n>  \t\tif (len < alloc) {\n>  \t\t\t*out = buf;\n> -\t\t\twarning(\"request size was %lu\", (unsigned long)len);\n>  \t\t\treturn len;\n>  \t\t}\n>  \n>\n> -Peff\n"}]}