Three bugs, all causing %- to malfunction after fg %- on a
function superjob (e.g. v() { vim "$@"; }; v, suspend, start
another job, %-).
Bug 1: setprevjob() picks internal jobs as previous job
When fg %- runs, the shuffle code calls setprevjob() to find
a new previous job. It already excludes STAT_SUBJOB jobs, but
not STAT_NOPRINT jobs. The fg builtin itself gets a job table
entry (with STAT_BUILTIN|STAT_NOPRINT), and setprevjob()
picks it. Then the next %- resolves to that internal job,
which bin_fg rejects at the STAT_NOPRINT check:
"%-: no such job".
Fix: exclude STAT_NOPRINT from both loops in setprevjob(),
same as STAT_SUBJOB.
Bug 2: setprevjob() can pick the job being fg'd
The shuffle code in bin_fg calls setprevjob() to choose a new
previous job, but setprevjob() only excludes curjob and
thisjob. Since thisjob at that point is the fg builtin's own
job (not the job being operated on), setprevjob() can pick
the fg'd job itself as prevjob.
Fix: temporarily set thisjob to the fg'd job around the
shuffle block so setprevjob() won't consider it.
Bug 3: Subjob handler doesn't update curjob/prevjob
When a function's child (the subjob) stops, update_job()
takes the STAT_SUBJOB early-return path at line 539. This
path sets STAT_STOPPED on the superjob but never runs the
prevjob = curjob; curjob = job assignment at lines 635-638.
The superjob's own update_job() then returns early too
(line 542: already STAT_STOPPED). So curjob/prevjob are
never updated after re-suspension, leaving prevjob pointing
at whatever stale value the shuffle left behind.
Fix: add the curjob/prevjob update in the subjob handler,
right after it marks the superjob as STAT_STOPPED. Since
STAT_STOPPED was just set unconditionally on the line above,
use !(sjn->stat & STAT_DONE) rather than the equivalent but
redundant (sjn->stat & (STAT_DONE | STAT_STOPPED)) ==
STAT_STOPPED.
---
Src/jobs.c | 36 +++++++++++++++++++++++++-----------
Test/W02jobs.ztst | 26 ++++++++++++++++++++++++++
2 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/Src/jobs.c b/Src/jobs.c
index 8353f11..d7df51f 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -500,6 +500,10 @@ update_job(Job jn)
* fg/bg is the superjob) a SIGCONT if we need it.
*/
sjn->stat |= STAT_CHANGED | STAT_STOPPED;
+ if (!(sjn->stat & STAT_DONE)) {
+ prevjob = curjob;
+ curjob = i;
+ }
if (isset(NOTIFY) && (sjn->stat & STAT_LOCKED) &&
!(sjn->stat & STAT_NOPRINT)) {
/*
@@ -661,13 +665,15 @@ setprevjob(void)
for (i = maxjob; i; i--)
if ((jobtab[i].stat & STAT_INUSE) && (jobtab[i].stat & STAT_STOPPED) &&
- !(jobtab[i].stat & STAT_SUBJOB) && i != curjob && i != thisjob) {
+ !(jobtab[i].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
+ i != curjob && i != thisjob) {
prevjob = i;
return;
}
for (i = maxjob; i; i--)
- if ((jobtab[i].stat & STAT_INUSE) && !(jobtab[i].stat & STAT_SUBJOB) &&
+ if ((jobtab[i].stat & STAT_INUSE) &&
+ !(jobtab[i].stat & (STAT_SUBJOB | STAT_NOPRINT)) &&
i != curjob && i != thisjob) {
prevjob = i;
return;
@@ -2449,15 +2455,21 @@ bin_fg(char *name, char **argv, Options ops, int func)
}
/* It's time to shuffle the jobs around! Reset the current job,
and pick a sensible secondary job. */
- if (curjob == job) {
- curjob = prevjob;
- prevjob = (func == BIN_BG) ? -1 : job;
- }
- if (prevjob == job || prevjob == -1)
- setprevjob();
- if (curjob == -1) {
- curjob = prevjob;
- setprevjob();
+ {
+ /* Exclude this job from setprevjob() consideration. */
+ int saved_thisjob = thisjob;
+ thisjob = job;
+ if (curjob == job) {
+ curjob = prevjob;
+ prevjob = (func == BIN_BG) ? -1 : job;
+ }
+ if (prevjob == job || prevjob == -1)
+ setprevjob();
+ if (curjob == -1) {
+ curjob = prevjob;
+ setprevjob();
+ }
+ thisjob = saved_thisjob;
}
if (func != BIN_WAIT)
/* for bg and fg -- show the job we are operating on */
diff --git a/Test/W02jobs.ztst b/Test/W02jobs.ztst
index d52888dd9..1dc6662a5 100644
--- a/Test/W02jobs.ztst
+++ b/Test/W02jobs.ztst
@@ -193,6 +193,32 @@
*>\[2] ? interrupt*sleep*
*>\[1] ? kill*sleep*
+# The following test exercises a bug where setprevjob() could pick an
+# internal NOPRINT job (e.g. a builtin's own job entry) as the previous
+# job. This happened when a function superjob was involved, because the
+# superjob's subjob was excluded but the builtin job was not.
+# The function's child stops itself, triggering the superjob mechanism.
+# Then fg %- resumes the superjob (which exits since the child is done).
+# Afterwards, the remaining sleep job should still be accessible.
+ zpty_start
+ zpty_input "f() { sh -c 'kill -TSTP 0' }"
+ zpty_input 'f'
+ zpty_line # consume stopped message from f
+ zpty_input 'sleep 100 &'
+ zpty_line # consume [N] PID
+ zpty_input 'kill -STOP %sleep'
+ zpty_line # consume stopped message from kill
+ zpty_input 'fg %-'
+ zpty_line # consume continued message
+ zpty_input 'jobs'
+ zpty_stop
+0:fg %- with function superjob does not pick internal job as previous
+*>zsh:*(stopped|suspended)*
+*>\[[0-9]##\] [0-9]##
+*>\[[0-9]##\] + (stopped|suspended)*sleep*
+*>\[[0-9]##\] continued*
+*>\[[0-9]##\] - (stopped|suspended)*sleep*
+
%clean
zmodload -ui zsh/zpty
--
2.43.0
The numeric job path (e.g. %1) already prints the jobspec via
specifier the user typed. Use the same "no such job" format
for consistency and to fix A05execution.ztst which expects it.