{"thread":{"id":"3196","subject":"git-http-fetch failure/segfault -- alas no patch","startedAt":"2006-01-30T20:34:16Z","lastAt":"2006-01-31T17:58:13Z","messageCount":4,"participants":["Mark Wooding","Uwe Zeisberger","Nick Hengeveld"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"15279","messageId":"17374.30792.548889.344768@metalzone.distorted.org.uk","threadId":"3196","inReplyTo":null,"subject":"git-http-fetch failure/segfault -- alas no patch","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-01-30T20:34:16Z","receivedAt":"2006-01-30T20:34:16Z","isPatch":false,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"[This is a repost -- sorry!  I fouled up the last one by missing the\nsubject off, which makes me look like a prize pillock.  I also failed to\nmention the relevant versions, so let me try again.  I do actually have\na brain, honest; it's just not working at the moment for some reason.]\n\ngit-http-fetch seems buggy.  (This is in both 1.1.5 and 1506fc34... on\nthe git.kernel.org master -- same symptoms.)\n\nI /think/ it's getting confused by a combination of a large top-level\ntree (lots of blobs directly attached) attached to the top commit,\ntogether with most of the things being packed.\n\nTo illustrate the bug, create a repository with the following shell\nscript.  (It will create a working tree called `funt' with a little GIT\nhistory inside.)\n\n----\n#! /bin/sh\n\nset -e\nmkdir funt\ncd funt\ngit-init-db \nyes | nl | head -200 | while read n hunoz; do echo file $n >foo.$n; done\ngit-add *\necho Boo. | git-commit -F - -a\ngit-repack\ngit-prune-packed \necho Censored >foo.197\necho Ouch. | git-commit -F - -a\ngit-update-server-info\n----\n\nThen put the repository somewhere your web server will let you get to\nit, and try to clone it, say using\n\n  git-clone http://boyle.nsict.org/~mdw/funt.git\n\n(Yes, that repository exists and is live; the server is fairly\nwell-connected.  FWIW, I've put up a tarball of the repository called\n.../funt.git.tar.gz too.)  You ought to be greeted with text like this:\n\n  error: Unable to find b4f495485ca9ae825ec8c504cdcf24652342f43c under\n  http://boyle.nsict.org/~mdw/funt/.git/\n\n  Cannot obtain needed commit b4f495485ca9ae825ec8c504cdcf24652342f43c\n  while processing commit 351c72525b9ee5b2321c65598ce82a4e79015012.\n\nIf you're very lucky, git-http-fetch will segfault.\n\nWhat's going on here?\n\nThink about the repository layout for a bit.  There's a `big' pack file,\nand a little commit.  The commit has an unpacked tree attached, but\nalmost all of its leaves are in the pack.  The commit's parent is\npacked.\n\nSo git-http-fetch starts by filling its queue with prefetches for blob\nobjects which are packed (and so it gets 404s for them).  This is fine.\nHowever! when it comes to collect the parent commit, it realises it\nneeds to fetch the pack list.  Unfortunately, something goes wrong in\nrun_active_slot.  As far as I can make out, the slot used to collect\n.../info/packs is being /reused/ by fill_active_slots (called by\nstep_active_slots) before fetch_indices is returned to.  Since the\nprefetch which got the new slot is for an object which got packed, it\nfails with a 404, which is written back to the slot.  The result is that\nfetch_indices thinks that the pack list doesn't exist (even though it\nactually does, and libcurl fetched it just fine, thanks).  This is\nmarked as a permanent error, and that parent commit can't be found.\n\nThe segfault is I think due to this reuse too, but it only happens\nsometimes and I'm not entirely clear on why.\n\nI'm afraid I don't have a patch.  I've spent a little while trying to\nfix this bug myself, but my changes just seem to cause wedging, or fd\nleaks, or segfaults, or all three, so it's obvious I don't understand\nthe code well enough.\n\n-- [mdw]\n"},{"id":"15288","messageId":"20060131093200.GA5154@informatik.uni-freiburg.de","threadId":"3196","inReplyTo":"17374.30792.548889.344768@metalzone.distorted.org.uk","subject":"Re: git-http-fetch failure/segfault -- alas no patch","fromName":"Uwe Zeisberger","fromEmail":"zeisberg@informatik.uni-freiburg.de","sentAt":"2006-01-31T09:32:00Z","receivedAt":"2006-01-31T09:32:00Z","isPatch":false,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\nMark Wooding wrote:\n> (Yes, that repository exists and is live; the server is fairly\n> well-connected.  FWIW, I've put up a tarball of the repository called\n> .../funt.git.tar.gz too.)  You ought to be greeted with text like this:\n> \n>   error: Unable to find b4f495485ca9ae825ec8c504cdcf24652342f43c under\n>   http://boyle.nsict.org/~mdw/funt/.git/\n> \n>   Cannot obtain needed commit b4f495485ca9ae825ec8c504cdcf24652342f43c\n>   while processing commit 351c72525b9ee5b2321c65598ce82a4e79015012.\n> \n> If you're very lucky, git-http-fetch will segfault.\nI have the same problem here with an other http repo.  I just finished\nbisecting when i got your mail.\n\nThe offending commit is 056211053b7516a57ff7a6dd02f503ecef6fca70:\n\ngit-clone: do not special case dumb http.\n\nBest regards\nUwe\n\n\n-- \nUwe Zeisberger\n\nprimes where sieve (p:xs) = [ x | x<-xs, x `rem` p /= 0 ]; \\\nprimes = map head (iterate sieve [2..])\n"},{"id":"15293","messageId":"17375.20010.283458.869140@metalzone.distorted.org.uk","threadId":"3196","inReplyTo":"20060131093200.GA5154@informatik.uni-freiburg.de","subject":"Re: git-http-fetch failure/segfault -- alas no patch","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-01-31T11:46:50Z","receivedAt":"2006-01-31T11:46:50Z","isPatch":false,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Uwe Zeisberger <zeisberg@informatik.uni-freiburg.de> wrote:\n\n> The offending commit is 056211053b7516a57ff7a6dd02f503ecef6fca70:\n> \n> git-clone: do not special case dumb http.\n\nFine, except that it's quite clear to me that the bug is actually in\ngit-http-fetch -- it shouldn't segfault, dammit! -- and that commit\naffects only git-clone.sh.  Besides, I get the same symptoms running\ngit-http-fetch by hand.  That commit appears if you bisect on a test\nbased on git-clone because before then it fetched the pack list and pack\nfiles before it tried to mop up the remaining objects, which obviously\nhides the underlying bug.\n\nHere's my low-level test-case.\n\n----\n#! /bin/sh\n\nset -e\nrm -rf funt\nGIT_DIR=funt; export GIT_DIR\n./git-init-db\n./git-http-fetch -a -v \\\n  heads/master http://boyle.nsict.org/~mdw/git/catacomb.broken/\n----\n\nThis is the `real-life' repository which has broken.  I'm using it\nbecause the patch I made /works/ on the trivial `funt' example, but\nfails miserably on the real thing.\n\nBisecting with this test lays the blame on 1d389ab65dc6867d30...  Which\nI think is just telling me that it's always been broken.\n\nFor amusement value, my patch is below.  Here's what happens if I try to\nuse it.\n\n----\ngot 7c40480318648672af86e03bc72bc45c07194c37\nwalk 7c40480318648672af86e03bc72bc45c07194c37\nGetting alternates list for\nhttp://boyle.nsict.org/~mdw/git/catacomb.broken/\ngot 0fd8bd4823315feb1c537a154efba002c053ed8e\nGetting pack list for http://boyle.nsict.org/~mdw/git/catacomb.broken/\nGetting index for pack f6d543b58ba9183c53ddbd981835f0378bdab919\nGetting pack f6d543b58ba9183c53ddbd981835f0378bdab919\n which contains a137c6b3695c32ea9c42520a387a85641687662f\nwalk a137c6b3695c32ea9c42520a387a85641687662f\nerror: fd leakage in release: 4\nerror: fd leakage in release: 9\nerror: fd leakage in release: 5\nerror: fd leakage in release: 7\ngot 53124000638e53ee8aa51830fab72d8a0ce654e4\nerror: Couldn't find request for d2f2768351d41f80469240d1b533575d833c7370 in the queue\nerror: Unable to find d2f2768351d41f80469240d1b533575d833c7370 under http://boyle.nsict.org/~mdw/git/catacomb.broken/\n\nCannot obtain needed blob d2f2768351d41f80469240d1b533575d833c7370\nwhile processing commit a137c6b3695c32ea9c42520a387a85641687662f.\nWaiting for\nhttp://boyle.nsict.org/~mdw/git/catacomb.broken/objects/eb/2dc153b8981ad02430bc166b925ef2457c7c36\ngot eb2dc153b8981ad02430bc166b925ef2457c7c36\nSegmentation fault      (core dumped)\n----\n\nWithout the patch, I get\n\n----\ngot 7c40480318648672af86e03bc72bc45c07194c37\nwalk 7c40480318648672af86e03bc72bc45c07194c37\nGetting alternates list for\nhttp://boyle.nsict.org/~mdw/git/catacomb.broken/\ngot 0fd8bd4823315feb1c537a154efba002c053ed8e\nGetting pack list for http://boyle.nsict.org/~mdw/git/catacomb.broken/\ngot 53124000638e53ee8aa51830fab72d8a0ce654e4\ngot eb2dc153b8981ad02430bc166b925ef2457c7c36\nerror: Unable to find a137c6b3695c32ea9c42520a387a85641687662f under http://boyle.nsict.org/~mdw/git/catacomb.broken/\n----\n\nAnd the patch itself.  Warning!  This DOES NOT WORK!  (But it may be of\nsome use to you lot anyway.)\n\ngit-http-fetch: Fix failure to find pack list.\n\nFrom:  <mdw@distorted.org.uk>\n\n  * Problem seems to be premature reuse of a slot.  In some cases, we\n    end up fetching the exit status for a different fetch; if that fetch\n    fails (because the object is already in a pack) then the pack list\n    fetch is assumed to fail and the entire fetch falls apart.\n---\n\n http.c   |   49 +++++++++++++++++++++++++++++++++++++++----------\n http.h   |    8 +++++++-\n 2 files changed, 46 insertions(+), 11 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 75e6717..54aa9ee 100644\n--- a/http.c\n+++ b/http.c\n@@ -1,3 +1,4 @@\n+#include <assert.h>\n #include \"http.h\"\n \n int data_received;\n@@ -192,6 +193,9 @@ static CURL* get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);\n \n+\tif (getenv(\"GIT_CURL_VERBOSE\"))\n+\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n+\n \treturn result;\n }\n \n@@ -262,7 +266,7 @@ void http_cleanup(void)\n \n \twhile (slot != NULL) {\n #ifdef USE_CURL_MULTI\n-\t\tif (slot->in_use) {\n+\t\tif (slot->in_use & SLOTUSE_ACTIVE) {\n \t\t\tcurl_easy_getinfo(slot->curl,\n \t\t\t\t\t  CURLINFO_EFFECTIVE_URL,\n \t\t\t\t\t  &wait_url);\n@@ -311,6 +315,7 @@ struct active_request_slot *get_active_s\n \t\tnewslot->curl = NULL;\n \t\tnewslot->in_use = 0;\n \t\tnewslot->next = NULL;\n+\t\tnewslot->nrefs = 0;\n \n \t\tslot = active_queue_head;\n \t\tif (slot == NULL) {\n@@ -333,7 +338,7 @@ struct active_request_slot *get_active_s\n \t}\n \n \tactive_requests++;\n-\tslot->in_use = 1;\n+\tslot->in_use |= SLOTUSE_ACTIVE;\n \tslot->local = NULL;\n \tslot->callback_data = NULL;\n \tslot->callback_func = NULL;\n@@ -351,8 +356,9 @@ int start_active_slot(struct active_requ\n \n \tif (curlm_result != CURLM_OK &&\n \t    curlm_result != CURLM_CALL_MULTI_PERFORM) {\n-\t\tactive_requests--;\n-\t\tslot->in_use = 0;\n+\t\tslot->in_use &= ~SLOTUSE_ACTIVE;\n+\t\tif (!slot->in_use)\n+\t\t\tactive_requests--;\n \t\treturn 0;\n \t}\n #endif\n@@ -375,6 +381,25 @@ void step_active_slots(void)\n }\n #endif\n \n+void watch_active_slot(struct active_request_slot *slot)\n+{\n+\tif (!slot->in_use)\n+\t\tactive_requests++;\n+\tslot->in_use |= SLOTUSE_REF;\n+\tslot->nrefs++;\n+}\n+\n+void unwatch_active_slot(struct active_request_slot *slot)\n+{\n+\tassert(slot->nrefs);\n+\tslot->nrefs--;\n+\tif (!slot->nrefs) {\n+\t\tslot->in_use &= ~SLOTUSE_REF;\n+\t\tif (!slot->in_use)\n+\t\t\tactive_requests--;\n+\t}\n+}\n+\n void run_active_slot(struct active_request_slot *slot)\n {\n #ifdef USE_CURL_MULTI\n@@ -386,7 +411,8 @@ void run_active_slot(struct active_reque\n \tint max_fd;\n \tstruct timeval select_timeout;\n \n-\twhile (slot->in_use) {\n+\twatch_active_slot(slot);\n+\twhile (slot->in_use & SLOTUSE_ACTIVE) {\n \t\tdata_received = 0;\n \t\tstep_active_slots();\n \n@@ -397,7 +423,7 @@ void run_active_slot(struct active_reque\n \t\t\tlast_pos = current_pos;\n \t\t}\n \n-\t\tif (slot->in_use && !data_received) {\n+\t\tif ((slot->in_use & SLOTUSE_ACTIVE) && !data_received) {\n \t\t\tmax_fd = 0;\n \t\t\tFD_ZERO(&readfds);\n \t\t\tFD_ZERO(&writefds);\n@@ -408,8 +434,9 @@ void run_active_slot(struct active_reque\n \t\t\t       &excfds, &select_timeout);\n \t\t}\n \t}\n+\tunwatch_active_slot(slot);\n #else\n-\twhile (slot->in_use) {\n+\twhile (slot->in_use & SLOTUSE_ACTIVE) {\n \t\tslot->curl_result = curl_easy_perform(slot->curl);\n \t\tfinish_active_slot(slot);\n \t}\n@@ -418,8 +445,10 @@ void run_active_slot(struct active_reque\n \n static void finish_active_slot(struct active_request_slot *slot)\n {\n-        active_requests--;\n-        slot->in_use = 0;\n+\tassert(slot->in_use & SLOTUSE_ACTIVE);\n+        slot->in_use &= ~SLOTUSE_ACTIVE;\n+\tif (!slot->in_use)\n+\t\tactive_requests--;\n         curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);\n  \n         /* Run callback if appropriate */\n@@ -433,7 +462,7 @@ void finish_all_active_slots(void)\n \tstruct active_request_slot *slot = active_queue_head;\n \n \twhile (slot != NULL)\n-\t\tif (slot->in_use) {\n+\t\tif (slot->in_use & SLOTUSE_ACTIVE) {\n \t\t\trun_active_slot(slot);\n \t\t\tslot = active_queue_head;\n \t\t} else {\ndiff --git a/http.h b/http.h\nindex ed4ea33..6913c6a 100644\n--- a/http.h\n+++ b/http.h\n@@ -26,7 +26,8 @@ struct active_request_slot\n {\n \tCURL *curl;\n \tFILE *local;\n-\tint in_use;\n+\tunsigned in_use;\n+\tint nrefs;\n \tCURLcode curl_result;\n \tlong http_code;\n \tvoid *callback_data;\n@@ -34,6 +35,9 @@ struct active_request_slot\n \tstruct active_request_slot *next;\n };\n \n+#define SLOTUSE_ACTIVE 1u\n+#define SLOTUSE_REF 2u\n+\n struct buffer\n {\n         size_t posn;\n@@ -54,6 +58,8 @@ extern struct active_request_slot *get_a\n extern int start_active_slot(struct active_request_slot *slot);\n extern void run_active_slot(struct active_request_slot *slot);\n extern void finish_all_active_slots(void);\n+extern void watch_active_slot(struct active_request_slot *slot);\n+extern void unwatch_active_slot(struct active_request_slot *slot);\n \n #ifdef USE_CURL_MULTI\n extern void fill_active_slots(void);\n\n-- [mdw]\n"},{"id":"15310","messageId":"20060131175813.GD3873@reactrix.com","threadId":"3196","inReplyTo":"17374.30792.548889.344768@metalzone.distorted.org.uk","subject":"Re: git-http-fetch failure/segfault -- alas no patch","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2006-01-31T17:58:13Z","receivedAt":"2006-01-31T17:58:13Z","isPatch":false,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Mon, Jan 30, 2006 at 08:34:16PM +0000, Mark Wooding wrote:\n\n> run_active_slot.  As far as I can make out, the slot used to collect\n> .../info/packs is being /reused/ by fill_active_slots (called by\n> step_active_slots) before fetch_indices is returned to.\n\nGood catch, there were a couple of places where slot result data could\nbe processed after the slot had been reused, with rather unpredictable\nresults.\n\nThere's a patch on the way after a bit more testing.\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"}]}