Re: [PATCH v3 3/4] daemon: use sigaction() to install child_handler()

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Carlo

On 26/06/2025 09:53, Carlo Marcelo Arenas Belón via GitGitGadget wrote:
> From: =?UTF-8?q?Carlo=20Marcelo=20Arenas=20Bel=C3=B3n?= <carenas@gmail.com>
> 
> In a future change, the flags used for processing SIGCHLD will need to
> be updated, which is only possible by using sigaction().

Only because you have chosen to use SA_RESTART here. I think it would be 
better to drop that and instead say something like


POSIX leaves several aspects of the behavior of signal() up to the 
implementer. It is unspecified whether long running syscalls are 
restarted after an interrupt is received.  The event loop in "git 
daemon" assumes that poll() will return with EINTR if a child exits 
while it is polling but if poll() is restarted after an interrupt is 
received that does not happen which can lead to a build up of 
uncollected zombie processes.

It is also unspecified whether the handler is reset after an interrupt 
is received. In order to work correctly on operating systems such as 
Solaris where the handler for SIGCLD is reset when a signal is received 
"git daemon" calls signal() from the SIGCLD handler to re-establish a 
handler for that signal. Unfortunately this causes infinite recursion on 
other operating systems such as AIX.

Both of these problems can be addressed by using sigaction() instead of 
signal() which has much stricter semantics and so, by setting the 
appropriate flags, we can rely on poll() being interrupted and the 
SIGCLD handler not being reset. This change means that all long running 
system calls could fail with EINTR not just pall() but rest of the event 
loop code is designed to gracefully handle that.

After this change there is still a race where a child that exits after 
it has been checked in check_dead_children() but before we call poll() 
will not be collected until a new connection is received or a child 
exits while we're polling. We could fix this by using the "self-pipe 
trick" but do not because ....


Then we can drop patches 1 and 4.

Best Wishes

Phillip

> 
> Replace signal() with an equivalent invocation of sigaction(), which
> has the added benefit of using BSD semantics reliably and therefore
> not needing the rearming call in the signal handler.
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   daemon.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/daemon.c b/daemon.c
> index d1be61fd5789..155b2e180167 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -915,11 +915,9 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
>   static void child_handler(int signo UNUSED)
>   {
>   	/*
> -	 * Otherwise empty handler because systemcalls will get interrupted
> -	 * upon signal receipt
> -	 * SysV needs the handler to be rearmed
> +	 * Otherwise empty handler because systemcalls should get interrupted
> +	 * upon signal receipt.
>   	 */
> -	signal(SIGCHLD, child_handler);
>   }
>   
>   static int set_reuse_addr(int sockfd)
> @@ -1120,6 +1118,7 @@ static void socksetup(struct string_list *listen_addr, int listen_port, struct s
>   
>   static int service_loop(struct socketlist *socklist)
>   {
> +	struct sigaction sa;
>   	struct pollfd *pfd;
>   
>   	CALLOC_ARRAY(pfd, socklist->nr);
> @@ -1129,7 +1128,10 @@ static int service_loop(struct socketlist *socklist)
>   		pfd[i].events = POLLIN;
>   	}
>   
> -	signal(SIGCHLD, child_handler);
> +	sigemptyset(&sa.sa_mask);
> +	sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
> +	sa.sa_handler = child_handler;
> +	sigaction(SIGCHLD, &sa, NULL);
>   
>   	for (;;) {
>   		check_dead_children();