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

[Alexey Izbyshev <izbyshev@ispras.ru>]

[-- Attachment #1: Type: text/plain, Size: 1615 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-posix_spawn-fix-child-spinning-on-write-to-a-broken-.patch --]
[-- Type: text/x-diff; name=0001-posix_spawn-fix-child-spinning-on-write-to-a-broken-.patch, Size: 1426 bytes --]

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