From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 13408 invoked from network); 15 Mar 2021 19:19:05 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 15 Mar 2021 19:19:05 -0000 Received: (qmail 18189 invoked by uid 550); 15 Mar 2021 19:19:02 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 18166 invoked from network); 15 Mar 2021 19:19:01 -0000 Date: Mon, 15 Mar 2021 15:18:48 -0400 From: Rich Felker To: musl@lists.openwall.com Message-ID: <20210315191848.GD32655@brightrain.aerifal.cx> References: <2HXA7ZPT82TJK.2KFTHDGNA32X6@8pit.net> <20210315011512.GC32655@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20210315011512.GC32655@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] 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