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

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

[Axel Beckert]

Hi,

this has been reported[1] against zsh 5.0.3 in Debian today. Looks like
some regression in 5.0.3 on a first glance. Maybe related to the
test-suite freezes which have been reported on a few architectures.

[1] http://bugs.debian.org/732726

Since 5.0.4 seems close, I thought I'd better forward it soon despite
I haven't looked at it closer yet.

----- Forwarded message from Vincent Lefevre <vincent@vinc17.net> -----
Date: Fri, 20 Dec 2013 19:14:36 +0100
From: Vincent Lefevre <vincent@vinc17.net>
Subject: [Pkg-zsh-devel] Bug#732726: zsh function freeze
Reply-To: Vincent Lefevre <vincent@vinc17.net>, 732726@bugs.debian.org

Package: zsh
Version: 5.0.3-1
Severity: important

This is an important regression (no problems after downgrading to
5.0.2-6), always reproducible.

With a private SVN repository, "svncdiff -c65935 ~/wd | head" freezes, where
svncdiff is the following zsh function (I have "autoload -U svncdiff"):

------------------------------------------------------------------------
#!/usr/bin/env zsh

# Wrapper to "svn diff", written by Vincent Lefevre <vincent@vinc17.org>

# Needs my tdiff utility to process the diff output; otherwise you need
# to remove the "| tdiff" at the end.

# Example of svncdiff usage:
#   svncdiff -5 -x -p file
# for 5 lines of unified context and function information.

emulate -LR zsh
local -a args xopt
setopt EXTENDED_GLOB

