mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Potentially infinite loop in posix_spawn'ed child
@ 2021-05-24 10:09 Alexey Izbyshev
  2021-05-24 15:40 ` Szabolcs Nagy
  2021-05-24 20:33 ` Rich Felker
  0 siblings, 2 replies; 8+ messages in thread
From: Alexey Izbyshev @ 2021-05-24 10:09 UTC (permalink / raw)
  To: musl

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

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.

A test case is attached. It overrides execve() to abuse it as a 
callback, avoiding reliance on timings.

As an aside, perhaps it would make sense to call execve() in 
posix_spawn() implementation via a hidden symbol? This would both make 
it consistent with posix_spawnp() and avoid any trouble with user code 
executing in a vfork'ed child if execve() is overridden via e.g. 
LD_PRELOAD.

Alexey

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: test.c --]
[-- Type: text/x-c; name=test.c, Size: 440 bytes --]

#include <errno.h>
#include <signal.h>
#include <spawn.h>
#include <unistd.h>

static int p[2];

int execve(const char *path, char *const argv[], char *const envp[]) {
  close(p[1]);
  kill(getppid(), SIGKILL);
  read(p[0], &(char){0}, 1);
  errno = ENOENT;
  return -1;
}

int main(int argc, char *argv[], char *envp[]) {
    signal(SIGPIPE, SIG_IGN);
    pipe(p);
    posix_spawn(0L, "/non-existing", 0L, 0L, argv, envp);
    return 0;
}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-24 10:09 [musl] Potentially infinite loop in posix_spawn'ed child Alexey Izbyshev
@ 2021-05-24 15:40 ` Szabolcs Nagy
  2021-05-24 16:50   ` Alexey Izbyshev
  2021-05-24 20:33 ` Rich Felker
  1 sibling, 1 reply; 8+ messages in thread
From: Szabolcs Nagy @ 2021-05-24 15:40 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

* Alexey Izbyshev <izbyshev@ispras.ru> [2021-05-24 13:09:21 +0300]:
> As an aside, perhaps it would make sense to call execve() in posix_spawn()
> implementation via a hidden symbol? This would both make it consistent with
> posix_spawnp() and avoid any trouble with user code executing in a vfork'ed
> child if execve() is overridden via e.g. LD_PRELOAD.

libc symbols cannot be interposed by user code, they are
all resolved within the libc, there should be no dynamic
symbolic relocation in libc.so except for global data and
malloc symbols (those are explicitly exported).

so musl posix_spawn always calls musl's execve.
except with static linking you cannot hide libc symbols
like that unless musl uses a different name internally
(but that prevents the compiler recognizing builtins in
libc code if we ever want to optimize them).

> #include <errno.h>
> #include <signal.h>
> #include <spawn.h>
> #include <unistd.h>
> 
> static int p[2];
> 
> int execve(const char *path, char *const argv[], char *const envp[]) {

this is only called with static linking, but that is not
supportable usage: libc may provide execve and e.g. signal
in the same object file and then there is a conflict
because signal has to be linked. so this is just a user
error.

>   close(p[1]);
>   kill(getppid(), SIGKILL);
>   read(p[0], &(char){0}, 1);
>   errno = ENOENT;
>   return -1;
> }
> 
> int main(int argc, char *argv[], char *envp[]) {
>     signal(SIGPIPE, SIG_IGN);
>     pipe(p);
>     posix_spawn(0L, "/non-existing", 0L, 0L, argv, envp);
>     return 0;
> }


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-24 15:40 ` Szabolcs Nagy
@ 2021-05-24 16:50   ` Alexey Izbyshev
  0 siblings, 0 replies; 8+ messages in thread
From: Alexey Izbyshev @ 2021-05-24 16:50 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: musl

On 2021-05-24 18:40, Szabolcs Nagy wrote:
> * Alexey Izbyshev <izbyshev@ispras.ru> [2021-05-24 13:09:21 +0300]:
>> As an aside, perhaps it would make sense to call execve() in 
>> posix_spawn()
>> implementation via a hidden symbol? This would both make it consistent 
>> with
>> posix_spawnp() and avoid any trouble with user code executing in a 
>> vfork'ed
>> child if execve() is overridden via e.g. LD_PRELOAD.
> 
> libc symbols cannot be interposed by user code, they are
> all resolved within the libc, there should be no dynamic
> symbolic relocation in libc.so except for global data and
> malloc symbols (those are explicitly exported).
> 
Thanks, I wasn't aware of musl's usage of --dynamic-list and thought 
that weak aliases to hidden symbols (like "weak_alias(__pthread_exit, 
pthread_exit)") are there to prevent interposition.

>> int execve(const char *path, char *const argv[], char *const envp[]) {
> 
> this is only called with static linking, but that is not
> supportable usage: libc may provide execve and e.g. signal
> in the same object file and then there is a conflict
> because signal has to be linked. so this is just a user
> error.
> 
I never intended this test case to be "supportable usage". This is just 
a hack to make the problem with infinite loop reliably reproducible.

Alexey

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-24 10:09 [musl] Potentially infinite loop in posix_spawn'ed child Alexey Izbyshev
  2021-05-24 15:40 ` Szabolcs Nagy
@ 2021-05-24 20:33 ` Rich Felker
  2021-05-25  6:30   ` Alexey Izbyshev
  1 sibling, 1 reply; 8+ messages in thread
From: Rich Felker @ 2021-05-24 20:33 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

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?

> A test case is attached. It overrides execve() to abuse it as a
> callback, avoiding reliance on timings.

As noted that's undefined but it's completely reasonable as a way to
do the testing. Using seccomp to make the parent die might be slightly
less hackish but it doesn't matter. Thanks for the report.

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-24 20:33 ` Rich Felker
@ 2021-05-25  6:30   ` Alexey Izbyshev
  2021-05-25 14:32     ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Alexey Izbyshev @ 2021-05-25  6:30 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl

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?

Alexey

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-25  6:30   ` Alexey Izbyshev
@ 2021-05-25 14:32     ` Rich Felker
  2024-02-29 14:03       ` Alexey Izbyshev
  0 siblings, 1 reply; 8+ messages in thread
From: Rich Felker @ 2021-05-25 14:32 UTC (permalink / raw)
  To: Alexey Izbyshev; +Cc: musl

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.

Rich

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2021-05-25 14:32     ` Rich Felker
@ 2024-02-29 14:03       ` Alexey Izbyshev
  2024-02-29 15:35         ` Rich Felker
  0 siblings, 1 reply; 8+ messages in thread
From: Alexey Izbyshev @ 2024-02-29 14:03 UTC (permalink / raw)
  To: musl

[-- 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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [musl] Potentially infinite loop in posix_spawn'ed child
  2024-02-29 14:03       ` Alexey Izbyshev
@ 2024-02-29 15:35         ` Rich Felker
  0 siblings, 0 replies; 8+ messages in thread
From: Rich Felker @ 2024-02-29 15:35 UTC (permalink / raw)
  To: musl

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!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-02-29 15:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-24 10:09 [musl] Potentially infinite loop in posix_spawn'ed child Alexey Izbyshev
2021-05-24 15:40 ` Szabolcs Nagy
2021-05-24 16:50   ` Alexey Izbyshev
2021-05-24 20:33 ` Rich Felker
2021-05-25  6:30   ` Alexey Izbyshev
2021-05-25 14:32     ` Rich Felker
2024-02-29 14:03       ` Alexey Izbyshev
2024-02-29 15:35         ` Rich Felker

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).