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

[Bart Schaefer <schaefer@brasslantern.com>]

On Dec 21, 10:34pm, Peter Stephenson wrote:
}
} Unfortunately it (the patch in 32171) doesn't fix the problem for me.

Drat.

} 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).

I wondered if that would be necessary, but couldn't ever manage to get
DPUTS() statements in those two places to print anything, so concluded
that the issue was in the place that I did patch.

What concerns me is whether we might be closing too many file descriptors
if we remove all is_fd entries from filelist at that point, but if that's
in the parent that's going to do nothing but wait for the child it should
be OK.
 
} 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).

Argh!  Did "git commit" but forgot "git push".  Did that now.

Here's your new bit of 32175 plus an additional regression test (fails
with just 32171 but succeeds after 32175).


diff --git a/Src/exec.c b/Src/exec.c
index 4480033..f16cfd3 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2389,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];
@@ -2911,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;
@@ -3264,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 */
@@ -3342,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) &&
diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index 61d24fe..6abfd8b 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -207,10 +207,12 @@ F:This similar test was triggering a reproducible failure with pipestatus.
   coproc { read -Et 5 || kill -INT $$ }
   print -u $ZTST_fd 'This test takes 5 seconds to fail...'
   { printf "%d\n" {1..20000} } | ( read -E )
+  hang(){ printf "%d\n" {2..20000} | cat }; hang | ( read -E )
   print -p done
   read -Ep
 0:Bug regression: piping a shell construct to an external process may hang
 >1
+>2
 >done
 F:This test checks for a file descriptor leak that could cause the left
 F:side of a pipe to block on write after the right side has exited