[PATCH v3] redir: Handle nested exec within REALLY_CLOSED redirection

[Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>]

On Fri, Dec 14, 2018 at 01:01:12AM +0000, Martijn Dekker wrote:
>
> > So did anything happen on the bash front? I'm happy to change if
> > bash moves in the same direction.
> 
> Yes. According to the bash changelog, Chet fixed it in git last 30th of
> April, meaning it'll be in bash 5.0.
> 
> Patch attached, as per Harald's suggestion.

Thanks for the patch.  I took a deeper look into the history of
the bug and it turned out that I added REALLY_CLOSED as an
optimisation in order to avoid an unnecessary close(2) syscall.

Removing REALLY_CLOSED would mean an unnecessary close(2) in
such cases.

So I'm going to go for a more complicated fix:

---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 going up one level when popping redirections
and changing REALLY_CLOSED back to CLOSED when we encounter the
nested exec(1) command.

Reported-by: Martijn Dekker <martijn@xxxxxxxx>
Fixes: ce0f1900d869 ("[REDIR] Fix redirect restore on saved file...")
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>

diff --git a/src/redir.c b/src/redir.c
index e67cc0a..4f84a80 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -128,12 +128,12 @@ redirect(union node *redir, int flags)
 			p = &sv->renamed[fd];
 			i = *p;
 
-			if (likely(i == EMPTY)) {
-				i = CLOSED;
-				if (fd != newfd) {
+			if (likely(i <= EMPTY)) {
+				if (i == EMPTY && fd != newfd) {
 					i = savefd(fd, fd);
 					fd = -1;
-				}
+				} else
+					i = CLOSED;
 			}
 
 			if (i == newfd)
@@ -340,16 +340,20 @@ out:
 void
 popredir(int drop)
 {
+	struct redirtab *nrp;
 	struct redirtab *rp;
 	int i;
 
 	INTOFF;
 	rp = redirlist;
+	nrp = rp->next;
 	for (i = 0 ; i < 10 ; i++) {
 		switch (rp->renamed[i]) {
 		case CLOSED:
 			if (!drop)
 				close(i);
+			else if (nrp && nrp->renamed[i] == REALLY_CLOSED)
+				nrp->renamed[i] = CLOSED;
 			break;
 		case EMPTY:
 		case REALLY_CLOSED:
@@ -361,7 +365,7 @@ popredir(int drop)
 			break;
 		}
 	}
-	redirlist = rp->next;
+	redirlist = nrp;
 	ckfree(rp);
 	INTON;
 }
@@ -451,8 +455,17 @@ struct redirtab *pushredir(union node *redir)
 	sv = ckmalloc(sizeof (struct redirtab));
 	sv->next = q;
 	redirlist = sv;
-	for (i = 0; i < 10; i++)
-		sv->renamed[i] = EMPTY;
+	for (i = 0; i < 10; i++) {
+		int val = EMPTY;
+
+		if (q) {
+			val = q->renamed[i];
+			if (val > EMPTY)
+				val = EMPTY;
+		}
+
+		sv->renamed[i] = val;
+	}
 
 out:
 	return q;
-- 
Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt