From ab74c86edb30fa04fda5fe7fa01e404334f7c2b0 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Wed, 17 Feb 2016 10:40:55 +0000 Subject: 37999: Sticky behaviour of EXIT traps. They now have POSIX or non-POSIX behaviour based on the setting of POSIX_TRAPS where the trap was defined, rather than where the trap would (or would not) be executed. Tweaks possible. --- Src/signals.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-) (limited to 'Src/signals.c') diff --git a/Src/signals.c b/Src/signals.c index aa0b5aaa7..32452ae4f 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -55,6 +55,15 @@ mod_export Eprog siglists[VSIGCOUNT]; /**/ mod_export int nsigtrapped; +/* + * Flag that exit trap has been set in POSIX mode. + * The setter's expectation is therefore that it is run + * on programme exit, not function exit. + */ + +/**/ +static int exit_trap_posix; + /* Variables used by signal queueing */ /**/ @@ -755,7 +764,7 @@ killjb(Job jn, int sig) * at once, so just use a linked list. */ struct savetrap { - int sig, flags, local; + int sig, flags, local, posix; void *list; }; @@ -774,6 +783,7 @@ dosavetrap(int sig, int level) st = (struct savetrap *)zalloc(sizeof(*st)); st->sig = sig; st->local = level; + st->posix = (sig == SIGEXIT) ? exit_trap_posix : 0; if ((st->flags = sigtrapped[sig]) & ZSIG_FUNC) { /* * Get the old function: this assumes we haven't added @@ -873,6 +883,10 @@ settrap(int sig, Eprog l, int flags) * works just the same. */ sigtrapped[sig] |= (locallevel << ZSIG_SHIFT) | flags; + if (sig == SIGEXIT) { + /* Make POSIX behaviour of EXIT trap sticky */ + exit_trap_posix = isset(POSIXTRAPS); + } unqueue_signals(); return 0; } @@ -908,7 +922,7 @@ removetrap(int sig) * already one at the current locallevel we just overwrite it. */ if (!dontsavetrap && - (sig == SIGEXIT ? !isset(POSIXTRAPS) : isset(LOCALTRAPS)) && + (sig == SIGEXIT ? !exit_trap_posix : isset(LOCALTRAPS)) && locallevel && (!trapped || locallevel > (sigtrapped[sig] >> ZSIG_SHIFT))) dosavetrap(sig, locallevel); @@ -935,6 +949,8 @@ removetrap(int sig) #endif sig != SIGCHLD) signal_default(sig); + if (sig == SIGEXIT) + exit_trap_posix = 0; /* * At this point we free the appropriate structs. If we don't @@ -978,7 +994,7 @@ starttrapscope(void) * so give it the next higher one. dosavetrap() is called * automatically where necessary. */ - if (sigtrapped[SIGEXIT] && !isset(POSIXTRAPS)) { + if (sigtrapped[SIGEXIT] && !exit_trap_posix) { locallevel++; unsettrap(SIGEXIT); locallevel--; @@ -1005,7 +1021,7 @@ endtrapscope(void) * Don't do this inside another trap. */ if (!intrap && - !isset(POSIXTRAPS) && (exittr = sigtrapped[SIGEXIT])) { + !exit_trap_posix && (exittr = sigtrapped[SIGEXIT])) { if (exittr & ZSIG_FUNC) { exitfn = removehashnode(shfunctab, "TRAPEXIT"); } else { @@ -1031,7 +1047,9 @@ endtrapscope(void) if (st->flags & ZSIG_FUNC) settrap(sig, NULL, ZSIG_FUNC); else - settrap(sig, (Eprog) st->list, 0); + settrap(sig, (Eprog) st->list, 0); + if (sig == SIGEXIT) + exit_trap_posix = st->posix; dontsavetrap--; /* * counting of nsigtrapped should presumably be handled @@ -1042,16 +1060,26 @@ endtrapscope(void) if ((sigtrapped[sig] = st->flags) & ZSIG_FUNC) shfunctab->addnode(shfunctab, ((Shfunc)st->list)->node.nam, (Shfunc) st->list); - } else if (sigtrapped[sig]) - unsettrap(sig); + } else if (sigtrapped[sig]) { + /* + * Don't restore the old state if someone has set a + * POSIX-style exit trap --- allow this to propagate. + */ + if (sig != SIGEXIT || !exit_trap_posix) + unsettrap(sig); + } zfree(st, sizeof(*st)); } } if (exittr) { - if (!isset(POSIXTRAPS)) - dotrapargs(SIGEXIT, &exittr, exitfn); + /* + * We already made sure this wasn't set as a POSIX exit trap. + * We respect the user's intention when the trap in question + * was set. + */ + dotrapargs(SIGEXIT, &exittr, exitfn); if (exittr & ZSIG_FUNC) shfunctab->freenode((HashNode)exitfn); else -- cgit v1.2.3 From c55d8551718bcf2fc7661c31c13e934060a5f1a7 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Thu, 25 Feb 2016 14:20:26 +0000 Subject: 38024: Improve POSIX and native EXIT traps compatibility. Allow a nested function trap to leave save and restore a POSIX trap. Still fails if the POSIX trap was defined in a function. --- ChangeLog | 6 ++++++ Src/signals.c | 7 ++++++- Test/C03traps.ztst | 17 +++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) (limited to 'Src/signals.c') diff --git a/ChangeLog b/ChangeLog index 2bc3631c8..5e3162f21 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2016-02-25 Peter Stephenson + + * 38024: Src/signals.c, Test/C03traps.ztst: improve 37999 to + allow nested zsh-mode EXIT traps not to remove a POSIX EXIT + trap. + 2016-02-19 Daniel Shahaf * unposted: Etc/completion-style-guide: Clarify the term "variant". diff --git a/Src/signals.c b/Src/signals.c index 32452ae4f..1344395f7 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -920,9 +920,14 @@ removetrap(int sig) * Note that we save the trap here even if there isn't an existing * one, to aid in removing this one. However, if there's * already one at the current locallevel we just overwrite it. + * + * Note we save EXIT traps based on the *current* setting of + * POSIXTRAPS --- so if there is POSIX EXIT trap set but + * we are in native mode it can be saved, replaced by a function + * trap, and then restored. */ if (!dontsavetrap && - (sig == SIGEXIT ? !exit_trap_posix : isset(LOCALTRAPS)) && + (sig == SIGEXIT ? !isset(POSIXTRAPS) : isset(LOCALTRAPS)) && locallevel && (!trapped || locallevel > (sigtrapped[sig] >> ZSIG_SHIFT))) dosavetrap(sig, locallevel); diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index d8183a428..f4466b556 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -419,6 +419,23 @@ >end of program >EXIT TRAP TRIGGERED + (cd ..; $ZTST_exe -fc ' + echo entering program + emulate sh -c '\''trap "echo POSIX exit trap triggered" EXIT'\'' + fn() { + trap "echo native zsh function-local exit trap triggered" EXIT + echo entering native zsh function + } + fn + echo exiting program + ') +0:POSX EXIT trap can have nested native mode EXIT trap +>entering program +>entering native zsh function +>native zsh function-local exit trap triggered +>exiting program +>POSIX exit trap triggered + (set -e printf "a\nb\n" | while read line do -- cgit v1.2.3 From 17fb014dc7984902a6697c6412b0cca55300542b Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Mon, 7 Mar 2016 09:42:21 +0000 Subject: 38094: Fix POSIX EXIT traps defined in function. These aren't local, so set the local level to 0; else they can get overridden incorrectly. --- ChangeLog | 6 ++++++ Src/signals.c | 7 ++++++- Test/C03traps.ztst | 22 ++++++++++++++++++++-- 3 files changed, 32 insertions(+), 3 deletions(-) (limited to 'Src/signals.c') diff --git a/ChangeLog b/ChangeLog index b9d34e430..52299a364 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2016-03-07 Peter Stephenson + + * 38094: Src/signals.c, Test/C03traps.ztst: POSIX exit traps + aren't local so local level should be 0 so they don't + get trashed if defined in a function. + 2016-03-06 Barton E. Schaefer * 38106: Src/parse.c: if...then if...else should be a parse error. diff --git a/Src/signals.c b/Src/signals.c index 1344395f7..2eefc07de 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -877,16 +877,21 @@ settrap(int sig, Eprog l, int flags) sig != SIGCHLD) install_handler(sig); } + sigtrapped[sig] |= flags; /* * Note that introducing the locallevel does not affect whether * sigtrapped[sig] is zero or not, i.e. a test without a mask * works just the same. */ - sigtrapped[sig] |= (locallevel << ZSIG_SHIFT) | flags; if (sig == SIGEXIT) { /* Make POSIX behaviour of EXIT trap sticky */ exit_trap_posix = isset(POSIXTRAPS); + /* POSIX exit traps are not local. */ + if (!exit_trap_posix) + sigtrapped[sig] |= (locallevel << ZSIG_SHIFT); } + else + sigtrapped[sig] |= (locallevel << ZSIG_SHIFT); unqueue_signals(); return 0; } diff --git a/Test/C03traps.ztst b/Test/C03traps.ztst index f4466b556..83c05aa08 100644 --- a/Test/C03traps.ztst +++ b/Test/C03traps.ztst @@ -429,14 +429,32 @@ fn echo exiting program ') -0:POSX EXIT trap can have nested native mode EXIT trap +0:POSIX EXIT trap can have nested native mode EXIT trap >entering program >entering native zsh function >native zsh function-local exit trap triggered >exiting program >POSIX exit trap triggered - (set -e + (cd ..; $ZTST_exe -fc ' + echo entering program + emulate sh -c '\''spt() { trap "echo POSIX exit trap triggered" EXIT; }'\'' + fn() { + trap "echo native zsh function-local exit trap triggered" EXIT + echo entering native zsh function + } + spt + fn + echo exiting program + ') +0:POSIX EXIT trap not replaced if defined within function +>entering program +>entering native zsh function +>native zsh function-local exit trap triggered +>exiting program +>POSIX exit trap triggered + + (set -e printf "a\nb\n" | while read line do [[ $line = a* ]] || continue -- cgit v1.2.3 From 327f3dd3adfc8fdd6c356722d093a340b81190a7 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Fri, 16 Sep 2016 09:34:17 +0100 Subject: 39359: Fix remaining race with orphaned subjob. When shell is forked to run right hand side of pipieline it should use its own PID as process group if the left hand side of the pipeline has already exited. --- ChangeLog | 4 ++++ Src/exec.c | 44 +++++++++++++++++++++++++++++++++++++++++++- Src/jobs.c | 3 ++- Src/signals.c | 16 +++++++++++++--- 4 files changed, 62 insertions(+), 5 deletions(-) (limited to 'Src/signals.c') diff --git a/ChangeLog b/ChangeLog index 29796ed01..add2ad6a5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2016-09-16 Peter Stephenson + * 39359: Src/exec.c, Src/jobs.c, Src/signals.c: Further fix on + top of 39331 for remaining race. Ensure process group of forked + superjob is sane. + * 39331: Src/exec.c, Src/jobs.c, Src/zsh.h: Partially fix problem occurring when a subjop in the RHS of a pipeline needs to be picked up by a forked zsh after ^Z when the original superjob diff --git a/Src/exec.c b/Src/exec.c index 21270c82d..0755d55cd 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -1652,7 +1652,13 @@ execpline(Estate state, wordcode slcode, int how, int last1) int synch[2]; struct timeval bgtime; + /* + * A pipeline with the shell handling the right + * hand side was stopped. We'll fork to allow + * it to continue. + */ if (pipe(synch) < 0 || (pid = zfork(&bgtime)) == -1) { + /* Failure */ if (pid < 0) { close(synch[0]); close(synch[1]); @@ -1666,6 +1672,18 @@ execpline(Estate state, wordcode slcode, int how, int last1) thisjob = newjob; } else if (pid) { + /* + * Parent: job control is here. If the job + * started for the RHS of the pipeline is still + * around, then its a SUBJOB and the job for + * earlier parts of the pipeeline is its SUPERJOB. + * The newly forked shell isn't recorded as a + * separate job here, just as list_pipe_pid. + * If the superjob exits (it may already have + * done so, see child branch below), we'll use + * list_pipe_pid to form the basis of a + * replacement job --- see SUBLEADER code above. + */ char dummy; lpforked = @@ -1692,9 +1710,33 @@ execpline(Estate state, wordcode slcode, int how, int last1) break; } else { + Job pjn = jobtab + list_pipe_job; close(synch[0]); entersubsh(ESUB_ASYNC); - if (jobtab[list_pipe_job].procs) { + /* + * At this point, we used to attach this process + * to the process group of list_pipe_job (the + * new superjob) any time that was still available. + * That caused problems when the fork happened + * early enough that the subjob is in that + * process group, since then we will stop this + * job when we signal the subjob, and we have + * no way here to know that we shouldn't also + * send STOP to itself, resulting in two stops. + * So don't do that if the original + * list_pipe_job has exited. + * + * The choice here needs to match the assumption + * made when handling SUBLEADER above that the + * process group is our own PID. I'm not sure + * if there's a potential race for that. But + * setting up a new process group if the + * superjob is still functioning seems the wrong + * thing to do. + */ + if (pjn->procs && + (pjn->stat & STAT_INUSE) && + !(pjn->stat & STAT_DONE)) { if (setpgrp(0L, mypgrp = jobtab[list_pipe_job].gleader) == -1) { setpgrp(0L, mypgrp = getpid()); diff --git a/Src/jobs.c b/Src/jobs.c index 9284c7124..d1b98ac4d 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -232,11 +232,12 @@ super_job(int sub) static int handle_sub(int job, int fg) { + /* job: superjob; sj: subjob. */ Job jn = jobtab + job, sj = jobtab + jn->other; if ((sj->stat & STAT_DONE) || (!sj->procs && !sj->auxprocs)) { struct process *p; - + for (p = sj->procs; p; p = p->next) { if (WIFSIGNALED(p->status)) { if (jn->gleader != mypgrp && jn->procs->next) diff --git a/Src/signals.c b/Src/signals.c index 2eefc07de..30dde713f 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -723,7 +723,7 @@ killjb(Job jn, int sig) { Process pn; int err = 0; - + if (jobbing) { if (jn->stat & STAT_SUPERJOB) { if (sig == SIGCONT) { @@ -731,11 +731,21 @@ killjb(Job jn, int sig) if (killpg(pn->pid, sig) == -1) if (kill(pn->pid, sig) == -1 && errno != ESRCH) err = -1; - + + /* + * Note this does not kill the last process, + * which is assumed to be the one controlling the + * subjob, i.e. the forked zsh that was originally + * list_pipe_pid... + */ for (pn = jn->procs; pn->next; pn = pn->next) if (kill(pn->pid, sig) == -1 && errno != ESRCH) err = -1; + /* + * ...we only continue that once the external processes + * currently associated with the subjob are finished. + */ if (!jobtab[jn->other].procs && pn) if (kill(pn->pid, sig) == -1 && errno != ESRCH) err = -1; @@ -744,7 +754,7 @@ killjb(Job jn, int sig) } if (killpg(jobtab[jn->other].gleader, sig) == -1 && errno != ESRCH) err = -1; - + if (killpg(jn->gleader, sig) == -1 && errno != ESRCH) err = -1; -- cgit v1.2.3 From e35dcae40fca1baebc202561040f5c6eec421613 Mon Sep 17 00:00:00 2001 From: Peter Stephenson Date: Sun, 25 Sep 2016 19:18:43 +0100 Subject: 39436: Pass on status of SIGINT better. Set lastval to 128 + SIGINT on interrupt. Don't execute builtin if already interrupted at that point. --- ChangeLog | 3 +++ Src/exec.c | 3 ++- Src/signals.c | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) (limited to 'Src/signals.c') diff --git a/ChangeLog b/ChangeLog index 967c63170..f2404aeba 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ 2016-09-25 Peter Stephenson + * 39436: Src/exec.c, Src/signals.c: Don't execute builtin if + interrupted; set lastval to 128 + SIGINT on interrupt. + * 39435: Src/exec.c: Don't set gleader of SUBJOB immediately if SUPERJOB has no processes. diff --git a/Src/exec.c b/Src/exec.c index a5086c33c..4e8934061 100644 --- a/Src/exec.c +++ b/Src/exec.c @@ -3701,7 +3701,8 @@ execcmd(Estate state, int input, int output, int how, int last1) state->pc = opc; } dont_queue_signals(); - lastval = execbuiltin(args, assigns, (Builtin) hn); + if (!errflag) + lastval = execbuiltin(args, assigns, (Builtin) hn); if (do_save & BINF_COMMAND) errflag &= ~ERRFLAG_ERROR; restore_queue_signals(q); diff --git a/Src/signals.c b/Src/signals.c index 30dde713f..e2587dc72 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -646,6 +646,7 @@ zhandler(int sig) inerrflush(); check_cursh_sig(SIGINT); } + lastval = 128 + SIGINT; } break; -- cgit v1.2.3 From 4abbb4b4739598c50973593d2787696116e49fcf Mon Sep 17 00:00:00 2001 From: "Barton E. Schaefer" Date: Mon, 3 Oct 2016 09:37:10 -0700 Subject: 39548: DEBUG for queueing_enabled --- ChangeLog | 2 ++ Src/signals.c | 4 ++++ Src/signals.h | 31 ++++++++++++++++++++++++++++--- 3 files changed, 34 insertions(+), 3 deletions(-) (limited to 'Src/signals.c') diff --git a/ChangeLog b/ChangeLog index 81d31c969..a7dfa2e3c 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,7 @@ 2016-10-03 Barton E. Schaefer + * 39548: Src/signals.c, Src/signals.h: DEBUG for queueing_enabled + * 39547: Src/Zle/zle_main.c: handle zero delta in calc_timeout() 2016-10-03 Peter Stephenson diff --git a/Src/signals.c b/Src/signals.c index e2587dc72..9e05add09 100644 --- a/Src/signals.c +++ b/Src/signals.c @@ -72,6 +72,10 @@ mod_export int queueing_enabled, queue_front, queue_rear; mod_export int signal_queue[MAX_QUEUE_SIZE]; /**/ mod_export sigset_t signal_mask_queue[MAX_QUEUE_SIZE]; +#ifdef DEBUG +/**/ +mod_export int queue_in; +#endif /* Variables used by trap queueing */ diff --git a/Src/signals.h b/Src/signals.h index d68096891..1904f4326 100644 --- a/Src/signals.h +++ b/Src/signals.h @@ -82,8 +82,6 @@ #define MAX_QUEUE_SIZE 128 -#define queue_signals() (queueing_enabled++) - #define run_queued_signals() do { \ while (queue_front != queue_rear) { /* while signals in queue */ \ sigset_t oset; \ @@ -94,12 +92,35 @@ } \ } while (0) +#ifdef DEBUG + +#define queue_signals() (queue_in++, queueing_enabled++) + #define unqueue_signals() do { \ DPUTS(!queueing_enabled, "BUG: unqueue_signals called but not queueing"); \ + --queue_in; \ if (!--queueing_enabled) run_queued_signals(); \ } while (0) -#define queue_signal_level() queueing_enabled +#define dont_queue_signals() do { \ + queue_in = queueing_enabled; \ + queueing_enabled = 0; \ + run_queued_signals(); \ +} while (0) + +#define restore_queue_signals(q) do { \ + DPUTS2(queueing_enabled && queue_in != q, \ + "BUG: q = %d != queue_in = %d", q, queue_in); \ + queue_in = (queueing_enabled = (q)); \ +} while (0) + +#else /* !DEBUG */ + +#define queue_signals() (queueing_enabled++) + +#define unqueue_signals() do { \ + if (!--queueing_enabled) run_queued_signals(); \ +} while (0) #define dont_queue_signals() do { \ queueing_enabled = 0; \ @@ -108,6 +129,10 @@ #define restore_queue_signals(q) (queueing_enabled = (q)) +#endif /* DEBUG */ + +#define queue_signal_level() queueing_enabled + #ifdef BSD_SIGNALS #define signal_block(S) sigblock(S) #else -- cgit v1.2.3