[PATCH] git-daemon: fix segfaulting in child_handler() in AIX

This issue seems to be specific to git-daemon on AIX built with xlc.
After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),
git-daemon segfaults in child_handler() inside the signal() syscall
immediately after any remote clone/pull operation.  While it is not
fully understood why this happens, changing signal() to sigaction()
resolves the issue.

This commit converts singal() to sigaction() in child_handler().
---
 daemon.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..3ea5b2c 100644
--- a/daemon.c
+++ b/daemon.c

@@ -715,7 +715,10 @@ static void child_handler(int signo)
 	 * upon signal receipt
 	 * SysV needs the handler to be rearmed
 	 */
-	signal(SIGCHLD, child_handler);
+	struct sigaction sigact;
+	memset(&sigact, 0, sizeof(sigact));
+	sigact.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sigact, NULL);
 }
 
 static int set_reuse_addr(int sockfd)

@@ -889,7 +892,10 @@ static int service_loop(struct socketlist *socklist)
 		pfd[i].events = POLLIN;
 	}
 
-	signal(SIGCHLD, child_handler);
+	struct sigaction sigact;
+	memset(&sigact, 0, sizeof(sigact));
+	sigact.sa_handler = child_handler;
+	sigaction(SIGCHLD, &sigact, NULL);
 
 	for (;;) {
 		int i;

-- 
1.7.0.4

There is a git-daemon segfault issue that seems to be specific to AIX.

Whenever a remote user pulls or clones, the operation succeeds but
git-daemon crashes immediately afterward.

$ gdb git-daemon core
...
Core was generated by `git-daemon'.
Program terminated with signal 11, Segmentation fault.
#0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)
(gdb) where
#0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)
#1  0xd04f1874 in _p_sigaction () from /usr/lib/libpthreads.a(shr_xpg5.o)
#2  0xd013ae34 in sigaction () from /usr/lib/libc.a(shr.o)
#3  0xd0217cd8 in signal () from /usr/lib/libc.a(shr.o)
#4  0x10000b90 in child_handler (signo=0) at daemon.c:718
#5  <signal handler called>

Through experimentation, I found that using sigaction() instead of
signal() resolves the issue.  I'm not entirely sure why this is.

Any feedback about the issue or the patch is welcome.  There might be
a better solution.

On Fri, Apr 15, 2011 at 11:51 AM, Seth Jennings [off-list ref] wrote:

Hi,

I have no idea what the problem could be, but maybe I will be lucky
with my suggestions.

On Fri, Apr 15, 2011 at 6:54 PM, Seth Jennings [off-list ref] wrote:

There is a git-daemon segfault issue that seems to be specific to AIX.

Whenever a remote user pulls or clones, the operation succeeds but
git-daemon crashes immediately afterward.

$ gdb git-daemon core
...
Core was generated by `git-daemon'.
Program terminated with signal 11, Segmentation fault.
#0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)
(gdb) where
#0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)
#1  0xd04f1874 in _p_sigaction () from /usr/lib/libpthreads.a(shr_xpg5.o)
#2  0xd013ae34 in sigaction () from /usr/lib/libc.a(shr.o)
#3  0xd0217cd8 in signal () from /usr/lib/libc.a(shr.o)

signal() is calling sigaction() so sigaction() must be called with
different parameters in your patch and when it crashes.
Could you have a look at the difference between parameters?

#4  0x10000b90 in child_handler (signo=0) at daemon.c:718
#5  <signal handler called>

Through experimentation, I found that using sigaction() instead of
signal() resolves the issue.  I'm not entirely sure why this is.

Any feedback about the issue or the patch is welcome.  There might be
a better solution.

On Fri, Apr 15, 2011 at 11:51 AM, Seth Jennings [off-list ref] wrote:

quoted

This issue seems to be specific to git-daemon on AIX built with xlc.
After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),
git-daemon segfaults in child_handler() inside the signal() syscall
immediately after any remote clone/pull operation.

Could you try some variants that revert or change parts of this commit?

For example you could revert only this hunk:

@@ -1036,11 +1032,6 @@ int main(int argc, char **argv)
        gid_t gid = 0;
        int i;

-       /* Without this we cannot rely on waitpid() to tell
-        * what happened to our children.
-        */
-       signal(SIGCHLD, SIG_DFL);
-
        for (i = 1; i < argc; i++) {
                char *arg = argv[i];

quoted

While it is not
fully understood why this happens, changing signal() to sigaction()
resolves the issue.

Yeah but it would be nice to understand.

Thanks in advance,
Christian.

On Fri, Apr 15, 2011 at 6:51 PM, Seth Jennings [off-list ref] wrote:

This issue seems to be specific to git-daemon on AIX built with xlc.
After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),
git-daemon segfaults in child_handler() inside the signal() syscall
immediately after any remote clone/pull operation.  While it is not
fully understood why this happens, changing signal() to sigaction()
resolves the issue.

This commit converts singal() to sigaction() in child_handler().

At first I thought that a change like this would break the
Windows-port, because our win32-implementation of sigaction does not
support SIGCHLD. But looking at the code (and MSDN), our
win32-implementation of signal doesn't either. However, our
signal-implementation piggy-backs on the signal-implementation in
msvcrt.dll for non-SIGALRM signals, and microsoft's implementation
does not seem to update errno in this case. So, this patch will
introduce an error-line saying "sigaction only implemented for
SIGALRM".

Exactly what SHOULD be done for Windows in this case isn't entirely
clear. Perhaps we should just ignore it?

Apart from this, I must say I agree with Christian; we should find out
exactly why a change like this is needed.

This gives a warning on MinGW:
daemon.c: In function 'service_loop':
daemon.c:895: warning: ISO C90 forbids mixed declarations and code

Thanks Christian,

Sorry for the dup for you Christian.  The email sent in HTML instead
of plaintext and got bounced by the mailing list.

I tried reverting just the part the sets the SIGCHLD handler to the
default as you suggested, but the problem still exists.

It is true that the AIX libc signal() function basically wraps the a
call to sigaction().

I'll investigate more and update this thread when I have something.

Seth

On Wed, Apr 20, 2011 at 7:53 AM, Christian Couder
[off-list ref] wrote: