Re: [musl] Potentially infinite loop in posix_spawn'ed child

[Rich Felker <dalias@libc.org>]

On Thu, Feb 29, 2024 at 05:03:51PM +0300, Alexey Izbyshev wrote:
> On 2021-05-25 17:32, Rich Felker wrote:
> >On Tue, May 25, 2021 at 09:30:18AM +0300, Alexey Izbyshev wrote:
> >>On 2021-05-24 23:33, Rich Felker wrote:
> >>>On Mon, May 24, 2021 at 01:09:21PM +0300, Alexey Izbyshev wrote:
> >>>>Hi,
> >>>>
> >>>>I've noticed the following loop at https://git.musl-libc.org/cgit/musl/tree/src/process/posix_spawn.c#n159:
> >>>>
> >>>>    exec(args->path, args->argv, args->envp);
> >>>>    ret = -errno;
> >>>>
> >>>>fail:
> >>>>    /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> >>>>    ret = -ret;
> >>>>    if (ret) while (__syscall(SYS_write, p, &ret, sizeof ret) < 0);
> >>>>    _exit(127);
> >>>>
> >>>>Is there any reason that write is done in a loop? If SIGPIPE is
> >>>>blocked or ignored and the parent dies before this point, the child
> >>>>will spin in it forever.
> >>>
> >>>I suppose the special case of EPIPE should be considered here as no
> >>>need to inform the parent. Are there any other errors that should be
> >>>treated specially?
> >>>
> >>I'm not aware of any other errors that would need treatment. Is this
> >>loop intended to be a detection/debugging aid in case of an
> >>unexpected error?
> >
> >It's not a debugging aid so much as a guarantee against forward
> >progress doing the wrong thing (wrongly reporting success to the
> >parent when the execve failed). I don't think there are any errors
> >that should be able to happen here aside from EPIPE though, short of
> >munging with syscall semantics using seccomp or something which is
> >outside the scope of what could be expected to work correctly.
> >
> I've never sent a patch for this, doing it now.
> 
> Thanks,
> Alexey

> From 36eda01dbe0a35c4c65c394723a70c2d1b75e591 Mon Sep 17 00:00:00 2001
> From: Alexey Izbyshev <izbyshev@ispras.ru>
> Date: Thu, 29 Feb 2024 14:13:18 +0300
> Subject: [PATCH] posix_spawn: fix child spinning on write to a broken pipe
> Mail-Followup-To: musl@lists.openwall.com
> 
> A child process created by posix_spawn reports errors to its parent via
> a pipe, retrying infinitely on any write error to prevent falsely
> reporting success. If the (original) parent dies before write is
> attempted, there is nobody to report to, but the child will remain
> stuck in the write loop forever if SIGPIPE is blocked or ignored.
> Fix this by not retrying write if it fails with EPIPE.
> ---
>  src/process/posix_spawn.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/process/posix_spawn.c b/src/process/posix_spawn.c
> index 728551b36792..8294598bb7e3 100644
> --- a/src/process/posix_spawn.c
> +++ b/src/process/posix_spawn.c
> @@ -4,6 +4,7 @@
>  #include <unistd.h>
>  #include <signal.h>
>  #include <fcntl.h>
> +#include <errno.h>
>  #include <sys/wait.h>
>  #include "syscall.h"
>  #include "lock.h"
> @@ -156,7 +157,11 @@ static int child(void *args_vp)
>  fail:
>  	/* Since sizeof errno < PIPE_BUF, the write is atomic. */
>  	ret = -ret;
> -	if (ret) while (__syscall(SYS_write, p, &ret, sizeof ret) < 0);
> +	if (ret) {
> +		int r;
> +		do r = __syscall(SYS_write, p, &ret, sizeof ret);
> +		while (r<0 && r!=-EPIPE);
> +	}
>  	_exit(127);
>  }
>  
> -- 
> 2.39.2
> 

Thanks, applying!