[PATCH v2 0/3] fix http deadlock on giant ref negotiations
- From
Jeff King <peff@peff.net>
- Date
- May 20, 2015, 07:35 UTC
- Message-ID
- <20150520073526.GA16784@peff.net>
- In-Reply-To
- <55563AD5.4090105@linuxfoundation.org>
On Fri, May 15, 2015 at 02:28:37PM -0400, Konstantin Ryabitsev wrote:
Show 18 quoted lines
> On 15/05/15 02:22 PM, Junio C Hamano wrote: > > Also, is it worth allocating small and then growing up to the maximum? > > I think this only relays one request at a time anyway, and I suspect > > that a single 1MB allocation at the first call kept getting reused > > may be sufficient (and much simpler). > > Does it make sense to make that configurable via an env variable at all? > I suspect the vast majority of people would not hit this bug unless they > are dealing with repositories polluted with hundreds of refs created by > automation (like the codeaurora chromium repo). > > E.g. can be set via an Apache directive like > > SetEnv FOO_MAX_SIZE 2048 > > The backend can then be configured to emit an error message when the > spool size is exhausted saying "foo exhausted, increase FOO_MAX_SIZE to > allow for moar foo."
Yeah, that was the same conclusion I came to elsewhere in the thread. Here's a re-roll:
[1/3]: http-backend: fix die recursion with custom handler [2/3]: t5551: factor out tag creation [3/3]: http-backend: spool ref negotiation requests to buffer
It makes the size configurable (either through the environment, which is convenient for setting via Apache; or through the config, which is convenient if you have one absurdly-sized repo). It mentions the variable name when it barfs into the Apache log. I also bumped the default to 10MB, which I think should be enough to handle even ridiculous cases.
I also adapted Dennis's test into the third patch. Beware that it's quite slow to run (and is protected by the "EXPENSIVE" prerequisite). Patch 2 is new, and just refactors the script to make adding the new test easier.
I really wanted to add a test like:
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index e2a2fa1..1fff812 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -273,6 +273,16 @@ test_expect_success 'large fetch-pack requests can be split across POSTs' ' test_line_count = 2 posts ' +test_expect_success 'http-backend does not buffer arbitrarily large requests' ' + test_when_finished "( + cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" && + test_unconfig http.maxrequestbuffer + )" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ + config http.maxrequestbuffer 100 && + test_must_fail git clone $HTTPD_URL/smart/repo.git foo.git +' + test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' ' ( cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && to test that the maxRequestBuffer code does indeed work. Unfortunately, even though the server behaves reasonably in this case, the client ends up hanging forever. I'm not sure there is a simple solution to that; I think it is a protocol issue where remote-http is waiting for fetch-pack to speak, but fetch-pack is waiting for more data from the remote. Personally, I think I'd be much more interested in pursuing a saner, full duplex http solution like git-over-websockets than trying to iron out all of the corner cases in the stateless-rpc protocol. -Peff