bug with named pipes and process substitution

bug with named pipes and process substitution

[Petros Aggelatos]

Hi all,

I encountered a bug when running the following two commands:

mkfifo test_pipe
echo 1 | tee >(cat > test_pipe) | paste - test_pipe

In bash and zsh 5.0.2 it outputs "1 1" and then exits. In zsh 5.0.8
however, it outputs the same thing and hangs forever.

I bisected the repo for the commit that introduced this behaviour and
it was introduced in 3c5732223f65309c6820f15b8519f674bd21185b which
includes the patch from this thread
http://www.zsh.org/mla/workers/2013/msg00569.html

I haven't yet figured out how and what, since I'm not very familiar
with the codebase. I'll try to figure out a patch but if someone can
shed some light on what is going on that would be great.

System info:

$ echo $ZSH_VERSION $ZSH_PATCHLEVEL `uname -mo`
5.0.8 zsh-5.0.8-0-gf0068ed x86_64 GNU/Linux

Best,
Petros

Re: bug with named pipes and process substitution

[Peter Stephenson]

On Tue, 21 Jul 2015 17:25:15 -0700
Petros Aggelatos <petrosagg@gmail.com> wrote:
> I encountered a bug when running the following two commands:
> 
> mkfifo test_pipe
> echo 1 | tee >(cat > test_pipe) | paste - test_pipe
> 
> In bash and zsh 5.0.2 it outputs "1 1" and then exits. In zsh 5.0.8
> however, it outputs the same thing and hangs forever.

Thanks.  I can tell you roughly what's going on, but not the
answer.  We used to be fairly cavalier with handling file descriptors
for process substitutions, not closing them in the right place.  In the
patch you mention...

> I bisected the repo for the commit that introduced this behaviour and
> it was introduced in 3c5732223f65309c6820f15b8519f674bd21185b which
> includes the patch from this thread
> http://www.zsh.org/mla/workers/2013/msg00569.html

...we instead put them on a linked list to be removed when the job is
completed, which is better targeted.  However, it leaves the potential
for holes.  Commit aede5c52bf557f89d1d09fa5430186af6e295241 is also
relevant as it plugged some of those holes.  It appears there's at least
one more.

The specific difficulty here is that the "tee" is in a forked subshell
which is exec'd.  Because the process substitution is an argument to the
tee, it needs to be able to see the fd in the file system (e.g.
/proc/self/fd/15 when I tried it).  In the forked shell we can't close
the fd because we've lost control by the time it's needed.  However, we
need to close it in the parent shell before we fork anything else, in
time to ensure other processes don't hang because of it.

The call we need is probably pipecleanfilelist(...) from the second
commit, if we can find where to put it...

pws

Re: bug with named pipes and process substitution

[Peter Stephenson]

On Wed, 22 Jul 2015 10:09:44 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> The call we need is probably pipecleanfilelist(...) from the second
> commit, if we can find where to put it...

Best I could come up with was special handling of process substtitutions
after a fork... that looks like it does the business without side
effects:  the question is more whether this is general enough.

The rationale for this is that the process substitutions are always
handled locally in execcmd(), so we can be sure removing them in the
parent after the fork isn't doing any damage.  Other entries in filelist
don't have that guarantee.

Slightly tweaked test to remove dependencies but as it's actually more
complicated than the original, processwise, I think it should be OK.

One day this may even get committed...

pws

diff --git a/Src/exec.c b/Src/exec.c
index 4eee82b..7612d43 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -3047,6 +3047,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    addproc(pid, text, 0, &bgtime);
 	    if (oautocont >= 0)
 		opts[AUTOCONTINUE] = oautocont;
+	    pipecleanfilelist(jobtab[thisjob].filelist, 1);
 	    return;
 	}
 	/* pid == 0 */
