git clone over smart-http hanging for just one repo.

git clone over smart-http hanging for just one repo.

[Brady Catherman]

I have a git repo that fails to clone or fetch over smart-http, but  
works great over dav. I am wondering if somebody can help me debug the  
issue since I am at a loss why this is happening.

Just for clarification, I have several dozen repos in a directory,  
almost all others work without issue, but one of them is messed up.  
Replacing it with a different repo works so the error has to be  
somewhere in the git files themselves. Cloning the repo via DAV works  
and git fsck reports no problems with the repo.

This is all on git 1.7.0.1 on a Linux machine.

The interesting parts of a strace of git-http-backend following a git  
clone follow:

12037 execve("/usr/libexec/git-core/git-http-backend.real", ["/usr/ 
libexec/git-core/git-http-backend.real"], [/* 33 vars */]) = 0
... various library loads and such ...
12037 access("/usr/local/git/gitrepo", F_OK) = 0
12037 chdir("/usr/local/git/gitrepo")  = 0
12037 access("objects", X_OK)           = 0
12037 access("refs", X_OK)              = 0
12037 lstat("HEAD", {st_mode=S_IFREG|0644, st_size=23, ...}) = 0
12037 open("HEAD", O_RDONLY)            = 3
12037 read(3, "ref: refs/heads/master\n", 255) = 23
12037 access("/usr/local/etc/gitconfig", R_OK) = -1 ENOENT (No such  
file or directory)
12037 access("./config", R_OK)          = 0
12037 open("./config", O_RDONLY)        = 3
12037 read(3, "[core]\n\trepositoryformatversion = 0\n\tfilemode = true 
\n\tbare = true\n\tlogallrefupdates = true\n\tsharedRepository = true 
\n", 4096) = 116
12037 read(3, "", 4096)                 = 0
12037 close(3)                          = 0
12037 munmap(0x2aaaaaaac000, 4096)      = 0
12037 write(1, "Expires: Fri, 01 Jan 1980 00:00:00 GMT\r\n", 40) = 40
12037 write(1, "Pragma: no-cache\r\n", 18) = 18
12037 write(1, "Cache-Control: no-cache, max-age=0, must-revalidate\r 
\n", 53) = 53
12037 write(1, "Content-Type: application/x-git-upload-pack-result\r 
\n", 52) = 52
12037 write(1, "\r\n", 2)               = 2
12037 pipe([3, 4])                      = 0
12037 pipe([3, 4])                      = 012037 pipe([5,  
6])                      = 0
12037 clone(child_stack=0, flags=CLONE_CHILD_CLEARTID| 
CLONE_CHILD_SETTID|SIGCHLD
, child_tidptr=0x2aaaaae12c20) = 12038
12038 close(5 <unfinished ...>
12037 close(6)                          = 0
12037 read(5,  <unfinished ...>
12038 <... close resumed> )             = 0
12038 fcntl(6, F_GETFD)                 = 0
12038 fcntl(6, F_SETFD, FD_CLOEXEC)     = 012038 dup2(3,  
0)                        = 0
12038 close(3)                          = 012038  
close(4)                          = 0
12038 execve("/usr/local/libexec/git-core/git", ["git", "upload-pack",  
"--stateless-rpc", "."], [/* 36 vars */]) = -1 ENOENT (No such file or  
directory)
12038 execve("/usr/libexec/git-core/git", ["git", "upload-pack", "-- 
stateless-rp
c", "."], [/* 36 vars */] <unfinished ...>
12037 <... read resumed> "", 1)         = 0
12037 <... read resumed> "", 1)         = 012037  
close(5)                          = 0
12037 close(3)                          = 012038 <... execve  
resumed> )            = 0
12037 close(1)                          = 0
... More random library loads ...
12038 pipe([3, 4])                      = 0
12038 clone(child_stack=0, flags=CLONE_CHILD_CLEARTID| 
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x2aaaaae12c40) = 12039
12039 close(3)                          = 0
12039 fcntl(4, F_GETFD)                 = 0
12039 fcntl(4, F_SETFD, FD_CLOEXEC)     = 0
12039 execve("/usr/libexec/git-core/git-upload-pack", ["git-upload- 
pack", "--stateless-rpc", "."], [/* 36 vars */] <unfinished ...>
12038 close(4)                          = 0
12037 read(0, "0067want c83f5a4ec9fb6b6681e74dc3c2276de5b947a76c  
multi_ack_detailed side-band-64k thin-pack ofs-delta\n0032want  
92e9b73bbd59b5ecf711381716c8aa13948f5a5d\n0032want
.. clipped out lots of sha's ..
98040a8100513ad2852ca911ac330ebd4ff9e10d\n00000009done\n", 8192) = 2316
12039 <... execve resumed> )            = 0
12038 <... read resumed> "", 1)         = 0
12038 close(3)                          = 0
12039 brk(0 <unfinished ...>
12038 wait4(12039,  <unfinished ...>
12039 <... brk resumed> )               = 0xf9d1000
12037 write(1, "Status: 500 Internal Server Error\r\n", 35  
<unfinished ...>
12039 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE| 
MAP_ANONYMOUS, -1, 0 <unfinished ...>
12037 <... write resumed> )             = -1 EBADF (Bad file descriptor)
12039 <... mmap resumed> )              = 0x2aaaaaaab000
... This repeats for a while.. spitting out 500's on a bad file  
descriptor...
12037 write(1, "Status: 500 Internal Server Error\r\n", 35) = -1 EBADF  
(Bad file descriptor)
... Eventually this just repeats a few thousand times ...
12037 write(1, "Status: 500 Internal Server Error\r\n", 35) = -1 EBADF  
(Bad file descriptor)
12037 --- SIGSEGV (Segmentation fault) @ 0 (0) ---
12037 +++ killed by SIGSEGV +++
12039 <... read resumed> "", 4)         = 0
12039 write(2, "fatal: The remote end hung up unexpectedly\n", 43) = 43
12039 exit_group(128)                   = ?
12038 <... wait4 resumed> [{WIFEXITED(s) && WEXITSTATUS(s) == 128}],  
0, NULL) = 12039
12038 --- SIGCHLD (Child exited) @ 0 (0) ---
12038 exit_group(128)                   = ?

Anybody have any thoughts why this would happen or what can be done to  
fix it?

Re: git clone over smart-http hanging for just one repo.

[Shawn O. Pearce]

Brady Catherman <brady@catherman.org> wrote:
> I have a git repo that fails to clone or fetch over smart-http, but  
> works great over dav. I am wondering if somebody can help me debug the  
> issue since I am at a loss why this is happening.

Yea, I'm at a loss too. :-(

> The interesting parts of a strace of git-http-backend following a git  
> clone follow:
...
> 12037 close(1)                          = 0

Why did the CGI process just close stdout?  I'm guessing this is
part of the exec of the upload-pack child in the background.  Oh,
right, we closed it because we passed the descriptor to the child
and now the parent CGI doesn't want it anymore.

> 12037 write(1, "Status: 500 Internal Server Error\r\n", 35) = -1 EBADF  
> (Bad file descriptor)

This smells like the backend upload-pack process got into trouble and
exited early, so now the CGI is trying to change the status to 500
since the backend exited with a non-zero status.  Only its too late,
as the filedescriptor was already closed after the successful fork().

We're stuck in a loop because we're failing during the die routine.

Because the file descriptor is closed, safe_write() which was what
originated that write(1, ...) above, tries to call die().  But that
die() call invokes die_webcgi() which in turn tries to write that
500 error message again to 1.  So this goes on a for a while...

> 12037 --- SIGSEGV (Segmentation fault) @ 0 (0) ---
> 12037 +++ killed by SIGSEGV +++

and then we run out of stack space, due to too many recursions,
and the process is aborted by a SIGSEGV.

> Anybody have any thoughts why this would happen or what can be done to  
> fix it?

A gdb trace or something of the upload-pack process would help.
That appears to have also died and we don't know why.  Its death
is what contributed to the CGI crashing above.

I'll try to send a patch for this recursive crashing problem in
the CGI.

-- 
Shawn.

[PATCH] http-backend: Don't infinite loop during die()

[Shawn O. Pearce]

If stdout has already been closed by the CGI and die() gets called,
the CGI will fail to write the "Status: 500 Internal Server Error" to
the pipe, which results in die() being called again (via safe_write).
This goes on in an infinite loop until the stack overflows and the
process is killed by SIGSEGV.

Instead set a flag on the first die() invocation and perform no
action during recursive die() calls.  This way failures to write the
error messages to the stdout pipe do not result in an infinite loop.

We also now report on the death to stderr before we report to stdout,
to increase the chances that the cause of the die() invocation will
appear in the server's error log.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This patch should be put in maint.

 It doesn't fix the underlying problem Brady has found, but it
 will at least get us more information by avoiding the infinite
 loop and later SIGSEGV crash of the parent CGI.

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

diff --git a/http-backend.c b/http-backend.c
index 8c7b7d0..5c0d649 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -538,12 +538,17 @@ static void service_rpc(char *service_name)
 
 static NORETURN void die_webcgi(const char *err, va_list params)
 {
-	http_status(500, "Internal Server Error");
-	hdr_nocache();
-	end_headers();
+	static int dead;
 
-	vreportf("fatal: ", err, params);
-	exit(0);
+	if (!dead) {
+		dead = 1;
+
+		vreportf("fatal: ", err, params);
+		http_status(500, "Internal Server Error");
+		hdr_nocache();
+		end_headers();
+		exit(0);
+	}
 }
 
 static char* getdir(void)
-- 
1.6.4.rc2.182.g24de1

-- 
Shawn.

Re: [PATCH] http-backend: Don't infinite loop during die()

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> If stdout has already been closed by the CGI and die() gets called,
> the CGI will fail to write the "Status: 500 Internal Server Error" to
> the pipe, which results in die() being called again (via safe_write).
> This goes on in an infinite loop until the stack overflows and the
> process is killed by SIGSEGV.

Before looking at the code I first thought "wouldn't we have the same
problem in die(), and shouldn't we have "dying" flag there?" but I was
stupid as usual ;-)

The patch looks sane.  Thanks.

Re: [PATCH] http-backend: Don't infinite loop during die()

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> If stdout has already been closed by the CGI and die() gets called,
> the CGI will fail to write the "Status: 500 Internal Server Error" to
> the pipe, which results in die() being called again (via safe_write).
> This goes on in an infinite loop until the stack overflows and the
> process is killed by SIGSEGV.
>
> Instead set a flag on the first die() invocation and perform no
> action during recursive die() calls.  This way failures to write the
> error messages to the stdout pipe do not result in an infinite loop.

Hmm.  I would need something like this on top, but there must be a better
way.  Ideas?

-- >8 --
Subject: [PATCH] fixup! http-backend.c: Don't infinite loop

Now die_webcgi() actually can return during a recursive call into it,
causing

    http-backend.c:554: error: 'noreturn' function does return

Work it around with a somewhat ugly workaround.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http-backend.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index f4d49b6..d3ec6f0 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -536,7 +536,7 @@ static void service_rpc(char *service_name)
 	strbuf_release(&buf);
 }
 
-static NORETURN void die_webcgi(const char *err, va_list params)
+static void die_webcgi(const char *err, va_list params)
 {
 	static int dead;
 
@@ -606,7 +606,7 @@ int main(int argc, char **argv)
 	int i;
 
 	git_extract_argv0_path(argv[0]);
-	set_die_routine(die_webcgi);
+	set_die_routine((void *)die_webcgi);
 
 	if (!method)
 		die("No REQUEST_METHOD from server");
-- 
1.7.0.3.435.g097f4

Re: [PATCH] http-backend: Don't infinite loop during die()

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > If stdout has already been closed by the CGI and die() gets called,
> > the CGI will fail to write the "Status: 500 Internal Server Error" to
> > the pipe, which results in die() being called again (via safe_write).
> > This goes on in an infinite loop until the stack overflows and the
> > process is killed by SIGSEGV.
> >
> > Instead set a flag on the first die() invocation and perform no
> > action during recursive die() calls.  This way failures to write the
> > error messages to the stdout pipe do not result in an infinite loop.
> 
> Hmm.  I would need something like this on top, but there must be a better
> way.  Ideas?

Ick.

Just exit(0) if dead is true.  The *only* reason we would come back
into the die handler at this point is because we failed during the
die handler.  That recursive failure means we can't report the
errors, because that's all that was left once the dead variable
was set.

-- 
Shawn.

Re: [PATCH] http-backend: Don't infinite loop during die()

[Nicolas Pitre]

On Wed, 24 Mar 2010, Shawn O. Pearce wrote:

> Junio C Hamano <gitster@pobox.com> wrote:
> > "Shawn O. Pearce" <spearce@spearce.org> writes:
> > 
> > > If stdout has already been closed by the CGI and die() gets called,
> > > the CGI will fail to write the "Status: 500 Internal Server Error" to
> > > the pipe, which results in die() being called again (via safe_write).
> > > This goes on in an infinite loop until the stack overflows and the
> > > process is killed by SIGSEGV.
> > >
> > > Instead set a flag on the first die() invocation and perform no
> > > action during recursive die() calls.  This way failures to write the
> > > error messages to the stdout pipe do not result in an infinite loop.
> > 
> > Hmm.  I would need something like this on top, but there must be a better
> > way.  Ideas?
> 
> Ick.
> 
> Just exit(0) if dead is true.

Do you really want the exit code to be 0 if you're dying?


Nicolas

Re: [PATCH] http-backend: Don't infinite loop during die()

[Shawn Pearce]

On Wed, Mar 24, 2010 at 1:24 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Wed, 24 Mar 2010, Shawn O. Pearce wrote:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > "Shawn O. Pearce" <spearce@spearce.org> writes:
>> >
>> > > If stdout has already been closed by the CGI and die() gets called,
>> > > the CGI will fail to write the "Status: 500 Internal Server Error" to
>> > > the pipe, which results in die() being called again (via safe_write).
>> > > This goes on in an infinite loop until the stack overflows and the
>> > > process is killed by SIGSEGV.
>> > >
>> > > Instead set a flag on the first die() invocation and perform no
>> > > action during recursive die() calls.  This way failures to write the
>> > > error messages to the stdout pipe do not result in an infinite loop.
>> >
>> > Hmm.  I would need something like this on top, but there must be a better
>> > way.  Ideas?
>>
>> Ick.
>>
>> Just exit(0) if dead is true.
>
> Do you really want the exit code to be 0 if you're dying?

IIRC, yes.  If we exit non-zero Apache really freaked out.  That was a
big part of my motivation to write the die_webcgi handler.

-- 
Shawn.

Re: [PATCH] http-backend: Don't infinite loop during die()

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Just exit(0) if dead is true.

heh, very true.

-- >8 --
Subject: [PATCH] fixup! http-backend.c: Don't infinite loop

Now die_webcgi() actually can return during a recursive call into it,
causing

    http-backend.c:554: error: 'noreturn' function does return

The only reason we would come back to the die handler is because we
failed during it, so we cannot report anything anyway.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http-backend.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index f4d49b6..5e3f277 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -549,8 +549,8 @@ static NORETURN void die_webcgi(const char *err, va_list params)
 		http_status(500, "Internal Server Error");
 		hdr_nocache();
 		end_headers();
-		exit(0);
 	}
+	exit(0); /* keep apache happy */
 }
 
 static char* getdir(void)