mailing list of musl libc
 help / color / mirror / Atom feed
From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [musl] popen needs to close streams created by previous calls
Date: Mon, 15 Mar 2021 15:18:48 -0400
Message-ID: <20210315191848.GD32655@brightrain.aerifal.cx> (raw)
In-Reply-To: <20210315011512.GC32655@brightrain.aerifal.cx>

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

      reply	other threads:[~2021-03-15 19:19 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-14 16:04 Sören Tempel
2021-03-15  1:15 ` Rich Felker
2021-03-15 19:18   ` Rich Felker [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210315191848.GD32655@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

mailing list of musl libc

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.vuxu.org/musl

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 musl musl/ http://inbox.vuxu.org/musl \
		musl@inbox.vuxu.org
	public-inbox-index musl

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.musl


code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git