@@ -3492,7 +3493,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 
 	    if (is_shfunc) {
 		/* It's a shell function */
-		pipecleanfilelist(filelist);
+		pipecleanfilelist(filelist, 0);
 		execshfunc((Shfunc) hn, args);
 	    } else {
 		/* It's a builtin */
@@ -3682,7 +3683,7 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		DPUTS(varspc,
 		      "BUG: assignment before complex command");
 		list_pipe = 0;
-		pipecleanfilelist(filelist);
+		pipecleanfilelist(filelist, 0);
 		/* 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 948f61b..a71df68 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1179,7 +1179,7 @@ addfilelist(const char *name, int fd)
 
 /**/
 void
-pipecleanfilelist(LinkList filelist)
+pipecleanfilelist(LinkList filelist, int proc_subst_only)
 {
     LinkNode node;
 
@@ -1188,7 +1188,9 @@ pipecleanfilelist(LinkList filelist)
     node = firstnode(filelist);
     while (node) {
 	Jobfile jf = (Jobfile)getdata(node);
-	if (jf->is_fd) {
+	if (jf->is_fd &&
+	    (!proc_subst_only ||
+	     fdtable[jf->u.fd] == FDT_PROC_SUBST)) {
 	    LinkNode next = nextnode(node);
 	    zclose(jf->u.fd);
 	    (void)remnode(filelist, node);
@@ -1433,7 +1435,7 @@ zwaitjob(int job, int wait_cmd)
 	     * we can't deadlock on the fact that those still exist, so
 	     * that's not a problem.
 	     */
-	    pipecleanfilelist(jn->filelist);
+	    pipecleanfilelist(jn->filelist, 0);
 	}
 	while (!errflag && jn->stat &&
 	       !(jn->stat & STAT_DONE) &&
@@ -1623,7 +1625,7 @@ spawnjob(void)
 	deletejob(jobtab + thisjob, 0);
     else {
 	jobtab[thisjob].stat |= STAT_LOCKED;
-	pipecleanfilelist(jobtab[thisjob].filelist);
+	pipecleanfilelist(jobtab[thisjob].filelist, 0);
     }
     thisjob = -1;
 }
diff --git a/Src/zsh.h b/Src/zsh.h
index 69fef33..a99c900 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1752,9 +1752,10 @@ struct tieddata {
 				  * necessarily want to match multiple
 				  * elements
 				  */
-#define SCANPM_ISVAR_AT   ((-1)<<15)	/* "$foo[@]"-style substitution
-					 * Only sign bit is significant
-					 */
+/* "$foo[@]"-style substitution
+ * Only sign bit is significant
+ */
+#define SCANPM_ISVAR_AT   ((int)(((unsigned int)-1)<<15))
 
 /*
  * Flags for doing matches inside parameter substitutions, i.e.
diff --git a/Test/D03procsubst.ztst b/Test/D03procsubst.ztst
index 7b87589..1fc09b1 100644
--- a/Test/D03procsubst.ztst
+++ b/Test/D03procsubst.ztst
@@ -126,3 +126,18 @@
   eval 'foo here is some output)'
 0:full alias expanded when substitution starts in alias
 >here is some output
+
+  if ! (mkpipe test_pipe >/dev/null 2>&1); then
+    ZTST_skip="mkpipe not available"
+  else
+    echo 1 | tee >(cat > test_pipe) | (){
+      local pipein
+      read pipein <test_pipe
+      print $pipein
+      read pipein
+      print $pipein
+    }
+  fi
+0:proc subst fd in forked subshell closed in parent
+>1
+>1

Re: bug with named pipes and process substitution

[Peter Stephenson]

On Wed, 22 Jul 2015 10:58:29 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> --- a/Src/zsh.h
> +++ b/Src/zsh.h

Sorry, this is an irrelevant uncommitted change.

> +  if ! (mkpipe test_pipe >/dev/null 2>&1); then
> +    ZTST_skip="mkpipe not available"

mkpipe -> mkfifo.

pws

Re: bug with named pipes and process substitution

[Bart Schaefer]

On Jul 22, 11:27am, Peter Stephenson wrote:
} Subject: Re: bug with named pipes and process substitution
}
} Sorry, this is an irrelevant uncommitted change.

The following bit is from a different patch also, I believe.  I've been
"git stash"-ing all my changes to re-apply after sourceforge is back.

} diff --git a/Src/zsh.h b/Src/zsh.h
} -#define SCANPM_ISVAR_AT   ((-1)<<15)	/* "$foo[@]"-style substitution
} -					 * Only sign bit is significant
} -					 */
} +/* "$foo[@]"-style substitution
} + * Only sign bit is significant
} + */
} +#define SCANPM_ISVAR_AT   ((int)(((unsigned int)-1)<<15))

-- 
Barton E. Schaefer

Re: bug with named pipes and process substitution

[Mikael Magnusson]

On Wed, Jul 22, 2015 at 4:45 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jul 22, 11:27am, Peter Stephenson wrote:
> } Subject: Re: bug with named pipes and process substitution
> }
> } Sorry, this is an irrelevant uncommitted change.
>
> The following bit is from a different patch also, I believe.  I've been
> "git stash"-ing all my changes to re-apply after sourceforge is back.