while [[ $# -ge 1 ]]
do
  if [[ "x$1" == x-[0-9]# ]] then
    args=($args --diff-cmd diff)
    xopt=($xopt -U${1[2,-1]})
  elif [[ $# -ge 2 && "x$1" == x-x ]] then
    shift
    xopt=($xopt $1)
  else
    args=($args $1)
  fi
  shift
done

[[ $#xopt -ge 1 ]] && args=(-x "$xopt" $args)
svnwrapper diff "$args[@]" | tdiff

# $Id: svncdiff 38442 2010-08-05 11:41:16Z vinc17/ypig $
------------------------------------------------------------------------

The dependencies can be found on <https://www.vinc17.net/unix/>.

When svncdiff is called as a script, this is no such problem.

With my tps utility, I can observe:

10987  sshd: vlefevre@pts/2
└─> 10988  -zsh
  └─> 11262  -zsh
    ├─> 11264  zsh -f -- /home/vlefevre/bin/svnwrapper diff -c65935 /home/vlefevre/wd
    │ └─> 11269  svn diff -c65935 /home/vlefevre/wd
    │   ├─> 11273  zsh /home/vlefevre/scripts/ssh mysvn svnserve -t
    │   │ ├─> 11295  cat
    │   │ └─> 11298  ssh -F /home/vlefevre/.ssh/config -C mysvn svnserve -t
    │   └─> 11299  zsh /home/vlefevre/scripts/ssh mysvn svnserve -t
    │     ├─> 11317  cat
    │     └─> 11320  ssh -F /home/vlefevre/.ssh/config -C mysvn svnserve -t
    └─> 11265  perl /home/vlefevre/bin/tdiff

I think that this occurs on big changesets.

$ svncdiff -c65935 ~/wd | wc
   2166   14746  212680

I'll try to investigate, but any idea about which zsh change could
trigger the problem?

-- System Information:
Debian Release: jessie/sid
  APT prefers unstable
  APT policy: (500, 'unstable'), (500, 'testing'), (500, 'stable'), (1, 'experimental')
Architecture: amd64 (x86_64)
Foreign Architectures: i386

Kernel: Linux 3.11-2-amd64 (SMP w/8 CPU cores)
Locale: LANG=POSIX, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash

Versions of packages zsh depends on:
ii  libc6       2.17-97
ii  libcap2     1:2.22-1.2
ii  libtinfo5   5.9+20130608-1
ii  zsh-common  5.0.3-1

Versions of packages zsh recommends:
ii  libncursesw5  5.9+20130608-1
ii  libpcre3      1:8.31-2

Versions of packages zsh suggests:
ii  zsh-doc  5.0.3-1

-- no debconf information
----- End forwarded message -----

----- Forwarded message from Vincent Lefevre <vincent@vinc17.net> -----
Date: Fri, 20 Dec 2013 19:19:18 +0100
From: Vincent Lefevre <vincent@vinc17.net>
To: 732726@bugs.debian.org
Subject: [Pkg-zsh-devel] Bug#732726: zsh function freeze
Reply-To: Vincent Lefevre <vincent@vinc17.net>, 732726@bugs.debian.org

On 2013-12-20 19:14:36 +0100, Vincent Lefevre wrote:
> I think that this occurs on big changesets.

Indeed I can reproduce it with:

  svncdiff -c 8540 svn://scm.gforge.inria.fr/svn/mpfr/trunk | head
----- End forwarded message -----

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.asciiribbon.org/              | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

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

[Bart Schaefer]

On Dec 20,  8:24pm, Axel Beckert wrote:
}
} Since 5.0.4 seems close, I thought I'd better forward it soon despite
} I haven't looked at it closer yet.

You're just a tiny bit too late -- PWS announced zsh-5.0.4 about 25
minutes before your email was sent. :-(

} this has been reported[1] against zsh 5.0.3 in Debian today. Looks like
} some regression in 5.0.3 on a first glance. Maybe related to the
} test-suite freezes which have been reported on a few architectures.

I think that's unlikely unless svnwrapper is using zpty.  I'm not a user
of SVN at this point so I don't have a reference.

Best at this point to see if you can still reproduce it with the current
zsh source from git.

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

[Axel Beckert]

Hi,

On Fri, Dec 20, 2013 at 12:27:00PM -0800, Bart Schaefer wrote:
> On Dec 20,  8:24pm, Axel Beckert wrote:
> } Since 5.0.4 seems close, I thought I'd better forward it soon despite
> } I haven't looked at it closer yet.
> 
> You're just a tiny bit too late -- PWS announced zsh-5.0.4 about 25
> minutes before your email was sent. :-(

Hrm, I just saw the git tags so far, but neither me nor Frank Terbeck
have received such an announcement via e-mail yet. Well, maybe it's
hanging in Greylisting or such.

Thanks for the information anyway. :-)

		Kind regards, Axel
-- 
/~\  Plain Text Ribbon Campaign                   | Axel Beckert
\ /  Say No to HTML in E-Mail and News            | abe@deuxchevaux.org  (Mail)
 X   See http://www.asciiribbon.org/              | abe@noone.org (Mail+Jabber)
/ \  I love long mails: http://email.is-not-s.ms/ | http://noone.org/abe/ (Web)

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

[Vincent Lefevre]

On 2013-12-20 12:27:00 -0800, Bart Schaefer wrote:
> I think that's unlikely unless svnwrapper is using zpty.  I'm not a user
> of SVN at this point so I don't have a reference.

That's the pipe itself that is completely broken.
The problem can be reproduced with "zsh -f" (version 5.0.3) and:

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

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

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

[Vincent Lefevre]

On 2013-12-21 00:51:49 +0100, Vincent Lefevre wrote:
> The problem can be reproduced with "zsh -f" (version 5.0.3) and:
> 
> ypig% foo() { ls -R / }
> ypig% foo | head

And also with zsh 5.0.4, which has just been released.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

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

[Vincent Lefevre]

A better example:

foo() { printf "%d\n" {1..20000} }

"foo > /dev/null" terminates immediately.
"foo" terminates after a few seconds (output takes a bit time via ssh).
"foo | head" freezes.

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <http://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

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

[Bart Schaefer]

On Dec 21,  1:12am, Vincent Lefevre wrote:
} Subject: Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function
}
} A better example:
} 
} foo() { printf "%d\n" {1..20000} }

Actually the "ls -R /" example was better because it ruled out problems
with SIGPIPE handling and the "print" builtin.

This has to be result of either

+       * 31549: Src/exec,c, Src/zsh.h: replace ad-hoc subsh_close file
+       descriptor for pipes with new addfilelist() job-based mechanism.

(which is the tail of a whole thread starting with 31528) or else it's

+       * 31919: Src/exec.c, Src/init.c: fix deadlock when a shell builtin
+       with a multio redirection is used on the left side of a pipeline,
+       by making sure stdin/out/err file descriptors are closed for the
+       multio copy process, which means not re-using those descriptors
+       after they are closed and marked FDT_UNUSED in fdtable[].  For
+       completeness, initialize their fdtable[] state to FDT_EXTERNAL.

