From adc6a64b82a017949409abf143a92524f160fd73 Mon Sep 17 00:00:00 2001 From: Martijn Dekker Date: Thu, 14 Jul 2022 05:51:17 +0200 Subject: [PATCH] Fix bad job control msg on trapped SIGINT and set -b (re: fc655f1) When running an external command while trapping Ctrl+C via SIGINT, and set -b is on, then a spurious Done job control message is printed. No background job was executed. $ trap 'ls' INT $ set -b $ [file listing follows] [1] + Done set -b In jobs.c (487-493), job_reap() calls job_list() to list a running or completed background job, passing the JOB_NFLAG bit to only print jobs with the P_NOTIFY flag. But the 'ls' in the trap is not a background job. So it is getting the P_NOTIFY flag by mistake. In fact all processes get the P_NOTIFY flag by default when they terminate. Somehow the shell normally does not follow a code path that calls job_list() for foreground processes, but does when running one from a trap. I have not yet figured out how that works. What I do know is that there is no reason why non-background processes should ever have the P_NOTIFY flag set on termination, because those should never print any 'Done' messages. And we seem to have a handy P_BG flag that is set for background processes; we can check for this before setting P_NOTIFY. The only thing is that flag is only compiled in if SHOPT_BGX is enabled, as it was added to support that functionality. For some reason I am unable to reproduce the bug in a pty session, so there is no pty.sh regression test. src/cmd/ksh93/sh/jobs.c: - Rename misleadingly named P_FG flag to P_MOVED2FG; this flag is not set for all foreground processes but only for processes moved to the foreground by job_switch(), called by the fg command. - Compile in the P_BG flag even when SHOPT_BGX is not enabled. We need to set this flag to check for a background job. - job_reap(): Do not set the P_NOTIFY flag for all terminated processes, but only for those with P_BG set. src/cmd/ksh93/sh/xec.c: sh_fork(): - Also pass special argument 1 for background job to job_post() if SHOPT_BGX is not enabled. This is what gets it to set P_BG. - Simplify 5 lines of convoluted code into 1. Resolves: https://github.com/ksh93/ksh/issues/481 --- NEWS | 5 ++++ src/cmd/ksh93/include/version.h | 2 +- src/cmd/ksh93/sh/jobs.c | 48 +++++++++++++++------------------ src/cmd/ksh93/sh/xec.c | 10 +------ 4 files changed, 28 insertions(+), 37 deletions(-) diff --git a/NEWS b/NEWS index c6a1e5e4d..2c0fcf309 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0 Any uppercase BUG_* names are modernish shell bug IDs. +2022-07-14: + +- Fixed a bug that caused a spurious "Done" message on the interactive shell + when an external command was run as a foreground job from a SIGINT trap. + 2022-07-12: - The .sh.level variable can now only be changed within a DEBUG trap. When diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h index c09d3f754..20d4fae9a 100644 --- a/src/cmd/ksh93/include/version.h +++ b/src/cmd/ksh93/include/version.h @@ -23,7 +23,7 @@ #define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */ #define SH_RELEASE_SVER "1.0.0-beta.2" /* semantic version number: https://semver.org */ -#define SH_RELEASE_DATE "2022-07-12" /* must be in this format for $((.sh.version)) */ +#define SH_RELEASE_DATE "2022-07-14" /* must be in this format for $((.sh.version)) */ #define SH_RELEASE_CPYR "(c) 2020-2022 Contributors to ksh " SH_RELEASE_FORK /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */ diff --git a/src/cmd/ksh93/sh/jobs.c b/src/cmd/ksh93/sh/jobs.c index 974330b2e..4f9a09971 100644 --- a/src/cmd/ksh93/sh/jobs.c +++ b/src/cmd/ksh93/sh/jobs.c @@ -153,10 +153,8 @@ struct back_save #define P_DONE 040 #define P_COREDUMP 0100 #define P_DISOWN 0200 -#define P_FG 0400 -#if SHOPT_BGX -#define P_BG 01000 -#endif /* SHOPT_BGX */ +#define P_MOVED2FG 0400 /* set if the process was moved to the foreground by job_switch() */ +#define P_BG 01000 /* set if the process is running in the background */ static int job_chksave(pid_t); static struct process *job_bypid(pid_t); @@ -405,9 +403,12 @@ int job_reap(register int sig) sh_subjobcheck(pid); pw->p_flag &= ~(P_STOPPED|P_SIGNALLED); + pw->p_flag |= P_DONE; + if(pw->p_flag&P_BG) + pw->p_flag |= P_NOTIFY; if (WIFSIGNALED(wstat)) { - pw->p_flag |= (P_DONE|P_NOTIFY|P_SIGNALLED); + pw->p_flag |= P_SIGNALLED; if (WTERMCORE(wstat)) pw->p_flag |= P_COREDUMP; pw->p_exit = WTERMSIG(wstat); @@ -424,13 +425,12 @@ int job_reap(register int sig) } else { - pw->p_flag |= (P_DONE|P_NOTIFY); pw->p_exit = pw->p_exitmin; if(WEXITSTATUS(wstat) > pw->p_exitmin) pw->p_exit = WEXITSTATUS(wstat); } #if SHOPT_BGX - if((pw->p_flag&P_DONE) && (pw->p_flag&P_BG)) + if(pw->p_flag&P_BG) { job.numbjob--; if(sh.st.trapcom[SIGCHLD]) @@ -786,7 +786,7 @@ static void job_reset(register struct process *pw) return; #endif /* SIGTSTP */ /* force the following tty_get() to do a tcgetattr() unless fg */ - if(!(pw->p_flag&P_FG)) + if(!(pw->p_flag&P_MOVED2FG)) tty_set(-1, 0, NIL(struct termios*)); if(pw && (pw->p_flag&P_SIGNALLED) && pw->p_exit!=SIGHUP) { @@ -1206,18 +1206,16 @@ void job_clear(void) } /* - * put the process on the process list and return the job number - * if non-zero, is the process ID of the job to join + * Put the process on the process list and return the job number. + * If is 1, the job is marked as a background job (P_BG); + * otherwise, if non-zero, is the process ID of the job to join. */ int job_post(pid_t pid, pid_t join) { register struct process *pw; register History_t *hp = sh.hist_ptr; -#if SHOPT_BGX - int val,bg=0; -#else int val; -#endif + char bg = 0; sh.jobenv = sh.curenv; if(job.toclear) { @@ -1225,14 +1223,14 @@ int job_post(pid_t pid, pid_t join) return(0); } job_lock(); -#if SHOPT_BGX if(join==1) { join = 0; - bg = P_BG; + bg++; +#if SHOPT_BGX job.numbjob++; - } #endif /* SHOPT_BGX */ + } if(njob_savelist < NJOB_SAVELIST) init_savelist(); if(pw = job_bypid(pid)) @@ -1316,15 +1314,15 @@ int job_post(pid_t pid, pid_t join) else pw->p_flag |= (P_DONE|P_NOTIFY); } -#if SHOPT_BGX if(bg) { - if(pw->p_flag&P_DONE) - job.numbjob--; - else + if(!(pw->p_flag&P_DONE)) pw->p_flag |= P_BG; - } +#if SHOPT_BGX + else + job.numbjob--; #endif /* SHOPT_BGX */ + } lastpid = 0; job_unlock(); return(pw->p_job); @@ -1608,9 +1606,7 @@ int job_switch(register struct process *pw,int bgflag) { sfprintf(outfile,"[%d]\t",(int)pw->p_job); sh.bckpid = pw->p_pid; -#if SHOPT_BGX pw->p_flag |= P_BG; -#endif msg = "&"; } else @@ -1631,10 +1627,8 @@ int job_switch(register struct process *pw,int bgflag) return(1); } job.waitall = 1; - pw->p_flag |= P_FG; -#if SHOPT_BGX + pw->p_flag |= P_MOVED2FG; pw->p_flag &= ~P_BG; -#endif job_wait(pw->p_pid); job.waitall = 0; } diff --git a/src/cmd/ksh93/sh/xec.c b/src/cmd/ksh93/sh/xec.c index 4101bd98f..e67b9bcc1 100644 --- a/src/cmd/ksh93/sh/xec.c +++ b/src/cmd/ksh93/sh/xec.c @@ -2887,15 +2887,7 @@ pid_t _sh_fork(register pid_t parent,int flags,int *jobid) sh.cpid = parent; if(!postid && job.curjobid && (flags&FPOU)) postid = job.curpgid; -#if SHOPT_BGX - if(!postid && (flags&(FAMP|FINT)) == (FAMP|FINT)) - postid = 1; - myjob = job_post(parent,postid); - if(postid==1) - postid = 0; -#else - myjob = job_post(parent,postid); -#endif /* SHOPT_BGX */ + myjob = job_post(parent, (!postid && (flags&(FAMP|FINT))==(FAMP|FINT)) ? 1 : postid); if(job.waitall && (flags&FPOU)) { if(!job.curjobid)