git-http-fetch failure/segfault -- alas no patch

git-http-fetch failure/segfault -- alas no patch

[Mark Wooding]

[This is a repost -- sorry!  I fouled up the last one by missing the
subject off, which makes me look like a prize pillock.  I also failed to
mention the relevant versions, so let me try again.  I do actually have
a brain, honest; it's just not working at the moment for some reason.]

git-http-fetch seems buggy.  (This is in both 1.1.5 and 1506fc34... on
the git.kernel.org master -- same symptoms.)

I /think/ it's getting confused by a combination of a large top-level
tree (lots of blobs directly attached) attached to the top commit,
together with most of the things being packed.

To illustrate the bug, create a repository with the following shell
script.  (It will create a working tree called `funt' with a little GIT
history inside.)

----
#! /bin/sh

set -e
mkdir funt
cd funt
git-init-db 
yes | nl | head -200 | while read n hunoz; do echo file $n >foo.$n; done
git-add *
echo Boo. | git-commit -F - -a
git-repack
git-prune-packed 
echo Censored >foo.197
echo Ouch. | git-commit -F - -a
git-update-server-info
----

Then put the repository somewhere your web server will let you get to
it, and try to clone it, say using

  git-clone http://boyle.nsict.org/~mdw/funt.git

(Yes, that repository exists and is live; the server is fairly
well-connected.  FWIW, I've put up a tarball of the repository called
.../funt.git.tar.gz too.)  You ought to be greeted with text like this:

  error: Unable to find b4f495485ca9ae825ec8c504cdcf24652342f43c under
  http://boyle.nsict.org/~mdw/funt/.git/

  Cannot obtain needed commit b4f495485ca9ae825ec8c504cdcf24652342f43c
  while processing commit 351c72525b9ee5b2321c65598ce82a4e79015012.

If you're very lucky, git-http-fetch will segfault.

What's going on here?

Think about the repository layout for a bit.  There's a `big' pack file,
and a little commit.  The commit has an unpacked tree attached, but
almost all of its leaves are in the pack.  The commit's parent is
packed.

So git-http-fetch starts by filling its queue with prefetches for blob
objects which are packed (and so it gets 404s for them).  This is fine.
However! when it comes to collect the parent commit, it realises it
needs to fetch the pack list.  Unfortunately, something goes wrong in
run_active_slot.  As far as I can make out, the slot used to collect
.../info/packs is being /reused/ by fill_active_slots (called by
step_active_slots) before fetch_indices is returned to.  Since the
prefetch which got the new slot is for an object which got packed, it
fails with a 404, which is written back to the slot.  The result is that
fetch_indices thinks that the pack list doesn't exist (even though it
actually does, and libcurl fetched it just fine, thanks).  This is
marked as a permanent error, and that parent commit can't be found.

The segfault is I think due to this reuse too, but it only happens
sometimes and I'm not entirely clear on why.

I'm afraid I don't have a patch.  I've spent a little while trying to
fix this bug myself, but my changes just seem to cause wedging, or fd
leaks, or segfaults, or all three, so it's obvious I don't understand
the code well enough.

-- [mdw]

Re: git-http-fetch failure/segfault -- alas no patch

[Uwe Zeisberger]

Hello,

Mark Wooding wrote:
> (Yes, that repository exists and is live; the server is fairly
> well-connected.  FWIW, I've put up a tarball of the repository called
> .../funt.git.tar.gz too.)  You ought to be greeted with text like this:
> 
>   error: Unable to find b4f495485ca9ae825ec8c504cdcf24652342f43c under
>   http://boyle.nsict.org/~mdw/funt/.git/
> 
>   Cannot obtain needed commit b4f495485ca9ae825ec8c504cdcf24652342f43c
>   while processing commit 351c72525b9ee5b2321c65598ce82a4e79015012.
> 
> If you're very lucky, git-http-fetch will segfault.
I have the same problem here with an other http repo.  I just finished
bisecting when i got your mail.

The offending commit is 056211053b7516a57ff7a6dd02f503ecef6fca70:

git-clone: do not special case dumb http.

Best regards
Uwe


-- 
Uwe Zeisberger

primes where sieve (p:xs) = [ x | x<-xs, x `rem` p /= 0 ]; \
primes = map head (iterate sieve [2..])

Re: git-http-fetch failure/segfault -- alas no patch

[Mark Wooding]

Uwe Zeisberger <zeisberg@informatik.uni-freiburg.de> wrote:

> The offending commit is 056211053b7516a57ff7a6dd02f503ecef6fca70:
> 
> git-clone: do not special case dumb http.

Fine, except that it's quite clear to me that the bug is actually in
git-http-fetch -- it shouldn't segfault, dammit! -- and that commit
affects only git-clone.sh.  Besides, I get the same symptoms running
git-http-fetch by hand.  That commit appears if you bisect on a test
based on git-clone because before then it fetched the pack list and pack
files before it tried to mop up the remaining objects, which obviously
hides the underlying bug.

Here's my low-level test-case.

----
#! /bin/sh

set -e
rm -rf funt
GIT_DIR=funt; export GIT_DIR
./git-init-db
./git-http-fetch -a -v \
  heads/master http://boyle.nsict.org/~mdw/git/catacomb.broken/
----

This is the `real-life' repository which has broken.  I'm using it
because the patch I made /works/ on the trivial `funt' example, but
fails miserably on the real thing.

Bisecting with this test lays the blame on 1d389ab65dc6867d30...  Which
I think is just telling me that it's always been broken.

For amusement value, my patch is below.  Here's what happens if I try to
use it.

----
got 7c40480318648672af86e03bc72bc45c07194c37
walk 7c40480318648672af86e03bc72bc45c07194c37
Getting alternates list for
http://boyle.nsict.org/~mdw/git/catacomb.broken/
got 0fd8bd4823315feb1c537a154efba002c053ed8e
Getting pack list for http://boyle.nsict.org/~mdw/git/catacomb.broken/
Getting index for pack f6d543b58ba9183c53ddbd981835f0378bdab919
Getting pack f6d543b58ba9183c53ddbd981835f0378bdab919
 which contains a137c6b3695c32ea9c42520a387a85641687662f
walk a137c6b3695c32ea9c42520a387a85641687662f
error: fd leakage in release: 4
error: fd leakage in release: 9
error: fd leakage in release: 5
error: fd leakage in release: 7
got 53124000638e53ee8aa51830fab72d8a0ce654e4
error: Couldn't find request for d2f2768351d41f80469240d1b533575d833c7370 in the queue
error: Unable to find d2f2768351d41f80469240d1b533575d833c7370 under http://boyle.nsict.org/~mdw/git/catacomb.broken/

Cannot obtain needed blob d2f2768351d41f80469240d1b533575d833c7370
while processing commit a137c6b3695c32ea9c42520a387a85641687662f.
Waiting for
http://boyle.nsict.org/~mdw/git/catacomb.broken/objects/eb/2dc153b8981ad02430bc166b925ef2457c7c36
got eb2dc153b8981ad02430bc166b925ef2457c7c36
Segmentation fault      (core dumped)
----

Without the patch, I get

----
got 7c40480318648672af86e03bc72bc45c07194c37
walk 7c40480318648672af86e03bc72bc45c07194c37
Getting alternates list for
http://boyle.nsict.org/~mdw/git/catacomb.broken/
got 0fd8bd4823315feb1c537a154efba002c053ed8e
Getting pack list for http://boyle.nsict.org/~mdw/git/catacomb.broken/
got 53124000638e53ee8aa51830fab72d8a0ce654e4
got eb2dc153b8981ad02430bc166b925ef2457c7c36
error: Unable to find a137c6b3695c32ea9c42520a387a85641687662f under http://boyle.nsict.org/~mdw/git/catacomb.broken/
----

And the patch itself.  Warning!  This DOES NOT WORK!  (But it may be of
some use to you lot anyway.)

git-http-fetch: Fix failure to find pack list.

From:  <mdw@distorted.org.uk>

  * Problem seems to be premature reuse of a slot.  In some cases, we
    end up fetching the exit status for a different fetch; if that fetch
    fails (because the object is already in a pack) then the pack list
    fetch is assumed to fail and the entire fetch falls apart.
---

 http.c   |   49 +++++++++++++++++++++++++++++++++++++++----------
 http.h   |    8 +++++++-
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/http.c b/http.c
index 75e6717..54aa9ee 100644
--- a/http.c
+++ b/http.c
@@ -1,3 +1,4 @@
+#include <assert.h>
 #include "http.h"
 
 int data_received;
@@ -192,6 +193,9 @@ static CURL* get_curl_handle(void)
 
 	curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
 
+	if (getenv("GIT_CURL_VERBOSE"))
+		curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
+
 	return result;
 }
 
