[musl] popen needs to close streams created by previous calls

[musl] popen needs to close streams created by previous calls

[Sören Tempel]

Hi,

This is a follow-up to a discussion from IRC regarding a bug in popen().
The POSIX specification for popen() mandates the following [1]:

> The popen() function shall ensure that any streams from previous
> popen() calls that remain open in the parent process are closed in the
> new child process.

Currently, musl's popen() implementation does not adhere to this
requirement. When multiple popen() calls are used in an application,
newly created child processes will inherit the file descriptors for the
reading/writing end of pipes created by previous popen() calls. This can
lead to pclose() deadlocks when popen() has been used to create
multiples pipes which can be written to. As one would need to close the
writing end in all created child processes to cause an EOF on the
reading end [2].

Other implementations (e.g. glibc [3] or uclibc [4]) maintain an
internal list of pipes created by previous popen() calls and close them
in the child process created by popen(). Something similar will likely
be needed for musl as well.

On IRC, duncaen proposed the following patch to resolve this issue:

diff --git a/src/stdio/popen.c b/src/stdio/popen.c
index 92cb57ee..7233b08f 100644
--- a/src/stdio/popen.c
+++ b/src/stdio/popen.c
@@ -50,6 +50,12 @@ FILE *popen(const char *cmd, const char *mode)

 	e = ENOMEM;
 	if (!posix_spawn_file_actions_init(&fa)) {
+		for (FILE *f=*__ofl_lock(); f; f=f->next)
+			if (f->pipe_pid && posix_spawn_file_actions_addclose(&fa, f->fd)) {
+				__ofl_unlock();
+				goto fail;
+			}
+		__ofl_unlock();
 		if (!posix_spawn_file_actions_adddup2(&fa, p[1-op], 1-op)) {
 			if (!(e = posix_spawn(&pid, "/bin/sh", &fa, 0,
 			    (char *[]){ "sh", "-c", (char *)cmd, 0 }, __environ))) {

Further changes to the proposed patch could be discussed in this thread.

Greetings,
Sören

[1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/popen.html
[2]: https://github.com/leahneukirchen/mblaze/issues/203
[3]: https://sourceware.org/git/?p=glibc.git;a=blob;f=libio/iopopen.c;h=3afca7e173ef74eaf937b243b06ae40c2a590ec9#l63
[4]: https://git.uclibc.org/uClibc/tree/libc/stdio/popen.c?id=9b0f7d99899bf96510a4c3ea84218b7efb50f696#n88

Re: [musl] popen needs to close streams created by previous calls

[Rich Felker]

On Sun, Mar 14, 2021 at 05:04:34PM +0100, Sören Tempel wrote:
> Hi,
> 
> This is a follow-up to a discussion from IRC regarding a bug in popen().
> The POSIX specification for popen() mandates the following [1]:
> 
> > The popen() function shall ensure that any streams from previous
> > popen() calls that remain open in the parent process are closed in the
> > new child process.
> 
> Currently, musl's popen() implementation does not adhere to this
> requirement. When multiple popen() calls are used in an application,
> newly created child processes will inherit the file descriptors for the
> reading/writing end of pipes created by previous popen() calls. This can
> lead to pclose() deadlocks when popen() has been used to create
> multiples pipes which can be written to. As one would need to close the
> writing end in all created child processes to cause an EOF on the
> reading end [2].
> 
> Other implementations (e.g. glibc [3] or uclibc [4]) maintain an
> internal list of pipes created by previous popen() calls and close them
> in the child process created by popen(). Something similar will likely
> be needed for musl as well.
> 
> On IRC, duncaen proposed the following patch to resolve this issue:
> 
> diff --git a/src/stdio/popen.c b/src/stdio/popen.c
> index 92cb57ee..7233b08f 100644
> --- a/src/stdio/popen.c
> +++ b/src/stdio/popen.c
> @@ -50,6 +50,12 @@ FILE *popen(const char *cmd, const char *mode)
> 
>  	e = ENOMEM;
>  	if (!posix_spawn_file_actions_init(&fa)) {
> +		for (FILE *f=*__ofl_lock(); f; f=f->next)
> +			if (f->pipe_pid && posix_spawn_file_actions_addclose(&fa, f->fd)) {
> +				__ofl_unlock();
> +				goto fail;
> +			}
> +		__ofl_unlock();
>  		if (!posix_spawn_file_actions_adddup2(&fa, p[1-op], 1-op)) {
>  			if (!(e = posix_spawn(&pid, "/bin/sh", &fa, 0,
>  			    (char *[]){ "sh", "-c", (char *)cmd, 0 }, __environ))) {
> 
> Further changes to the proposed patch could be discussed in this thread.

Following up from what was discussed afterwards, the above does not
work because the open file list lock is not held across the entire
operation. It's taken and released once in the call to fdopen on the
new pipe fd, and once in the above patch hunk, and moreover released
before the call to posix_spawn. The former doesn't really matter
because the new pipe fd is close-on-exec during that window, but the
latter does: another popen call can happen between the __ofl_unlock
and posix_spawn, and we will miss any fd it opens.

I think this is salvagable just by moving the __ofl_unlock after the
posix_spawn (or by using a separate popen lock, but there are reasons
I'd prefer not to do that).

However this is also a second issue: the patch hunk above calls
(indirectly) malloc with a libc-internal lock held, which is against
policy since commits 8d37958d58cf36f53d5fcc7a8aa6d633da6071b2 and
34952fe5de44a833370cbe87b63fb8eec61466d7, as it places arbitrary and
undocumented restrictions on what libc functions a malloc replacement
can call.

The canonical solution here is just making posix_spawn_file_actions_*
always use the libc-internal malloc rather than the public one, and I
don't really see any other plausible approaches.

Rich

Re: [musl] popen needs to close streams created by previous calls

[Rich Felker]

On Sun, Mar 14, 2021 at 09:15:12PM -0400, Rich Felker wrote:
> On Sun, Mar 14, 2021 at 05:04:34PM +0100, Sören Tempel wrote:
> > Hi,
> > 
> > This is a follow-up to a discussion from IRC regarding a bug in popen().
> > The POSIX specification for popen() mandates the following [1]:
> > 
> > > The popen() function shall ensure that any streams from previous
> > > popen() calls that remain open in the parent process are closed in the
> > > new child process.
> > 
> > Currently, musl's popen() implementation does not adhere to this
> > requirement. When multiple popen() calls are used in an application,
> > newly created child processes will inherit the file descriptors for the
> > reading/writing end of pipes created by previous popen() calls. This can
> > lead to pclose() deadlocks when popen() has been used to create
> > multiples pipes which can be written to. As one would need to close the
> > writing end in all created child processes to cause an EOF on the
> > reading end [2].
> > 
> > Other implementations (e.g. glibc [3] or uclibc [4]) maintain an
> > internal list of pipes created by previous popen() calls and close them
> > in the child process created by popen(). Something similar will likely
> > be needed for musl as well.
> > 
> > On IRC, duncaen proposed the following patch to resolve this issue:
> > 
> > diff --git a/src/stdio/popen.c b/src/stdio/popen.c
> > index 92cb57ee..7233b08f 100644
> > --- a/src/stdio/popen.c
> > +++ b/src/stdio/popen.c
> > @@ -50,6 +50,12 @@ FILE *popen(const char *cmd, const char *mode)
> > 
> >  	e = ENOMEM;
> >  	if (!posix_spawn_file_actions_init(&fa)) {
> > +		for (FILE *f=*__ofl_lock(); f; f=f->next)
> > +			if (f->pipe_pid && posix_spawn_file_actions_addclose(&fa, f->fd)) {
> > +				__ofl_unlock();
> > +				goto fail;
> > +			}
> > +		__ofl_unlock();
> >  		if (!posix_spawn_file_actions_adddup2(&fa, p[1-op], 1-op)) {
> >  			if (!(e = posix_spawn(&pid, "/bin/sh", &fa, 0,
> >  			    (char *[]){ "sh", "-c", (char *)cmd, 0 }, __environ))) {
> > 
> > Further changes to the proposed patch could be discussed in this thread.
> 
> [...]
> 
> I think this is salvagable just by moving the __ofl_unlock after the
> posix_spawn (or by using a separate popen lock, but there are reasons
> I'd prefer not to do that).

Another small detail: it leaks memory if an addclose operation fails,
since "goto fail;" bypasses the destroy operation for the file_actions
object. This can be fixed just by moving the label, but the control
structure (mix of nested ifs and goto) is rather ugly.

Rich