mailing list of musl libc
 help / color / mirror / Atom feed
From: Rich Felker <dalias@libc.org>
To: "Sören Tempel" <soeren@soeren-tempel.net>
Cc: musl@lists.openwall.com
Subject: Re: [musl] popen needs to close streams created by previous calls
Date: Sun, 14 Mar 2021 21:15:12 -0400
Message-ID: <20210315011512.GC32655@brightrain.aerifal.cx> (raw)
In-Reply-To: <2HXA7ZPT82TJK.2KFTHDGNA32X6@8pit.net>

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

  reply	other threads:[~2021-03-15  1:15 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 [this message]
2021-03-15 19:18   ` Rich Felker

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=20210315011512.GC32655@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    --cc=soeren@soeren-tempel.net \
    /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