Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function freeze

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

On Sat, 21 Dec 2013 12:57:18 -0800
Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
> As far as I can tell this fixes the problem.  It'd be nice to hear from
> Vincent for confirmation but as that may not happen in time I'd say you
> should go ahead.

Unfortunately it (the patch in 32171) doesn't fix the problem for me.
With

foo() { ls -R / }
foo | head

I'm still getting a hang.

I think we're forking inside execcmd() after adding pipes[0] to the
filelist for thisjob.  This subshell is what's going to form the LHS of
the pipeline --- and we entersubsh() which will clear the job table.
So I think we need to salvage the filelist from the job table and remove
the pipe file descriptors in the danger cases, which I take to be the
places where we were handling subsh_close in the old version of the code
(where we are handling nested shell constructs of some sort).

The following does seem to fix the hang here and not cause any new test
failures.  Note it includes your code change, but not your regression
test (you hadn't pushed either yet last I looked).

The change to zwaitjob() is just pulling out the common code shared with
the two new cases.

I'm not going to commit this and make a new release just on the basis of
my guesses, however.

diff --git a/Src/exec.c b/Src/exec.c
index dccdc2b..f16cfd3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -1691,6 +1691,7 @@ execpline2(Estate state, wordcode pcode,
 	execcmd(state, input, output, how, last1 ? 1 : 2);
     else {
 	int old_list_pipe = list_pipe;
+	int subsh_close = -1;
 	Wordcode next = state->pc + (*state->pc), pc;
 	wordcode code;
 
@@ -1738,6 +1739,7 @@ execpline2(Estate state, wordcode pcode,
 	} else {
 	    /* otherwise just do the pipeline normally. */
 	    addfilelist(NULL, pipes[0]);
+	    subsh_close = pipes[0];
 	    execcmd(state, input, pipes[1], how, 0);
 	}
 	zclose(pipes[1]);
@@ -1750,6 +1752,8 @@ execpline2(Estate state, wordcode pcode,
 	execpline2(state, *state->pc++, how, pipes[0], output, last1);
 	list_pipe = old_list_pipe;
 	cmdpop();
+	if (subsh_close != pipes[0])
+	    zclose(pipes[0]);
     }
 }
 
@@ -2385,7 +2389,7 @@ static void
 execcmd(Estate state, int input, int output, int how, int last1)
 {
     HashNode hn = NULL;
-    LinkList args;
+    LinkList args, filelist = NULL;
     LinkNode node;
     Redir fn;
     struct multio *mfds[10];
@@ -2907,6 +2911,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    flags |= ESUB_KEEPTRAP;
 	if (type == WC_SUBSH && !(how & Z_ASYNC))
 	    flags |= ESUB_JOB_CONTROL;
+	filelist = jobtab[thisjob].filelist;
 	entersubsh(flags);
 	close(synch[1]);
 	forked = 1;
@@ -3260,6 +3265,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 
 	    if (is_shfunc) {
 		/* It's a shell function */
+		pipecleanfilelist(filelist);
 		execshfunc((Shfunc) hn, args);
 	    } else {
 		/* It's a builtin */
@@ -3338,6 +3344,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		DPUTS(varspc,
 		      "BUG: assignment before complex command");
 		list_pipe = 0;
+		pipecleanfilelist(filelist);
 		/* 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.");
diff --git a/Src/jobs.c b/Src/jobs.c
index 371b8eb..a321172 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1173,6 +1173,30 @@ addfilelist(const char *name, int fd)
     zaddlinknode(ll, jf);
 }
 
+/* Clean up pipes no longer needed associated with a job */
+
+/**/
+void
+pipecleanfilelist(LinkList filelist)
+{
+    LinkNode node;
+
+    if (!filelist)
+	return;
+    node = firstnode(filelist);
+    while (node) {
+	Jobfile jf = (Jobfile)getdata(node);
+	if (jf->is_fd) {
+	    LinkNode next = nextnode(node);
+	    zclose(jf->u.fd);
+	    (void)remnode(filelist, node);
+	    zfree(jf, sizeof(*jf));
+	    node = next;
+	} else
+	    incnode(node);
+    }
+}
+
 /* Finished with list of files for a job */
 
 /**/
@@ -1415,19 +1439,7 @@ zwaitjob(int job, int wait_cmd)
 	     * we can't deadlock on the fact that those still exist, so
 	     * that's not a problem.
 	     */
-	    LinkNode node = firstnode(jn->filelist);
-	    while (node) {
-		Jobfile jf = (Jobfile)getdata(node);
-		if (jf->is_fd) {
-		    LinkNode next = nextnode(node);
-		    (void)remnode(jn->filelist, node);
-		    zclose(jf->u.fd);
-		    zfree(jf, sizeof(*jf));
-		    node = next;
-		} else {
-		    incnode(node);
-		}
-	    }
+	    pipecleanfilelist(jn->filelist);
 	}
 	while (!errflag && jn->stat &&
 	       !(jn->stat & STAT_DONE) &&

-- 
Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/