@@ -262,7 +266,7 @@ void http_cleanup(void)
 
 	while (slot != NULL) {
 #ifdef USE_CURL_MULTI
-		if (slot->in_use) {
+		if (slot->in_use & SLOTUSE_ACTIVE) {
 			curl_easy_getinfo(slot->curl,
 					  CURLINFO_EFFECTIVE_URL,
 					  &wait_url);
@@ -311,6 +315,7 @@ struct active_request_slot *get_active_s
 		newslot->curl = NULL;
 		newslot->in_use = 0;
 		newslot->next = NULL;
+		newslot->nrefs = 0;
 
 		slot = active_queue_head;
 		if (slot == NULL) {
@@ -333,7 +338,7 @@ struct active_request_slot *get_active_s
 	}
 
 	active_requests++;
-	slot->in_use = 1;
+	slot->in_use |= SLOTUSE_ACTIVE;
 	slot->local = NULL;
 	slot->callback_data = NULL;
 	slot->callback_func = NULL;
@@ -351,8 +356,9 @@ int start_active_slot(struct active_requ
 
 	if (curlm_result != CURLM_OK &&
 	    curlm_result != CURLM_CALL_MULTI_PERFORM) {
-		active_requests--;
-		slot->in_use = 0;
+		slot->in_use &= ~SLOTUSE_ACTIVE;
+		if (!slot->in_use)
+			active_requests--;
 		return 0;
 	}
 #endif
@@ -375,6 +381,25 @@ void step_active_slots(void)
 }
 #endif
 
+void watch_active_slot(struct active_request_slot *slot)
+{
+	if (!slot->in_use)
+		active_requests++;
+	slot->in_use |= SLOTUSE_REF;
+	slot->nrefs++;
+}
+
+void unwatch_active_slot(struct active_request_slot *slot)
+{
+	assert(slot->nrefs);
+	slot->nrefs--;
+	if (!slot->nrefs) {
+		slot->in_use &= ~SLOTUSE_REF;
+		if (!slot->in_use)
+			active_requests--;
+	}
+}
+
 void run_active_slot(struct active_request_slot *slot)
 {
 #ifdef USE_CURL_MULTI
@@ -386,7 +411,8 @@ void run_active_slot(struct active_reque
 	int max_fd;
 	struct timeval select_timeout;
 
-	while (slot->in_use) {
+	watch_active_slot(slot);
+	while (slot->in_use & SLOTUSE_ACTIVE) {
 		data_received = 0;
 		step_active_slots();
 
@@ -397,7 +423,7 @@ void run_active_slot(struct active_reque
 			last_pos = current_pos;
 		}
 
-		if (slot->in_use && !data_received) {
+		if ((slot->in_use & SLOTUSE_ACTIVE) && !data_received) {
 			max_fd = 0;
 			FD_ZERO(&readfds);
 			FD_ZERO(&writefds);
@@ -408,8 +434,9 @@ void run_active_slot(struct active_reque
 			       &excfds, &select_timeout);
 		}
 	}
+	unwatch_active_slot(slot);
 #else
-	while (slot->in_use) {
+	while (slot->in_use & SLOTUSE_ACTIVE) {
 		slot->curl_result = curl_easy_perform(slot->curl);
 		finish_active_slot(slot);
 	}
@@ -418,8 +445,10 @@ void run_active_slot(struct active_reque
 
 static void finish_active_slot(struct active_request_slot *slot)
 {
-        active_requests--;
-        slot->in_use = 0;
+	assert(slot->in_use & SLOTUSE_ACTIVE);
+        slot->in_use &= ~SLOTUSE_ACTIVE;
+	if (!slot->in_use)
+		active_requests--;
         curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
  
         /* Run callback if appropriate */
@@ -433,7 +462,7 @@ void finish_all_active_slots(void)
 	struct active_request_slot *slot = active_queue_head;
 
 	while (slot != NULL)
-		if (slot->in_use) {
+		if (slot->in_use & SLOTUSE_ACTIVE) {
 			run_active_slot(slot);
 			slot = active_queue_head;
 		} else {
diff --git a/http.h b/http.h
index ed4ea33..6913c6a 100644
--- a/http.h
+++ b/http.h
@@ -26,7 +26,8 @@ struct active_request_slot
 {
 	CURL *curl;
 	FILE *local;
-	int in_use;
+	unsigned in_use;
+	int nrefs;
 	CURLcode curl_result;
 	long http_code;
 	void *callback_data;
@@ -34,6 +35,9 @@ struct active_request_slot
 	struct active_request_slot *next;
 };
 
+#define SLOTUSE_ACTIVE 1u
+#define SLOTUSE_REF 2u
+
 struct buffer
 {
         size_t posn;
@@ -54,6 +58,8 @@ extern struct active_request_slot *get_a
 extern int start_active_slot(struct active_request_slot *slot);
 extern void run_active_slot(struct active_request_slot *slot);
 extern void finish_all_active_slots(void);
+extern void watch_active_slot(struct active_request_slot *slot);
+extern void unwatch_active_slot(struct active_request_slot *slot);
 
 #ifdef USE_CURL_MULTI
 extern void fill_active_slots(void);

-- [mdw]

Re: git-http-fetch failure/segfault -- alas no patch

[Nick Hengeveld]

On Mon, Jan 30, 2006 at 08:34:16PM +0000, Mark Wooding wrote:

> run_active_slot.  As far as I can make out, the slot used to collect
> .../info/packs is being /reused/ by fill_active_slots (called by
> step_active_slots) before fetch_indices is returned to.

Good catch, there were a couple of places where slot result data could
be processed after the slot had been reused, with rather unpredictable
results.

There's a patch on the way after a bit more testing.

-- 
For a successful technology, reality must take precedence over public
relations, for nature cannot be fooled.