Why would you do that rather than just committing them?

-- 
Mikael Magnusson

Re: bug with named pipes and process substitution

[Bart Schaefer]

On Jul 23,  3:03am, Mikael Magnusson wrote:
}
} > "git stash"-ing all my changes to re-apply after sourceforge is back.
} 
} Why would you do that rather than just committing them?

Because (a) I spent the last six days thinking "gee, they've got to be
back on line any minute now" and (b) I prefer to "git pull" and then
edit the ChangeLog to reduce the chances of a conflict on ChangeLog,
because (c) even with --rebase I find the way git handles overlapping
diffs in a file that grows at the top to be really annoying and (d) I
want the ChangeLog entry to be the same commit as the rest of the diff.

ChangeLog (was Re: bug with named pipes and process substitution)

[Kamil Dudka]

On Wednesday 22 July 2015 18:46:18 Bart Schaefer wrote:
> On Jul 23,  3:03am, Mikael Magnusson wrote:
> }
> } > "git stash"-ing all my changes to re-apply after sourceforge is back.
> }
> } Why would you do that rather than just committing them?
> 
> Because (a) I spent the last six days thinking "gee, they've got to be
> back on line any minute now" and (b) I prefer to "git pull" and then
> edit the ChangeLog to reduce the chances of a conflict on ChangeLog,
> because (c) even with --rebase I find the way git handles overlapping
> diffs in a file that grows at the top to be really annoying and (d) I
> want the ChangeLog entry to be the same commit as the rest of the diff.

Do the ChangeLog entries actually capture anything that commit messages
can not?  Would not it be better to just generate ChangeLog from git log?

Unlike ChangeLog as a text file, a conflict on commit messages can never 
happen.

Kamil

Re: ChangeLog (was Re: bug with named pipes and process substitution)

[Daniel Shahaf]

Kamil Dudka wrote on Thu, Jul 23, 2015 at 10:15:40 +0200:
> On Wednesday 22 July 2015 18:46:18 Bart Schaefer wrote:
> > On Jul 23,  3:03am, Mikael Magnusson wrote:
> > }
> > } > "git stash"-ing all my changes to re-apply after sourceforge is back.
> > }
> > } Why would you do that rather than just committing them?
> > 
> > Because (a) I spent the last six days thinking "gee, they've got to be
> > back on line any minute now" and (b) I prefer to "git pull" and then
> > edit the ChangeLog to reduce the chances of a conflict on ChangeLog,
> > because (c) even with --rebase I find the way git handles overlapping
> > diffs in a file that grows at the top to be really annoying and (d) I
> > want the ChangeLog entry to be the same commit as the rest of the diff.
> 
> Do the ChangeLog entries actually capture anything that commit messages
> can not?  Would not it be better to just generate ChangeLog from git log?
> 
> Unlike ChangeLog as a text file, a conflict on commit messages can never 
> happen.
> 

That's what I do: locally I have just the log messages, and I generate
the ChangeLog from them prior to pushing.  (I just reuse the first
sentence verbatim, but any transformation could be substituted.)

An alternative is to use a custom merge tool that automatically resolves
conflicts on ChangeLog, and defers to the default merge tool for
conflicts on other files.  I think http://svn.apache.org/r1491816 is
exactly the sort of functionality that would be useful for the ChangeLog
file: it just concatenates the "<<<<<<" and ">>>>>>" sides of the
conflict.

(I don't know if git has equivalent functionality built-in, however,
I do know that mergetools are configurable so I think custom mergetool
could implement equivalent functionality.)

> Kamil

Re: bug with named pipes and process substitution

[Petros Aggelatos]

Hi,

Thanks a lot for the quick resolution, I tested the patch and it works
for me too. However, the test added works fine even on the master
branch without the patch (I included the mkpipe -> mkfifo change). Not
sure if I'm doing something wrong but I'm sending the test I used to
find the broken commit in case there is a problem with the current
test:

diff --git a/Test/D03procsubst.ztst b/Test/D03procsubst.ztst
index 7b87589..757ac49 100644
--- a/Test/D03procsubst.ztst
+++ b/Test/D03procsubst.ztst
@@ -126,3 +126,15 @@
   eval 'foo here is some output)'
 0:full alias expanded when substitution starts in alias
 >here is some output
