* bug with named pipes and process substitution @ 2015-07-22 0:25 Petros Aggelatos 2015-07-22 9:09 ` Peter Stephenson 0 siblings, 1 reply; 14+ messages in thread From: Petros Aggelatos @ 2015-07-22 0:25 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 0:25 bug with named pipes and process substitution Petros Aggelatos @ 2015-07-22 9:09 ` Peter Stephenson 2015-07-22 9:58 ` Peter Stephenson 0 siblings, 1 reply; 14+ messages in thread From: Peter Stephenson @ 2015-07-22 9:09 UTC (permalink / raw) To: Petros Aggelatos, zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 9:09 ` Peter Stephenson @ 2015-07-22 9:58 ` Peter Stephenson 2015-07-22 10:27 ` Peter Stephenson ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Peter Stephenson @ 2015-07-22 9:58 UTC (permalink / raw) To: Petros Aggelatos, zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 9:58 ` Peter Stephenson @ 2015-07-22 10:27 ` Peter Stephenson 2015-07-22 14:45 ` Bart Schaefer 2015-07-23 0:32 ` bug with named pipes and process substitution Petros Aggelatos ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Peter Stephenson @ 2015-07-22 10:27 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 10:27 ` Peter Stephenson @ 2015-07-22 14:45 ` Bart Schaefer 2015-07-23 1:03 ` Mikael Magnusson 0 siblings, 1 reply; 14+ messages in thread From: Bart Schaefer @ 2015-07-22 14:45 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 14:45 ` Bart Schaefer @ 2015-07-23 1:03 ` Mikael Magnusson 2015-07-23 1:46 ` Bart Schaefer 0 siblings, 1 reply; 14+ messages in thread From: Mikael Magnusson @ 2015-07-23 1:03 UTC (permalink / raw) To: Bart Schaefer; +Cc: zsh workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-23 1:03 ` Mikael Magnusson @ 2015-07-23 1:46 ` Bart Schaefer 2015-07-23 8:15 ` ChangeLog (was Re: bug with named pipes and process substitution) Kamil Dudka 0 siblings, 1 reply; 14+ messages in thread From: Bart Schaefer @ 2015-07-23 1:46 UTC (permalink / raw) To: zsh workers 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* ChangeLog (was Re: bug with named pipes and process substitution) 2015-07-23 1:46 ` Bart Schaefer @ 2015-07-23 8:15 ` Kamil Dudka 2015-07-23 22:01 ` Daniel Shahaf 0 siblings, 1 reply; 14+ messages in thread From: Kamil Dudka @ 2015-07-23 8:15 UTC (permalink / raw) To: Bart Schaefer; +Cc: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: ChangeLog (was Re: bug with named pipes and process substitution) 2015-07-23 8:15 ` ChangeLog (was Re: bug with named pipes and process substitution) Kamil Dudka @ 2015-07-23 22:01 ` Daniel Shahaf 0 siblings, 0 replies; 14+ messages in thread From: Daniel Shahaf @ 2015-07-23 22:01 UTC (permalink / raw) To: Kamil Dudka; +Cc: Bart Schaefer, zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 9:58 ` Peter Stephenson 2015-07-22 10:27 ` Peter Stephenson @ 2015-07-23 0:32 ` Petros Aggelatos 2015-07-23 9:01 ` Peter Stephenson 2015-07-27 12:25 ` Jun T. 2015-08-08 11:59 ` m0viefreak 3 siblings, 1 reply; 14+ messages in thread From: Petros Aggelatos @ 2015-07-23 0:32 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-23 0:32 ` bug with named pipes and process substitution Petros Aggelatos @ 2015-07-23 9:01 ` Peter Stephenson 0 siblings, 0 replies; 14+ messages in thread From: Peter Stephenson @ 2015-07-23 9:01 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 9:58 ` Peter Stephenson 2015-07-22 10:27 ` Peter Stephenson 2015-07-23 0:32 ` bug with named pipes and process substitution Petros Aggelatos @ 2015-07-27 12:25 ` Jun T. 2015-08-08 11:59 ` m0viefreak 3 siblings, 0 replies; 14+ messages in thread From: Jun T. @ 2015-07-27 12:25 UTC (permalink / raw) To: zsh-workers 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); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-07-22 9:58 ` Peter Stephenson ` (2 preceding siblings ...) 2015-07-27 12:25 ` Jun T. @ 2015-08-08 11:59 ` m0viefreak 2015-08-08 15:21 ` Jun T. 3 siblings, 1 reply; 14+ messages in thread From: m0viefreak @ 2015-08-08 11:59 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: bug with named pipes and process substitution 2015-08-08 11:59 ` m0viefreak @ 2015-08-08 15:21 ` Jun T. 0 siblings, 0 replies; 14+ messages in thread From: Jun T. @ 2015-08-08 15:21 UTC (permalink / raw) To: zsh-workers 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). ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-08-08 16:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-22 0:25 bug with named pipes and process substitution Petros Aggelatos 2015-07-22 9:09 ` Peter Stephenson 2015-07-22 9:58 ` Peter Stephenson 2015-07-22 10:27 ` Peter Stephenson 2015-07-22 14:45 ` Bart Schaefer 2015-07-23 1:03 ` Mikael Magnusson 2015-07-23 1:46 ` Bart Schaefer 2015-07-23 8:15 ` ChangeLog (was Re: bug with named pipes and process substitution) Kamil Dudka 2015-07-23 22:01 ` Daniel Shahaf 2015-07-23 0:32 ` bug with named pipes and process substitution Petros Aggelatos 2015-07-23 9:01 ` Peter Stephenson 2015-07-27 12:25 ` Jun T. 2015-08-08 11:59 ` m0viefreak 2015-08-08 15:21 ` Jun T.
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).