git on AIX: daemon.c & t5570-git-daemon.sh

git on AIX: daemon.c & t5570-git-daemon.sh

[REIX, Tony]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

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            

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: git-2.21.0-SIGCHLD-handler-v3.patch --]
[-- Type: text/x-patch; name="git-2.21.0-SIGCHLD-handler-v3.patch", Size: 827 bytes --]

--- ./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;

Re: git on AIX: daemon.c & t5570-git-daemon.sh

[Jeff King]

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