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

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 20, 2011, 12:53 UTC
Message-ID: <BANLkTimtfXgy9UcdQ8b-8dLrO-qgXFgQnw@mail.gmail.com>
In-Reply-To: <BANLkTi=6UVWpa2aPPebVUG9ZyL_h7OcwUQ@mail.gmail.com>

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:

@@ -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.