because the problem is that the parent shell still holds open the file
descriptor that is the standard output of the shell function, so the
write is blocked rather than getting SIGPIPE or other failure.

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

[Bart Schaefer]

On Dec 20,  6:19pm, Bart Schaefer wrote:
}
} +       * 31549: Src/exec,c, Src/zsh.h: replace ad-hoc subsh_close file
} +       descriptor for pipes with new addfilelist() job-based mechanism.

Yep, that's the one.

I compiled commit 709dbbbda82efde2020d9d67a19687c101b91570 and was
able to reproduce the problem.  The immediately preceding commit
39ab9952e8255cb99e9c0abcc8bbec43158a55d7 does not show the bug.

Non-interactive shells also do not show the bug, which may be why
"make check" didn't flag anything.

The problem seems to be with this hunk specifically:

@@ -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;
     }
 }
 

The problem is that pipes[0] isn't always added to the list of files
for the job; sometimes it really does need to be closed there.


diff --git a/Src/exec.c b/Src/exec.c
index dccdc2b..4480033 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]);
     }
 }
 

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

[Bart Schaefer]

On Dec 20,  7:42pm, Bart Schaefer wrote:
}
} The problem is that pipes[0] isn't always added to the list of files
} for the job; sometimes it really does need to be closed there.

Here's a regression test.  I assume it's OK to do this coproc trick,
since if it were not tests A01 and A04 would already have failed.

I verified that this test *fails* with 5.0.3 and 5.0.4 (sigh).


diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index c8320a1..61d24fe 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -202,3 +202,15 @@ F:the bug is still there or it reappeared. See workers-29973 for details.
 0:Check $pipestatus with a known difficult case
 >1 0 1 0 0
 F:This similar test was triggering a reproducible failure with pipestatus.
+
+  { unsetopt MONITOR } 2>/dev/null
+  coproc { read -Et 5 || kill -INT $$ }
+  print -u $ZTST_fd 'This test takes 5 seconds to fail...'
+  { printf "%d\n" {1..20000} } | ( read -E )
+  print -p done
+  read -Ep
+0:Bug regression: piping a shell construct to an external process may hang
+>1
+>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

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

[Peter Stephenson]

On Fri, 20 Dec 2013 19:42:23 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> The problem is that pipes[0] isn't always added to the list of files
> for the job; sometimes it really does need to be closed there.

Thanks for the quick work.

Are we happy this fixes the particular problem (the regression test looks
plausible, don't see why we shouldn't use coproc)?  If so I'll make a
new version immediately.  This evening is my last opportunity for a
while so if this is a definite improvement I'd like to get on with it.

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

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

[Bart Schaefer]

On Dec 21,  6:08pm, Peter Stephenson wrote:
}
} Thanks for the quick work.

The timing was convenient and I was afraid it might have been my fault.

} Are we happy this fixes the particular problem (the regression test looks
} plausible, don't see why we shouldn't use coproc)?  If so I'll make a
} new version immediately.

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.

-- 
Barton E. Schaefer

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

[Peter Stephenson]

On Sat, 21 Dec 2013 12:57:18 -0800
Bart Schaefer <schaefer@brasslantern.com> 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@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

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

[Bart Schaefer]

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

Re: (potential regression in 5.0.3)

[Paul Ackersviller]

I've ran across something else that first looked like a regression,
but upon further inspection Test/A05execution.ztst was doing this
for a couple of months.  Strangely, it goes into an infinite loop,
but only when run in the background.  It does this for me on both
Linux and FreeBSD, starting from the commit
c3114a7735c85b79771e08bd156470bde1a36950, but on 5.0.2 too.

Re: (potential regression in 5.0.3)

[Bart Schaefer]

On Dec 23,  2:06am, Paul Ackersviller wrote:
}
} I've ran across something else that first looked like a regression,
} but upon further inspection Test/A05execution.ztst was doing this
} for a couple of months.  Strangely, it goes into an infinite loop,
} but only when run in the background.  It does this for me on both
} Linux and FreeBSD, starting from the commit
} c3114a7735c85b79771e08bd156470bde1a36950, but on 5.0.2 too.

