# git on AIX: daemon.c & t5570-git-daemon.sh 2 messages from 2019-03-19 to 2019-04-22. Participants: REIX, Tony, Jeff King. Thread: https://gitlist.dev/t/50777 ## REIX, Tony, 2019-03-19 09:05 Subject: git on AIX: daemon.c & t5570-git-daemon.sh Message-ID: URL: https://gitlist.dev/e/DB7PR02MB46634F68900E0F81B0110AF586400%40DB7PR02MB4663.eurprd02.prod.outlook.com In-Reply-To: ``` Hi, When testing version 2.21.0 of git on AIX (6.1 & 7.2), I have found an issue with daemon.c and test t5570-git-daemon.sh : within test 4, the child_handler() code gets crazy and calls itself recursively till the process crashes. We do not have a clear idea why this issue occurs. Maybe that this issue also appears on other operating systems. This code: static void child_handler(int signo) {         signal(SIGCHLD, child_handler); } is the root cause and it appeared in 2008, for SysV :    https://git.kernel.org/pub/scm/git/git.git/commit/daemon.c?id=bd7b371e9c2aeb0aaf228dc1655e8d04fca6f797 I suggest to use the attached patch git-2.21.0-SIGCHLD-handler-v3.patch, so that this issue on AIX disappears. Moreover, this code may also be useful for other operating systems. Someone should tests it on Solaris. I have checked that this patch is OK (build & test t5570-git-daemon.sh) on Linux/Power (Fedora 28) with GCC (v8.1). I've also checked on Linux/Power that the 21 tests of t5570-git-daemon.sh are OK when one replaces the existing code using:     signal(SIGCHLD, child_handler); by the code dedicated to AIX. I mean to say that this sigaction(SIGCHLD, &sa, NULL) code could also replace on Linux the current potentially-dangerous code based on signal(SIGCHLD, child_handler). Regards, Tony tony.reix@atos.net ATOS / Bull SAS ATOS Expert IBM Coop Architect & Technical Leader Office : +33 (0) 4 76 29 72 67 1 rue de Provence - 38432 Échirolles - France www.atos.net --- ./daemon.c.ORIGIN 2019-03-18 17:53:51 +0100 +++ ./daemon.c 2019-03-18 18:00:16 +0100 @@ -943,8 +943,11 @@ * Otherwise empty handler because systemcalls will get interrupted * upon signal receipt * SysV needs the handler to be rearmed + * AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm it. */ +#ifndef _AIX signal(SIGCHLD, child_handler); +#endif } static int set_reuse_addr(int sockfd) @@ -1155,7 +1158,19 @@ pfd[i].events = POLLIN; } +#ifdef _AIX + /* AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm the SIGCHLD handler */ + struct sigaction sa; + + bzero(&sa, sizeof(sa)); + sa.sa_handler = child_handler; + sa.sa_flags = 0; + sigemptyset(&sa.sa_mask); + + sigaction(SIGCHLD, &sa, NULL); +#else signal(SIGCHLD, child_handler); +#endif for (;;) { int i; ``` ## Jeff King, 2019-04-22 19:52 Subject: Re: git on AIX: daemon.c & t5570-git-daemon.sh Message-ID: <20190422195226.GB22798@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190422195226.GB22798%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Mar 19, 2019 at 09:05:49AM +0000, REIX, Tony wrote: > When testing version 2.21.0 of git on AIX (6.1 & 7.2), I have found an > issue with daemon.c and test t5570-git-daemon.sh : within test 4, the > child_handler() code gets crazy and calls itself recursively till the > process crashes. We do not have a clear idea why this issue occurs. > Maybe that this issue also appears on other operating systems. Interesting. I think re-arming a signal() has been a well-understood technique for a while, but I am not too surprised that there would be a platform that doesn't like it. :) So I understand this hunk: > --- ./daemon.c.ORIGIN 2019-03-18 17:53:51 +0100 > +++ ./daemon.c 2019-03-18 18:00:16 +0100 > @@ -943,8 +943,11 @@ > * Otherwise empty handler because systemcalls will get interrupted > * upon signal receipt > * SysV needs the handler to be rearmed > + * AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm it. > */ > +#ifndef _AIX > signal(SIGCHLD, child_handler); > +#endif > } Although usually we would split this out to a Makefile knob. E.g., call it something like AVOID_REARMING_SIGNAL_HANDLERS or something, and then put it into config.mak.uname for AIX, so that it's turned on by default there. And then if other platforms need the same, they just need to add a similar Makefile line (and people can experiment by building with or without it). See how NO_INITGROUPS is used in daemon.c for prior art. Did you test on a daemon with this patch that when serving multiple clients it continues to handle SIGCHLD properly after the first instance? Specifically, we'd want to make sure that our accept() continues to be interrupted and we run check_dead_children() promptly. > static int set_reuse_addr(int sockfd) > @@ -1155,7 +1158,19 @@ > pfd[i].events = POLLIN; > } > > +#ifdef _AIX > + /* AIX does NOT like sometimes (t5570-git-daemon test 4) to rearm the SIGCHLD handler */ > + struct sigaction sa; > + > + bzero(&sa, sizeof(sa)); > + sa.sa_handler = child_handler; > + sa.sa_flags = 0; > + sigemptyset(&sa.sa_mask); > + > + sigaction(SIGCHLD, &sa, NULL); > +#else > signal(SIGCHLD, child_handler); > +#endif This hunk I don't quite understand. This signal() should be run only once, so does it matter if it's done via sigaction() or signal()? Does AIX require using sigaction() without SA_RESETHAND in order to not need the re-arming behavior? -Peff ```