+
+  if ! (mkfifo test_pipe >/dev/null 2>&1); then
+    ZTST_skip="mkfifo not available"
+  else
+    echo 1 | tee >(cat > test_pipe) | cat - test_pipe > /dev/null &
+    sleep 1
+    if [ ! -d /proc/$! ]; then
+      echo success
+    fi
+  fi
+0:proc subst fd in forked subshell closed in parent
+>success

Re: bug with named pipes and process substitution

[Peter Stephenson]

On Wed, 22 Jul 2015 17:32:05 -0700
Petros Aggelatos <petrosagg@gmail.com> wrote:
> Thanks a lot for the quick resolution, I tested the patch and it works
> for me too. However, the test added works fine even on the master
> branch without the patch (I included the mkpipe -> mkfifo change). Not
> sure if I'm doing something wrong but I'm sending the test I used to
> find the broken commit in case there is a problem with the current
> test.

Having the external command in the last section of the pipe can change
the behaviour, so it's probably that, which I didn't notice having fixed
the original case.

We do, in fact, use paste in procsubst tests already, so I'll just add
your original form as an additional test.

Thanks
pws

Re: bug with named pipes and process substitution

[Jun T.]

On 2015/07/22, at 18:58, Peter Stephenson <p.stephenson@samsung.com> wrote:
> 
> diff --git a/Src/jobs.c b/Src/jobs.c
> index 948f61b..a71df68 100644
(snip)
> @@ -1188,7 +1188,9 @@ pipecleanfilelist(LinkList filelist)
>     node = firstnode(filelist);
>     while (node) {
> 	Jobfile jf = (Jobfile)getdata(node);
> -	if (jf->is_fd) {
> +	if (jf->is_fd &&
> +	    (!proc_subst_only ||
> +	     fdtable[jf->u.fd] == FDT_PROC_SUBST)) {
> 	    LinkNode next = nextnode(node);

With this patch, build fails on cygwin where PATH_DEV_FD and
FDT_PROC_SUBST is not defined:

jobs.c:1193:28: error: 'FDT_PROC_SUBST' undeclared (first use in this function)
       fdtable[jf->u.fd] == FDT_PROC_SUBST)) {
                            ^

The following patch *seems* to work (at least D03procsubst.ztst succeeds),
but I haven't understood (will not understand) the full detail.


diff --git a/Src/jobs.c b/Src/jobs.c
index a71df68..9333488 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1189,8 +1189,11 @@ pipecleanfilelist(LinkList filelist, int proc_subst_only)
     while (node) {
 	Jobfile jf = (Jobfile)getdata(node);
 	if (jf->is_fd &&
-	    (!proc_subst_only ||
-	     fdtable[jf->u.fd] == FDT_PROC_SUBST)) {
+	    (!proc_subst_only
+#ifdef FDT_PROC_SUBST
+	     || fdtable[jf->u.fd] == FDT_PROC_SUBST
+#endif
+		)) {
 	    LinkNode next = nextnode(node);
 	    zclose(jf->u.fd);
 	    (void)remnode(filelist, node);

Re: bug with named pipes and process substitution

[m0viefreak]

On 22.07.2015 11:58, Peter Stephenson wrote:
> On Wed, 22 Jul 2015 10:09:44 +0100
> @@ -1188,7 +1188,9 @@ pipecleanfilelist(LinkList filelist)
>      node = firstnode(filelist);
>      while (node) {
>  	Jobfile jf = (Jobfile)getdata(node);
> -	if (jf->is_fd) {
> +	if (jf->is_fd &&
> +	    (!proc_subst_only ||
> +	     fdtable[jf->u.fd] == FDT_PROC_SUBST)) {
>  	    LinkNode next = nextnode(node);
>  	    zclose(jf->u.fd);
>  	    (void)remnode(filelist, node);

This change introduces a compile error when PATH_DEV_FD is not defined
(cygwin), because FDT_PROC_SUBST is only defined in case PATH_DEV_FD is,
too.

Re: bug with named pipes and process substitution

[Jun T.]

2015/08/08 20:59, m0viefreak <m0viefreak.cm@googlemail.com> wrote:

> This change introduces a compile error when PATH_DEV_FD is not defined
> (cygwin), because FDT_PROC_SUBST is only defined in case PATH_DEV_FD is,
> too.

Yes, I've just pushed a simple patch (cf X-seq:35929).