Re: bug with eval, proc-subst and pipes

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

On Sat, 20 Jul 2013 21:25:31 +0100
Stephane Chazelas <stephane.chazelas@xxxxxxxxx> wrote:
> It's better:
> 
> $ eval 'ls -l <(echo 1) <(echo 2) <(echo 3) <(echo 4)' | cat
> ls: cannot access /proc/self/fd/11: No such file or directory
> lr-x------ 1 chazelas chazelas 64 Jul 20 21:23 /proc/self/fd/10 -> pipe:[1541173]
> lr-x------ 1 chazelas chazelas 64 Jul 20 21:23 /proc/self/fd/12 -> pipe:[1541175]
> lr-x------ 1 chazelas chazelas 64 Jul 20 21:23 /proc/self/fd/13 -> pipe:[1541176]
> 
> Only one now instead of two.

OK, getting somewhere...

In that case, your previous mail suggests zclose(subsh_close) is the
other thing that needs looking at.

It seems to me that subsh_close is liable to the same problem as I
(eventually) fixed in the example Bart came up with.  Any chance the
following patch resolves the issue?  (I checked and there shouldn't be
any work for Vincent this time round...)

One of the lines I removed,

-    if (subsh_close >= 0 && fdtable[subsh_close] == FDT_UNUSED)
-	subsh_close = -1;

suggests we might be on the right track here... at some point, someone
was obviously worried that subsh_close might already have been closed,
and if that's the case it's entirely possible that it was also reopened
for a different purpose, which would be what you're seeing.

Paranoid note: the traditional problem with associating new things with
jobs, as addfilelist() does, is you find out there isn't a job because
it's been optimised out.  However, I believe that by the time we've got
as far as execpline2() the case is sufficiently complicated it must have
a job.  (Ultra paranoid note: in which case we *must* ensure the job is
cleared up, right?  There's no possibility of a file descriptor leak?)
(If you understood that I will endorse your Unix experience on
LinkedIn...)

diff --git a/Src/exec.c b/Src/exec.c
index f9efb3a..1c44565 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1653,8 +1653,6 @@ execpline(Estate state, wordcode slcode, int how, int last1)
     return lastval;
 }
 
-static int subsh_close = -1;
-
 /* execute pipeline.  This function assumes the `pline' is not NULL. */
 
 /**/
@@ -1731,7 +1729,7 @@ execpline2(Estate state, wordcode pcode,
 	    }
 	} else {
 	    /* otherwise just do the pipeline normally. */
-	    subsh_close = pipes[0];
+	    addfilelist(NULL, pipes[0]);
 	    execcmd(state, input, pipes[1], how, 0);
 	}
 	zclose(pipes[1]);
@@ -1744,8 +1742,6 @@ execpline2(Estate state, wordcode pcode,
 	execpline2(state, *state->pc++, how, pipes[0], output, last1);
 	list_pipe = old_list_pipe;
 	cmdpop();
-	zclose(pipes[0]);
-	subsh_close = -1;
     }
 }
 
@@ -2162,8 +2158,6 @@ addfd(int forked, int *save, struct multio **mfds, int fd1, int fd2, int rflag,
 	    mfds[fd1]->fds[mfds[fd1]->ct++] = fdN;
 	}
     }
-    if (subsh_close >= 0 && fdtable[subsh_close] == FDT_UNUSED)
-	subsh_close = -1;
 }
 
 /**/
@@ -3245,11 +3239,6 @@ execcmd(Estate state, int input, int output, int how, int last1)
 
 	    if (is_shfunc) {
 		/* It's a shell function */
-
-		if (subsh_close >= 0)
-		    zclose(subsh_close);
-		subsh_close = -1;
-
 		execshfunc((Shfunc) hn, args);
 	    } else {
 		/* It's a builtin */
@@ -3325,9 +3314,6 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		DPUTS(varspc,
 		      "BUG: assignment before complex command");
 		list_pipe = 0;
-		if (subsh_close >= 0)
-		    zclose(subsh_close);
-                subsh_close = -1;
 		/* If we're forked (and we should be), no need to return */
 		DPUTS(last1 != 1 && !forked, "BUG: not exiting?");
 		DPUTS(type != WC_SUBSH, "Not sure what we're doing.");
@@ -5069,7 +5055,6 @@ execsave(void)
     es->trapisfunc = trapisfunc;
     es->traplocallevel = traplocallevel;
     es->noerrs = noerrs;
-    es->subsh_close = subsh_close;
     es->underscore = ztrdup(zunderscore);
     es->next = exstack;
     exstack = es;
@@ -5100,7 +5085,6 @@ execrestore(void)
     trapisfunc = exstack->trapisfunc;
     traplocallevel = exstack->traplocallevel;
     noerrs = exstack->noerrs;
-    subsh_close = exstack->subsh_close;
     setunderscore(exstack->underscore);
     zsfree(exstack->underscore);
     en = exstack->next;
diff --git a/Src/zsh.h b/Src/zsh.h
index ebd3cb7..d7b130c 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -977,7 +977,6 @@ struct execstack {
     int trapisfunc;
     int traplocallevel;
     int noerrs;
-    int subsh_close;
     char *underscore;
 };
 
-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/