Re: [PATCH] daemon: handle EINTR failures from waitpid()

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

Hi Carlo

On 30/06/2025 05:13, Carlo Marcelo Arenas Belón wrote:
> Since 695605b508 (git-daemon: Simplify dead-children reaping logic,
> 2008-08-14), the logic to check for zombie children was moved out of
> the SIGCHLD signal handler, but adding checks for a failed waitpid()
> were missed, with the possibility that a badly timed signal could
> prevent the promptly reaping of those defunct processes.
> 
> After the refactoring of 30e1560230 (daemon: use run-command api for
> async serving, 2010-11-04), that reproduced that bug, a single
> process could be skipped from reaping, so prevent that by adding the
> missing error handling, and while at it make sure that ECHILD (or
> other errors) are correctly reported as a BUG().

I agree with you analysis, I've left a couple of comments on the fix. I 
noticed this when I was reading the code to see how well it handled 
EINTR and decided it wasn't worth worrying about as we still collect the 
child the next time we call check_dead_children() but there is no harm 
in checking for EINTR here. It might be worth noting in the commit 
message that the linux man page for waitpid() explicitly says that EINTR 
cannot happen when WNOHANG is given though. I wonder if that is the case 
on other platforms as well because the calling thread is not suspended 
and EINTR is usually associated with calls that block.

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   daemon.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/daemon.c b/daemon.c
> index d1be61fd57..16ae66a2da 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -864,8 +864,11 @@ static void check_dead_children(void)
>   			live_children--;
>   			child_process_clear(&blanket->cld);
>   			free(blanket);
> -		} else
> +		} else if (!pid)

Our style guidelines say that if one clause of an if statement needs 
braces then all the clauses should be braced.

>   			cradle = &blanket->next;
> +		else if (errno != EINTR)
> +			BUG("invalid child '%" PRIuMAX "'",
> +			    (uintmax_t)blanket->cld.pid);

POSIX says pid_t is signed so I'm not sure about the unsigned cast here. 
Do any of the platforms we support have a pid_t that is wider than a 
long integer? I wondered if we should be logging an error instead of 
calling BUG() but I think any error other that EINTR indicates a 
programming error so BUG() seems appropriate.

Thanks

Phillip