[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 15 Mar 2021 15:18:48 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: popen needs to close streams created by previous calls
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
