Skip to content

Commit

Permalink
kill: do not send SIGCONT along with every signal (!)
Browse files Browse the repository at this point in the history
Reproducer 1:

  $ ksh -c 'trap "echo SIGCONT" CONT; trap "echo SIGUSR1" USR1;
	 kill -s USR1 $$'
  SIGUSR1
  SIGCONT

Why on earth is the SIGCONT trap triggered? Turns out the 'kill'
command sends SIGCONT as well as SIGUSR1, which is very wrong. No
other shell acts like this and neither does any external 'kill'
command, and of course this violates POSIX as well.

Reproducer 2:

  $ ksh
  $ suspend
  [1] + Stopped (SIGSTOP)        ksh
  $ kill %%
  $ $ t
  $ es
  $ t

After suspending the child ksh, 'kill %%' is supposed to send it
SIGTERM, which is ignored by an interactive shell. But it also
wrongly sends it SIGCONT, resuming it while it is still in the
background. So now you have two shells competing for your keybaord
input, and chaos ensues. The above reproducer shows a possible
result of typing 'test' (with no RETURN key).

Archaeological research shows that the offending code was present
as early as ksh88, so the reasons are lost to history.

src/cmd/ksh93/sh/jobs.c: job_kill():
- Remove code sending SIGCONT along with every signal.
- Only clear the P_STOPPED flag in the job entry or call
  job_unstop() if the signal is SIGCONT.
- Tweak for code legibility.
  • Loading branch information
McDutchie committed Jan 12, 2024
1 parent 7d75ed4 commit c7e1225
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 26 deletions.
7 changes: 7 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-01-11:

- Fixed ancient incorrect behaviour in the 'kill' built-in: it sent SIGCONT
to the given process along with every other non-SIGCONT signal issued,
causing a stopped process to be incorrectly resumed upon sending it any
signal and causing any SIGCONT trap handler to be incorrectly triggered.

2024-01-05:

- Fixed a potential crash on pressing Ctrl+C on the command line while
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.9-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-01-08" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-01-11" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
35 changes: 10 additions & 25 deletions src/cmd/ksh93/sh/jobs.c
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,6 @@ int job_kill(struct process *pw,int sig)
pid_t pid;
int r = -1;
const char *msg;
int stopsig = (sig==SIGSTOP||sig==SIGTSTP||sig==SIGTTIN||sig==SIGTTOU);
job_lock();
errno = ECHILD;
if(!pw)
Expand All @@ -992,45 +991,31 @@ int job_kill(struct process *pw,int sig)
errno = EPERM;
r = -1;
}
else if(pid>=0)
{
r = kill(pid,sig);
if(r>=0 && sig==SIGCONT && (pw->p_flag&P_STOPPED))
pw->p_flag &= ~(P_STOPPED|P_SIGNALLED);
}
else
{
if(pid>=0)
{
if((r = kill(pid,sig))>=0 && !stopsig)
{
if(pw->p_flag&P_STOPPED)
pw->p_flag &= ~(P_STOPPED|P_SIGNALLED);
if(sig)
kill(pid,SIGCONT);
}
}
else
{
if((r = killpg(-pid,sig))>=0 && !stopsig)
{
job_unstop(job_bypid(pw->p_pid));
if(sig)
killpg(-pid,SIGCONT);
}
}
r = killpg(-pid,sig);
if(r>=0 && sig==SIGCONT)
job_unstop(job_bypid(pw->p_pid));
}
}
else
{
if(pid = pw->p_pgrp)
{
r = killpg(pid,sig);
if(r>=0 && (sig==SIGHUP||sig==SIGTERM || sig==SIGCONT))
if(r>=0 && sig==SIGCONT)
job_unstop(pw);
if(r>=0)
sh_delay(.05,0);
}
while(pw && pw->p_pgrp==0 && (r=kill(pw->p_pid,sig))>=0)
{
if(sig==SIGHUP || sig==SIGTERM)
kill(pw->p_pid,SIGCONT);
pw = pw->p_nxtproc;
}
}
if(r<0 && job_string)
{
Expand Down
12 changes: 12 additions & 0 deletions src/cmd/ksh93/tests/signal.sh
Original file line number Diff line number Diff line change
Expand Up @@ -588,5 +588,17 @@ float s=SECONDS
wait $!
(( (SECONDS-s) < 1.8)) && err_exit "'trap - INT' causing trap to not be ignored"
# ======
# Ancient SIGCONT nonsense present as early as ksh88:
# the 'kill' built-in sent SIGCONT along with every non-SIGCONT signal issued!
for sig in TERM HUP INT USR1
do for cmd in kill $(whence -p kill)
do got=$("$SHELL" -c "trap 'echo \"SIGCONT (!!)\"' CONT; trap 'echo SIG$sig' $sig; $cmd -s $sig \$\$")
[[ e=$? -eq 0 && $got == "SIG$sig" ]] || err_exit "CONT+$sig trap, SIG$sig issued using '$cmd':" \
"expected status 0, SIG$sig;" \
"got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))"
done
done
# ======
exit $((Errors<125?Errors:125))

0 comments on commit c7e1225

Please sign in to comment.