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

[Rich Felker <dalias@libc.org>]

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