[v4] redir: Handle nested exec within REALLY_CLOSED redirection

Commit Message

Herbert Xu Jan. 18, 2019, 5:01 a.m. UTC

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 <[e-mail]>
Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...")
Signed-off-by: Herbert Xu <[e-mail]>

Patch

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);