threads / patch / 27105

patchgit-daemon: fix segfaulting in child_handler() in AIX

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

## tl;dr

5 messages between Apr 15, 2011 and May 2, 2011. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Seth Jennings· Apr 15, 2011, 16:51 UTC · lore

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(-)
Show changes to daemon.c +8 −2
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
Seth Jennings· Apr 15, 2011, 16:54 UTC · re: Seth Jennings · lore

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

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 <spartacus06@gmail.com> wrote:
Show 44 quoted lines
> 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
>
>
Christian Couder· Apr 20, 2011, 12:53 UTC · re: Seth Jennings · lore

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

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 <spartacus06@gmail.com> wrote:
Show 15 quoted lines
> 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?

Show 14 quoted lines
> #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 <spartacus06@gmail.com> 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.
Could you try some variants that revert or change parts of this commit?
For example you could revert only this hunk:
Show changes to diff +0 −5
@@ -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];


>> 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.
Seth Jennings· May 2, 2011, 17:51 UTC · re: Christian Couder · lore

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

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 <christian.couder@gmail.com> wrote:

Show 67 quoted lines
>
> 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 <spartacus06@gmail.com> 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 <spartacus06@gmail.com> 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.
>
> 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];
>
>
> >> 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.
Erik Faye-Lund· Apr 21, 2011, 20:43 UTC · re: Seth Jennings · lore

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

On Fri, Apr 15, 2011 at 6:51 PM, Seth Jennings <spartacus06@gmail.com> wrote:
Show 8 quoted lines
> 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.

Show 26 quoted lines
> ---
>  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;

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

← back to recent threads