From 5c55b3fb50bbfe602fcfa55fa6258e398ecc6b20 Mon Sep 17 00:00:00 2001 From: Chris Down Date: Mon, 17 Feb 2020 10:12:11 -0500 Subject: 45453: builtins: kill: Do not signal current process group when pid is empty The following case was encountered in the wild: % zsh; echo "$?" % trap 'exit 5' TERM % kill '' 5 This behaviour seems more likely to be the result of bugs in programs (e.g. `kill -9 "$unsetvar") rather than being desirable behaviour to me. It also seems unintentional judging by the code and documentation, since it comes about as a result of the fact that: - `isanum` returns true for empty strings (since an empty string technically only consists of digits and minuses...); - `atoi`, when passed a pointer to an invalid number, returns 0; - `kill(0, signal)` sends the signal in question to all processes in the current process group. There are (at least) two ways to solve this issue: 1. Add special handling to `kill` to avoid this case. See this patch[0] for a version that does that. 2. Change how isanum behaves. Since the only two call sites that use it both seem like they should handle the case where the input char array is empty, that seems like a reasonable overall change to me.[1] After this patch: % trap 'exit 5' TERM % kill '' kill: illegal pid: The regression test for `kill` without a sigspec is also included in this commit, as previously it's not possible to test it trivially as it would still kill the test runner in expected-to-fail mode; see discussion in workers/45449. 0: workers/45426: https://www.zsh.org/mla/workers/2020/msg00251.html 1: The other call site using isanum() is the fg builtin, but in that case we just fail later since we can't find any job named '', so no big deal either way. It's the kill case which is more concerning. --- ChangeLog | 3 +++ Src/jobs.c | 5 +++-- Test/B11kill.ztst | 10 +++++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 162bde6b0..9a7ca36a0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,8 @@ 2020-02-18 Chris Down + * 45453: Src/jobs.c, Test/B11kill.ztst: builtins: kill: Do not + signal current process group when pid is empty + * 45452: Test/B11kill.ztst: builtins: kill: Add `kill ''` regression test with explicit sigspec diff --git a/Src/jobs.c b/Src/jobs.c index e7438251e..0485f2c7c 100644 --- a/Src/jobs.c +++ b/Src/jobs.c @@ -1854,13 +1854,14 @@ scanjobs(void) /* This simple function indicates whether or not s may represent * * a number. It returns true iff s consists purely of digits and * - * minuses. Note that minus may appear more than once, and the empty * - * string will produce a `true' response. */ + * minuses. Note that minus may appear more than once. */ /**/ static int isanum(char *s) { + if (*s == '\0') + return 0; while (*s == '-' || idigit(*s)) s++; return *s == '\0'; diff --git a/Test/B11kill.ztst b/Test/B11kill.ztst index 957a7b385..40dd99cd3 100644 --- a/Test/B11kill.ztst +++ b/Test/B11kill.ztst @@ -65,5 +65,13 @@ trap 'exit 11' URG kill -URG '' ) -1f:kill with empty pid and sigspec should not send signal to current process group +1:kill with empty pid and sigspec should not send signal to current process group ?(eval):kill:3: illegal pid: + + ( + trap 'exit 19' TERM + kill '' + ) +1:Plain kill with empty pid should not send signal to current process group +?(eval):kill:3: illegal pid: + -- cgit 1.4.1