{"thread":{"id":"27105","subject":"[PATCH] git-daemon: fix segfaulting in child_handler() in AIX","startedAt":"2011-04-15T16:51:00Z","lastAt":"2011-05-02T17:51:08Z","messageCount":5,"participants":["Seth Jennings","Christian Couder","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"165907","messageId":"1302886260-25860-1-git-send-email-spartacus06@gmail.com","threadId":"27105","inReplyTo":null,"subject":"[PATCH] git-daemon: fix segfaulting in child_handler() in AIX","fromName":"Seth Jennings","fromEmail":"spartacus06@gmail.com","sentAt":"2011-04-15T16:51:00Z","receivedAt":"2011-04-15T16:51:00Z","isPatch":true,"sender":{"key":"spartacus06@gmail.com","avatar":"https://gravatar.com/avatar/d7f3507547ca08a9fa6f2708aacd1f6a16bf67469f6287051ba4908fbc35a72b?d=mp&s=160"},"body":"This issue seems to be specific to git-daemon on AIX built with xlc.\nAfter commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),\ngit-daemon segfaults in child_handler() inside the signal() syscall\nimmediately after any remote clone/pull operation. While it is not\nfully understood why this happens, changing signal() to sigaction()\nresolves the issue.\n\nThis commit converts singal() to sigaction() in child_handler().\n---\n daemon.c | 10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..3ea5b2c 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -715,7 +715,10 @@ static void child_handler(int signo)\n \t * upon signal receipt\n \t * SysV needs the handler to be rearmed\n \t */\n-\tsignal(SIGCHLD, child_handler);\n+\tstruct sigaction sigact;\n+\tmemset(&sigact, 0, sizeof(sigact));\n+\tsigact.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sigact, NULL);\n }\n \n static int set_reuse_addr(int sockfd)\n@@ -889,7 +892,10 @@ static int service_loop(struct socketlist *socklist)\n \t\tpfd[i].events = POLLIN;\n \t}\n \n-\tsignal(SIGCHLD, child_handler);\n+\tstruct sigaction sigact;\n+\tmemset(&sigact, 0, sizeof(sigact));\n+\tsigact.sa_handler = child_handler;\n+\tsigaction(SIGCHLD, &sigact, NULL);\n \n \tfor (;;) {\n \t\tint i;\n-- \n1.7.0.4\n"},{"id":"165908","messageId":"BANLkTi=6UVWpa2aPPebVUG9ZyL_h7OcwUQ@mail.gmail.com","threadId":"27105","inReplyTo":"1302886260-25860-1-git-send-email-spartacus06@gmail.com","subject":"Re: [PATCH] git-daemon: fix segfaulting in child_handler() in AIX","fromName":"Seth Jennings","fromEmail":"spartacus06@gmail.com","sentAt":"2011-04-15T16:54:12Z","receivedAt":"2011-04-15T16:54:12Z","isPatch":true,"sender":{"key":"spartacus06@gmail.com","avatar":"https://gravatar.com/avatar/d7f3507547ca08a9fa6f2708aacd1f6a16bf67469f6287051ba4908fbc35a72b?d=mp&s=160"},"body":"There is a git-daemon segfault issue that seems to be specific to AIX.\n\nWhenever a remote user pulls or clones, the operation succeeds but\ngit-daemon crashes immediately afterward.\n\n$ gdb git-daemon core\n...\nCore was generated by `git-daemon'.\nProgram terminated with signal 11, Segmentation fault.\n#0 0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n(gdb) where\n#0 0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n#1 0xd04f1874 in _p_sigaction () from /usr/lib/libpthreads.a(shr_xpg5.o)\n#2 0xd013ae34 in sigaction () from /usr/lib/libc.a(shr.o)\n#3 0xd0217cd8 in signal () from /usr/lib/libc.a(shr.o)\n#4 0x10000b90 in child_handler (signo=0) at daemon.c:718\n#5 \n\nThrough experimentation, I found that using sigaction() instead of\nsignal() resolves the issue. I'm not entirely sure why this is.\n\nAny feedback about the issue or the patch is welcome. There might be\na better solution.\n\nOn Fri, Apr 15, 2011 at 11:51 AM, Seth Jennings wrote:\n> This issue seems to be specific to git-daemon on AIX built with xlc.\n> After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),\n> git-daemon segfaults in child_handler() inside the signal() syscall\n> immediately after any remote clone/pull operation.  While it is not\n> fully understood why this happens, changing signal() to sigaction()\n> resolves the issue.\n>\n> This commit converts singal() to sigaction() in child_handler().\n> ---\n>  daemon.c |   10 ++++++++--\n>  1 files changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 4c8346d..3ea5b2c 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -715,7 +715,10 @@ static void child_handler(int signo)\n>         * upon signal receipt\n>         * SysV needs the handler to be rearmed\n>         */\n> -       signal(SIGCHLD, child_handler);\n> +       struct sigaction sigact;\n> +       memset(&sigact, 0, sizeof(sigact));\n> +       sigact.sa_handler = child_handler;\n> +       sigaction(SIGCHLD, &sigact, NULL);\n>  }\n>\n>  static int set_reuse_addr(int sockfd)\n> @@ -889,7 +892,10 @@ static int service_loop(struct socketlist *socklist)\n>                pfd[i].events = POLLIN;\n>        }\n>\n> -       signal(SIGCHLD, child_handler);\n> +       struct sigaction sigact;\n> +       memset(&sigact, 0, sizeof(sigact));\n> +       sigact.sa_handler = child_handler;\n> +       sigaction(SIGCHLD, &sigact, NULL);\n>\n>        for (;;) {\n>                int i;\n> --\n> 1.7.0.4\n>\n>\n"},{"id":"166148","messageId":"BANLkTimtfXgy9UcdQ8b-8dLrO-qgXFgQnw@mail.gmail.com","threadId":"27105","inReplyTo":"BANLkTi=6UVWpa2aPPebVUG9ZyL_h7OcwUQ@mail.gmail.com","subject":"Re: [PATCH] git-daemon: fix segfaulting in child_handler() in AIX","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2011-04-20T12:53:03Z","receivedAt":"2011-04-20T12:53:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nI have no idea what the problem could be, but maybe I will be lucky\nwith my suggestions.\n\nOn Fri, Apr 15, 2011 at 6:54 PM, Seth Jennings wrote:\n> There is a git-daemon segfault issue that seems to be specific to AIX.\n>\n> Whenever a remote user pulls or clones, the operation succeeds but\n> git-daemon crashes immediately afterward.\n>\n> $ gdb git-daemon core\n> ...\n> Core was generated by `git-daemon'.\n> Program terminated with signal 11, Segmentation fault.\n> #0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> (gdb) where\n> #0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> #1  0xd04f1874 in _p_sigaction () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> #2  0xd013ae34 in sigaction () from /usr/lib/libc.a(shr.o)\n> #3  0xd0217cd8 in signal () from /usr/lib/libc.a(shr.o)\n\nsignal() is calling sigaction() so sigaction() must be called with\ndifferent parameters in your patch and when it crashes.\nCould you have a look at the difference between parameters?\n\n> #4  0x10000b90 in child_handler (signo=0) at daemon.c:718\n> #5  \n>\n> Through experimentation, I found that using sigaction() instead of\n> signal() resolves the issue.  I'm not entirely sure why this is.\n>\n> Any feedback about the issue or the patch is welcome.  There might be\n> a better solution.\n>\n> On Fri, Apr 15, 2011 at 11:51 AM, Seth Jennings wrote:\n>> This issue seems to be specific to git-daemon on AIX built with xlc.\n>> After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),\n>> git-daemon segfaults in child_handler() inside the signal() syscall\n>> immediately after any remote clone/pull operation.\n\nCould you try some variants that revert or change parts of this commit?\n\nFor example you could revert only this hunk:\n\n@@ -1036,11 +1032,6 @@ int main(int argc, char **argv)\n gid_t gid = 0;\n int i;\n\n- /* Without this we cannot rely on waitpid() to tell\n- * what happened to our children.\n- */\n- signal(SIGCHLD, SIG_DFL);\n-\n for (i = 1; i < argc; i++) {\n char *arg = argv[i];\n\n\n>> While it is not\n>> fully understood why this happens, changing signal() to sigaction()\n>> resolves the issue.\n\nYeah but it would be nice to understand.\n\nThanks in advance,\nChristian.\n"},{"id":"166204","messageId":"BANLkTinu0Rx9AM-+S8fyC97vUXXgeT5jbQ@mail.gmail.com","threadId":"27105","inReplyTo":"1302886260-25860-1-git-send-email-spartacus06@gmail.com","subject":"Re: [PATCH] git-daemon: fix segfaulting in child_handler() in AIX","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-21T20:43:26Z","receivedAt":"2011-04-21T20:43:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 15, 2011 at 6:51 PM, Seth Jennings wrote:\n> This issue seems to be specific to git-daemon on AIX built with xlc.\n> After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),\n> git-daemon segfaults in child_handler() inside the signal() syscall\n> immediately after any remote clone/pull operation.  While it is not\n> fully understood why this happens, changing signal() to sigaction()\n> resolves the issue.\n>\n> This commit converts singal() to sigaction() in child_handler().\n\nAt first I thought that a change like this would break the\nWindows-port, because our win32-implementation of sigaction does not\nsupport SIGCHLD. But looking at the code (and MSDN), our\nwin32-implementation of signal doesn't either. However, our\nsignal-implementation piggy-backs on the signal-implementation in\nmsvcrt.dll for non-SIGALRM signals, and microsoft's implementation\ndoes not seem to update errno in this case. So, this patch will\nintroduce an error-line saying \"sigaction only implemented for\nSIGALRM\".\n\nExactly what SHOULD be done for Windows in this case isn't entirely\nclear. Perhaps we should just ignore it?\n\nApart from this, I must say I agree with Christian; we should find out\nexactly why a change like this is needed.\n\n> ---\n>  daemon.c |   10 ++++++++--\n>  1 files changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 4c8346d..3ea5b2c 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -715,7 +715,10 @@ static void child_handler(int signo)\n>         * upon signal receipt\n>         * SysV needs the handler to be rearmed\n>         */\n> -       signal(SIGCHLD, child_handler);\n> +       struct sigaction sigact;\n> +       memset(&sigact, 0, sizeof(sigact));\n> +       sigact.sa_handler = child_handler;\n> +       sigaction(SIGCHLD, &sigact, NULL);\n>  }\n>\n>  static int set_reuse_addr(int sockfd)\n> @@ -889,7 +892,10 @@ static int service_loop(struct socketlist *socklist)\n>                pfd[i].events = POLLIN;\n>        }\n>\n> -       signal(SIGCHLD, child_handler);\n> +       struct sigaction sigact;\n\nThis gives a warning on MinGW:\ndaemon.c: In function 'service_loop':\ndaemon.c:895: warning: ISO C90 forbids mixed declarations and code\n"},{"id":"166886","messageId":"BANLkTinOkE-w3qdASuje72a3G6bpEoxWxA@mail.gmail.com","threadId":"27105","inReplyTo":"BANLkTimtfXgy9UcdQ8b-8dLrO-qgXFgQnw@mail.gmail.com","subject":"Re: [PATCH] git-daemon: fix segfaulting in child_handler() in AIX","fromName":"Seth Jennings","fromEmail":"spartacus06@gmail.com","sentAt":"2011-05-02T17:51:08Z","receivedAt":"2011-05-02T17:51:08Z","isPatch":true,"sender":{"key":"spartacus06@gmail.com","avatar":"https://gravatar.com/avatar/d7f3507547ca08a9fa6f2708aacd1f6a16bf67469f6287051ba4908fbc35a72b?d=mp&s=160"},"body":"Thanks Christian,\n\nSorry for the dup for you Christian. The email sent in HTML instead\nof plaintext and got bounced by the mailing list.\n\nI tried reverting just the part the sets the SIGCHLD handler to the\ndefault as you suggested, but the problem still exists.\n\nIt is true that the AIX libc signal() function basically wraps the a\ncall to sigaction().\n\nI'll investigate more and update this thread when I have something.\n\nSeth\n\nOn Wed, Apr 20, 2011 at 7:53 AM, Christian Couder\n wrote:\n>\n> Hi,\n>\n> I have no idea what the problem could be, but maybe I will be lucky\n> with my suggestions.\n>\n> On Fri, Apr 15, 2011 at 6:54 PM, Seth Jennings wrote:\n> > There is a git-daemon segfault issue that seems to be specific to AIX.\n> >\n> > Whenever a remote user pulls or clones, the operation succeeds but\n> > git-daemon crashes immediately afterward.\n> >\n> > $ gdb git-daemon core\n> > ...\n> > Core was generated by `git-daemon'.\n> > Program terminated with signal 11, Segmentation fault.\n> > #0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> > (gdb) where\n> > #0  0xd04f0c50 in _sigsetmask () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> > #1  0xd04f1874 in _p_sigaction () from /usr/lib/libpthreads.a(shr_xpg5.o)\n> > #2  0xd013ae34 in sigaction () from /usr/lib/libc.a(shr.o)\n> > #3  0xd0217cd8 in signal () from /usr/lib/libc.a(shr.o)\n>\n> signal() is calling sigaction() so sigaction() must be called with\n> different parameters in your patch and when it crashes.\n> Could you have a look at the difference between parameters?\n>\n> > #4  0x10000b90 in child_handler (signo=0) at daemon.c:718\n> > #5  \n> >\n> > Through experimentation, I found that using sigaction() instead of\n> > signal() resolves the issue.  I'm not entirely sure why this is.\n> >\n> > Any feedback about the issue or the patch is welcome.  There might be\n> > a better solution.\n> >\n> > On Fri, Apr 15, 2011 at 11:51 AM, Seth Jennings wrote:\n> >> This issue seems to be specific to git-daemon on AIX built with xlc.\n> >> After commit 695605b5080e1957bd9dab1fed35a7fee9814297 (from Aug 2008),\n> >> git-daemon segfaults in child_handler() inside the signal() syscall\n> >> immediately after any remote clone/pull operation.\n>\n> Could you try some variants that revert or change parts of this commit?\n>\n> For example you could revert only this hunk:\n>\n> @@ -1036,11 +1032,6 @@ int main(int argc, char **argv)\n>        gid_t gid = 0;\n>        int i;\n>\n> -       /* Without this we cannot rely on waitpid() to tell\n> -        * what happened to our children.\n> -        */\n> -       signal(SIGCHLD, SIG_DFL);\n> -\n>        for (i = 1; i < argc; i++) {\n>                char *arg = argv[i];\n>\n>\n> >> While it is not\n> >> fully understood why this happens, changing signal() to sigaction()\n> >> resolves the issue.\n>\n> Yeah but it would be nice to understand.\n>\n> Thanks in advance,\n> Christian.\n"}]}