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

From: Jeff King <peff@peff.net>
Date: Apr 22, 2019, 19:52 UTC
Message-ID: <20190422195226.GB22798@sigill.intra.peff.net>
In-Reply-To: <DB7PR02MB46634F68900E0F81B0110AF586400@DB7PR02MB4663.eurprd02.prod.outlook.com>

On Tue, Mar 19, 2019 at 09:05:49AM +0000, REIX, Tony wrote:

Show 5 quoted lines

> 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:

Show 12 quoted lines

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

Show 18 quoted lines

>  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