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 8370 invoked from network); 15 Mar 2021 01:15:27 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 15 Mar 2021 01:15:27 -0000 Received: (qmail 30406 invoked by uid 550); 15 Mar 2021 01:15:25 -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 30388 invoked from network); 15 Mar 2021 01:15:25 -0000 Date: Sun, 14 Mar 2021 21:15:12 -0400 From: Rich Felker To: =?utf-8?B?U8O2cmVu?= Tempel Cc: musl@lists.openwall.com Message-ID: <20210315011512.GC32655@brightrain.aerifal.cx> References: <2HXA7ZPT82TJK.2KFTHDGNA32X6@8pit.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2HXA7ZPT82TJK.2KFTHDGNA32X6@8pit.net> 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 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