It's a known issue that changing the MONITOR option from off to on
can cause the acquire_pgrp() routine to go into an infinite loop,
depending on the shape of the process tree from the foreground.
The jobs.c diff in c3114a7 partly fixed that, but the new A05execution
test needs setopt MONITOR to create the conditions it is regressing,
so it is still possible for that to trigger a loop.

Here's an attempt to resolve the remaining infinite-loop condition:

diff --git a/Src/jobs.c b/Src/jobs.c
index a321172..8719465 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -2619,6 +2619,7 @@ acquire_pgrp(void)
     sigset_t blockset, oldset;
 
     if ((mypgrp = GETPGRP()) > 0) {
+	long lastpgrp = mypgrp;
 	sigemptyset(&blockset);
 	sigaddset(&blockset, SIGTTIN);
 	sigaddset(&blockset, SIGTTOU);
@@ -2639,6 +2640,9 @@ acquire_pgrp(void)
 	    if (read(0, NULL, 0) != 0) {} /* Might generate SIGT* */
 	    signal_block(blockset);
 	    mypgrp = GETPGRP();
+	    if (mypgrp == lastpgrp && !interact)
+		break; /* Unlikely that pgrp will ever change */
+	    lastpgrp = mypgrp;
 	}
 	if (mypgrp != mypid) {
 	    if (setpgrp(0, 0) == 0) {

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

[Peter Stephenson]

I'm now back and looking through the mail, most of which I've already
seen as I managed to get it onto my phone while the network was
networking (it was not just that it was 2G, I think the backhaul was a
woodpecker with a dodgy beak and a morse key it kept missing).

It looks like it was either a good or a bad time for me to be away,
depending whether you're me or Bart.  As far as I can see most of the
stuff has been sorted out, by Bart with assistance.  If there's nothing
to indicate otherwise over the next couple of days I'll make a 5.0.5
with the fixes in at the weekend.

First...

On Sat, 21 Dec 2013 17:34:59 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> } 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.

Yes, I don't think there should be too much problem with what we *are*
doing, and the regression test seems fine.

I'm a little bit worried about what we're *not* doing.  The closure is
tied to thisjob.  That's probably better than the previous version where
it was simply signalled by a static, neither associated with the current
job nor with the call hierarchy, but I wonder if we should be closing
even more in the subshell (only); should we, in fact, be closing all
f.d.s associated with pipes now that we're no longer in a shell that has
any interest in them?  Or doesn't this actually cause a problem in
practice?  (We clearly shouldn't be removing files, the other purpose of
the filelist, since that's needed at exactly one point after the final
use --- it might be cleaner having a separate list to emphasise the
different behaviour of files and f.d.s but it's probably not worth the
extra pointer now.)

Anyway, that's for future study (FFS, as a colleague slightly
ambiguously puts it).

pws

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

[Bart Schaefer]

On Jan 2,  6:06pm, Peter Stephenson wrote:
} Subject: Re: Fwd (potential regression in 5.0.3): Bug#732726: zsh function
}
} It looks like it was either a good or a bad time for me to be away,
} depending whether you're me or Bart.

I was at home with a few days off, so it really wasn't a problem.

} If there's nothing to indicate otherwise over the next couple of days
} I'll make a 5.0.5 with the fixes in at the weekend.

There was one thing I left for your attention: Carl's patch in 32196 for
copy-prev-shell-word.


} > } 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).
[...]
} > 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
} 
} Yes, I don't think there should be too much problem with what we *are*
} doing, and the regression test seems fine.

Is it only pipes in that filelist?  I was concerned that there might be
descriptors for real files there, that shouldn't be closed.  But again I
think it's OK because in that context all the shell is going to do is
wait.
 
} I wonder if we should be closing even more in the subshell (only);
} should we, in fact, be closing all f.d.s associated with pipes now
} that we're no longer in a shell that has any interest in them?

Probably yes, which might in fact fix some of the nits with coprocs
(though zsh's coproc doesn't seem to have any nits of that particular
flavor that e.g. ksh's does not also have).  To do that I think we'd
need to start keeping track of what kind of fd is in the filelist,
and whether it's read or write, not just that it's a descriptor
associated with a particular job.

} Or doesn't this actually cause a problem in practice?

As you pointed out, we're doing better now than we were, and even the
old way it hasn't been especially noticable for many years so far.