From patchwork Fri Jan 18 05:01:18 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 10769427 X-Patchwork-Delegate: herbert@gondor.apana.org.au Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 583491390 for ; Fri, 18 Jan 2019 05:01:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3ED812EDC4 for ; Fri, 18 Jan 2019 05:01:28 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2EEA22EE00; Fri, 18 Jan 2019 05:01:28 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 844E82EDC4 for ; Fri, 18 Jan 2019 05:01:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726401AbfARFB1 (ORCPT ); Fri, 18 Jan 2019 00:01:27 -0500 Received: from orcrist.hmeau.com ([104.223.48.154]:48178 "EHLO deadmen.hmeau.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726226AbfARFB0 (ORCPT ); Fri, 18 Jan 2019 00:01:26 -0500 Received: from gondobar.mordor.me.apana.org.au ([192.168.128.4] helo=gondobar) by deadmen.hmeau.com with esmtps (Exim 4.89 #2 (Debian)) id 1gkMHR-0007b0-5J; Fri, 18 Jan 2019 13:01:21 +0800 Received: from herbert by gondobar with local (Exim 4.89) (envelope-from ) id 1gkMHO-0000i6-Hc; Fri, 18 Jan 2019 13:01:18 +0800 Date: Fri, 18 Jan 2019 13:01:18 +0800 From: Herbert Xu To: Martijn Dekker Cc: Harald van Dijk , dash@vger.kernel.org Subject: [PATCH v4] redir: Handle nested exec within REALLY_CLOSED redirection Message-ID: <20190118050118.5wqov6idhyrtenbh@gondor.apana.org.au> References: <20180919042550.6z2rhszi4m5r6vqa@gondor.apana.org.au> <7bd61c96-8c14-aaba-d410-1ec4335a0608@inlv.org> <20190116133231.mx53nyjhgrfiflch@gondor.apana.org.au> <6ad455fe-f68d-154e-c314-9f4d9d69ff34@inlv.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6ad455fe-f68d-154e-c314-9f4d9d69ff34@inlv.org> User-Agent: NeoMutt/20170113 (1.7.2) Sender: dash-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: dash@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jan 17, 2019 at 11:07:23PM +0100, Martijn Dekker wrote: > Op 16-01-19 om 14:32 schreef Herbert Xu: > > So I'm going to go for a more complicated fix: > > The v3 patch introduces a bug: > > ====begin test script==== > init() { > exec 8 } >&- > > { > init > : <&8 && echo ok > } 8<&- > ====end test script==== > > Actual output: > test2.sh: 9: test2.sh: 8: Bad file descriptor > > Expected output: > ok > > The 'read' gets a 'bad file descriptor', so the effect of the 'exec' > from the init function is lost. > > Interestingly, removing either the ">&-" from the function definition > block, or the "8<&-" from the other braces block, or both, makes the > error go away. Thanks for testing this. Yes my patch was completely broken. We need to track the current status separately from the previous state which is what redirlist does. ---8<--- The value of REALLY_CLOSED is used to avoid an unnecessary close(2) call when restoring redirections. However, as it stands it can remove a close(2) call that's actually needed. This happens when an enclosed exec(1) command leaves an open file descriptor behind. This patch fixes this by replacing REALLY_CLOSED with closed_redirs to track the current status of redirected file descriptors and leaving redirlist to only handle the previous state of redirected file descriptors. Reported-by: Martijn Dekker Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...") Signed-off-by: Herbert Xu diff --git a/src/redir.c b/src/redir.c index e67cc0a..6c81dd0 100644 --- a/src/redir.c +++ b/src/redir.c @@ -57,7 +57,6 @@ #include "error.h" -#define REALLY_CLOSED -3 /* fd that was closed and still is */ #define EMPTY -2 /* marks an unused slot in redirtab */ #define CLOSED -1 /* fd opened for redir needs to be closed */ @@ -77,6 +76,9 @@ struct redirtab { MKINIT struct redirtab *redirlist; +/* Bit map of currently closed file descriptors. */ +static unsigned closed_redirs; + STATIC int openredirect(union node *); #ifdef notyet STATIC void dupredirect(union node *, int, char[10]); @@ -86,6 +88,20 @@ STATIC void dupredirect(union node *, int); STATIC int openhere(union node *); +static unsigned update_closed_redirs(int fd, int nfd) +{ + unsigned val = closed_redirs; + unsigned bit = 1 << fd; + + if (nfd >= 0) + closed_redirs &= ~bit; + else + closed_redirs |= bit; + + return val & bit; +} + + /* * Process a list of redirection commands. If the REDIR_PUSH flag is set, * old file descriptors are stashed away so that the redirection can be @@ -125,21 +141,21 @@ redirect(union node *redir, int flags) fd = n->nfile.fd; if (sv) { + int closed; + p = &sv->renamed[fd]; i = *p; + closed = update_closed_redirs(fd, newfd); + if (likely(i == EMPTY)) { i = CLOSED; - if (fd != newfd) { + if (fd != newfd && !closed) { i = savefd(fd, fd); fd = -1; } } - if (i == newfd) - /* Can only happen if i == newfd == CLOSED */ - i = REALLY_CLOSED; - *p = i; } @@ -346,14 +362,18 @@ popredir(int drop) INTOFF; rp = redirlist; for (i = 0 ; i < 10 ; i++) { + int closed; + + if (rp->renamed[i] == EMPTY) + continue; + + closed = drop ? 1 : update_closed_redirs(i, rp->renamed[i]); + switch (rp->renamed[i]) { case CLOSED: - if (!drop) + if (!closed) close(i); break; - case EMPTY: - case REALLY_CLOSED: - break; default: if (!drop) dup2(rp->renamed[i], i);