Re: [PATCH] posix: Fix improper assert in Linux posix_spawn (BZ#22273)

[Florian Weimer <fw at deneb dot enyo dot de>]

* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index dea1650..d6acc1e 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -365,9 +365,17 @@ __spawnix (pid_t * pid, const char *file,
>    if (new_pid > 0)
>      {
>        ec = args.err;
> -      assert (ec >= 0);
>        if (ec != 0)
> -	  __waitpid (new_pid, NULL, 0);
> +	{
> +	  /* It handles the unlikely case where the auxiliary process is
> +	     terminated by a signal before calling _exit or execve.  For
> +	     this case the signal is relayed to the called, as the spawn
> +	     was successful.  */
> +	  int status;
> +	  __waitpid (new_pid, &status, WNOHANG);
> +	  if (WIFSIGNALED (status))
> +	    ec = 0;
> +	}

I'm sorry to say, but this does not look correct to me.  A SIGCHLD
handler may have collected the PID at the point of the waitpid call,
so it can fail with ECHLD or collect the wait notification for another
subprocess due to PID reuse.

I think we need to treat ec == -1 (process did not proceed to the
execve call because it was terminated by a signal) just like ec == 0
(process made a successful execve call, or was terminated just before
the system call, or just after a failed execve, again due to signals).
That is, return the PID to the caller and let it handle the abnormal
process termination due t a signal.

So the assert needs to be removed and the condition changed from ec !=
0 to ec > 0.  And this needs plenty of comments, explaining that we
report subprocess termination due to a signal to the caller and cannot
handle it locally due to the PID reuse race.

I'm not sure if it is necessary to initialize args.err (and ec) to -1
in the first place and reset it in the subprocess.  It seems to me
that this can be simplified a bit because we only need the non-zero
value to carry the information that execve failed, and the error code
with which it failed.