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</dev/null
> } >&-
>
> {
> 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 <[email protected]>
Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...")
Signed-off-by: Herbert Xu <[email protected]>
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);
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
