From fcd9efce7fc987076165825a1f2bc01cb6dc2122 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Wed, 22 Dec 2021 04:43:42 +0000 Subject: [PATCH] Interactive: Avoid losing the job after suspending a subshell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproducer: run vi in a subshell: $ (vi) vi opens; now press Ctrl+Z to suspend. The output is as expected: [2] + Stopped (vi) …but the exit status is 18 (SIGTSTP's signal number) instead of 0. Now do: $ fg (vi) $ The exit status is 18 again, vi is not resumed, and the job is lost. You have to find vi's pid manually using ps and kill it. Forking all non-command substitution subshells invoked from the interactive main shell is the only reliable and effective fix I've found. I've tried to fork the subshell conditionally in every other remotely plausible place I can think of in fault.c and xec.c, but I can't get anything to work properly. If anyone can get this to work without forking as much (or at all), please do submit a patch or PR that supersedes this fix. At least subshells of subshells don't need to fork, so the performance impact can be limited. Plus, it's not as if most people need maximum speed on the interactive command line. Scripts (including login/profile scripts) are not affected at all. Command substitutions can be handled differently. My testing shows that all shells except ksh93 simply block SIGTSTP (the ^Z signal) while they run. We should do the same, so they don't need to fork. NOTE for any backporters: the subshell.c and fault.c changes depend on commits 35b02626 and 48ba6964 to work correctly. src/cmd/ksh93/sh/subshell.c: sh_subshell(): - If the interactive shell state bit is on, then before executing the subshell's code: - for command substitutions, block SIGTSTP; - for other subshells, fork. - For command substitutions, release SIGTSTP if the interactive shell state bit was on upon invoking the subshell. src/cmd/ksh93/sh/fault.c: - Instead of checking for a virtual subshell, check the shell's interactive state bit to decide whether to handle SIGTSTP, as that is only turned on in the interactive main shell. src/cmd/ksh93/sh/main.c: sh_main(): - To avoid bugs, ignore SIGTSTP while running profile scripts. Blocking it doesn't work because delaying it until after sigrelease() will cause a crash. Thanks to @JohnoKing for this. - While we're here, prevent a possible overflow of the 'beenhere' static char variable by only incrementing it once. Co-authored-by: Johnothan King Resolves: https://github.com/ksh93/ksh/issues/390 --- NEWS | 9 +++++++++ src/cmd/ksh93/sh/fault.c | 4 ++-- src/cmd/ksh93/sh/main.c | 5 ++++- src/cmd/ksh93/sh/subshell.c | 14 ++++++++++++++ 4 files changed, 29 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 02c5cbd7c..67f963541 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,15 @@ Any uppercase BUG_* names are modernish shell bug IDs. [1] 30909 <--- incorrect job control output from subshell done +- Fixed: after suspending (Ctrl+Z) a subshell that is running an external + command, resuming the subshell with 'fg' failed and the job was lost. + $ (vi) <--- press Ctrl+Z + [2] + Stopped (vi) + $ fg + (vi) <--- vi failed to resume; immediate return to command line + $ fg + ksh: no such job + 2021-12-17: - Release 1.0.0-beta.2. diff --git a/src/cmd/ksh93/sh/fault.c b/src/cmd/ksh93/sh/fault.c index 92146f40d..1ba4a38b1 100644 --- a/src/cmd/ksh93/sh/fault.c +++ b/src/cmd/ksh93/sh/fault.c @@ -547,7 +547,7 @@ void sh_exit(register int xno) sh_offstate(SH_STOPOK); shp->trapnote = 0; shp->forked = 1; - if(!shp->subshell && (sig=sh_fork(shp,0,NIL(int*)))) + if(sh_isstate(SH_INTERACTIVE) && (sig=sh_fork(shp,0,NIL(int*)))) { job.curpgid = 0; job.parent = (pid_t)-1; @@ -564,7 +564,7 @@ void sh_exit(register int xno) { if(shp->subshell) sh_subfork(); - /* child process, put to sleep */ + /* script or child process; put to sleep */ sh_offstate(SH_STOPOK); sh_offstate(SH_MONITOR); shp->sigflag[SIGTSTP] = 0; diff --git a/src/cmd/ksh93/sh/main.c b/src/cmd/ksh93/sh/main.c index 0ce2d81a9..17efc8451 100644 --- a/src/cmd/ksh93/sh/main.c +++ b/src/cmd/ksh93/sh/main.c @@ -160,9 +160,11 @@ int sh_main(int ac, char *av[], Shinit_f userinit) else sh_onoption(SH_BRACEEXPAND); #endif - if((beenhere++)==0) + if(!beenhere) { + beenhere++; sh_onstate(SH_PROFILE); + shp->sigflag[SIGTSTP] |= SH_SIGIGNORE; if(shp->gd->ppid==1) shp->login_sh++; if(shp->login_sh >= 2) @@ -236,6 +238,7 @@ int sh_main(int ac, char *av[], Shinit_f userinit) sh_source(shp, iop, e_suidprofile); } shp->st.cmdname = error_info.id = command; + shp->sigflag[SIGTSTP] &= ~(SH_SIGIGNORE); sh_offstate(SH_PROFILE); if(rshflag) sh_onoption(SH_RESTRICTED); diff --git a/src/cmd/ksh93/sh/subshell.c b/src/cmd/ksh93/sh/subshell.c index 81ab3ae01..f76a6168e 100644 --- a/src/cmd/ksh93/sh/subshell.c +++ b/src/cmd/ksh93/sh/subshell.c @@ -660,6 +660,16 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) sh_subfork(); } #endif /* _lib_fchdir */ +#ifdef SIGTSTP + /* Virtual subshells are not safe to suspend (^Z, SIGTSTP) in the interactive main shell. */ + if(sh_isstate(SH_INTERACTIVE)) + { + if(comsub) + sigblock(SIGTSTP); + else if(!sh_isstate(SH_PROFILE)) + sh_subfork(); + } +#endif sh_offstate(SH_INTERACTIVE); sh_exec(t,flags); } @@ -686,6 +696,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub) nv_restore(sp); if(comsub) { +#ifdef SIGTSTP + if(savst.states & sh_state(SH_INTERACTIVE)) + sigrelease(SIGTSTP); +#endif /* re-enable job control */ if(!sp->nofork) sh_offstate(SH_NOFORK);