[PATCH v3 1/3] builtins: kill: Add basic test suite

[PATCH v3 1/3] builtins: kill: Add basic test suite

[Chris Down]

This is not totally comprehensive, but at least it's a start for the
core functionality. In the next commits, we'll also use this base to add
some regression tests.
---
 Test/B11kill.ztst | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Test/B11kill.ztst

diff --git a/Test/B11kill.ztst b/Test/B11kill.ztst
new file mode 100644
index 000000000..c254b0925
--- /dev/null
+++ b/Test/B11kill.ztst
@@ -0,0 +1,60 @@
+# Tests for the kill builtin.
+#
+# The exit codes 11 and 19 in this file don't mean anything special, they're
+# just an exit code which is specific enough that the failure of `kill` itself
+# can be differentiated from exiting due to executing a trap.
+
+%test
+
+# Correct invocation
+
+  if zmodload zsh/system &>/dev/null; then
+    (
+      trap 'exit 19' TERM
+      kill $sysparams[pid]
+    )
+  else
+    ZTST_skip='Cannot zmodload zsh/system, skipping kill with no sigspec'
+  fi
+19:kill with no sigspec
+
+
+  if zmodload zsh/system &>/dev/null; then
+    (
+      trap 'exit 11' USR1
+      kill -USR1 $sysparams[pid]
+    )
+  else
+    ZTST_skip='Cannot zmodload zsh/system, skipping kill with sigspec'
+  fi
+11:kill with sigspec
+
+# Incorrect invocation
+
+  (
+    kill a b c
+  )
+3:kill with multiple wrong inputs should increment status
+?(eval):kill:2: illegal pid: a
+?(eval):kill:2: illegal pid: b
+?(eval):kill:2: illegal pid: c
+
+  (
+    kill -INT a b c
+  )
+3:kill with sigspec and wrong inputs should increment status
+?(eval):kill:2: illegal pid: a
+?(eval):kill:2: illegal pid: b
+?(eval):kill:2: illegal pid: c
+
+  (
+    kill
+  )
+1:kill with no arguments
+?(eval):kill:2: not enough arguments
+
+  (
+    kill -INT
+  )
+1:kill with sigspec only
+?(eval):kill:2: not enough arguments
-- 
2.25.0

[PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Chris Down]

The version without a sigspec can't be added yet because it would still
kill the test runner even in expected-to-fail mode, see workers/45449
for discussion. For the same reason, we use a signal which is non-fatal
by default and unlikely to be sent by someone else, SIGURG, to do the
expected-to-fail case prior to the fix.
---
 Test/B11kill.ztst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Test/B11kill.ztst b/Test/B11kill.ztst
index c254b0925..fe2da1012 100644
--- a/Test/B11kill.ztst
+++ b/Test/B11kill.ztst
@@ -58,3 +58,12 @@
   )
 1:kill with sigspec only
 ?(eval):kill:2: not enough arguments
+
+# Regression tests: `kill ''` should not result in `kill 0`.
+
+  (
+    trap 'exit 11' URG
+    kill -URG ''
+  )
+1f:kill with empty pid and sigspec should not send signal to current process group
+?(eval):kill:3: illegal pid: 
-- 
2.25.0

[PATCH v3 3/3] builtins: kill: Do not signal current process group when pid is empty

[Chris Down]

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.
---
 Src/jobs.c        |  5 +++--
 Test/B11kill.ztst | 10 +++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

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 fe2da1012..ef263126a 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: 
+
-- 
2.25.0

Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Daniel Shahaf]

Good morning Chris,

Chris Down wrote on Mon, 17 Feb 2020 10:12 -0500:
> The version without a sigspec can't be added yet because it would still
> kill the test runner even in expected-to-fail mode, see workers/45449
> for discussion. For the same reason, we use a signal which is non-fatal
> by default and unlikely to be sent by someone else, SIGURG, to do the
> expected-to-fail case prior to the fix.

Do you consider the rationale for using SIGURG important enough to be
added in a code comment?

I've pushed this series to master with prose changes only.

Thanks a lot!

Daniel

Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Chris Down]

Morning!

Daniel Shahaf writes:
>Chris Down wrote on Mon, 17 Feb 2020 10:12 -0500:
>> The version without a sigspec can't be added yet because it would still
>> kill the test runner even in expected-to-fail mode, see workers/45449
>> for discussion. For the same reason, we use a signal which is non-fatal
>> by default and unlikely to be sent by someone else, SIGURG, to do the
>> expected-to-fail case prior to the fix.
>
>Do you consider the rationale for using SIGURG important enough to be
>added in a code comment?

Hmm, when I was considering whether to write it in the commit message or in the 
file, my rationale was that after 3/3 we're still going to end up killing the 
whole process group if it regresses, so likely the only people who'd run into 
this distinction are those doing `git bisect`. As such the commit message 
seemed the best place to me. I'm neutral though :-)

>I've pushed this series to master with prose changes only.

Thank you, and thanks for your diligent review!

Chris

Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Daniel Shahaf]

Chris Down wrote on Tue, 18 Feb 2020 10:26 -0500:
> Daniel Shahaf writes:
> >Chris Down wrote on Mon, 17 Feb 2020 10:12 -0500:  
> >> The version without a sigspec can't be added yet because it would still
> >> kill the test runner even in expected-to-fail mode, see workers/45449
> >> for discussion. For the same reason, we use a signal which is non-fatal
> >> by default and unlikely to be sent by someone else, SIGURG, to do the
> >> expected-to-fail case prior to the fix.  
> >
> >Do you consider the rationale for using SIGURG important enough to be
> >added in a code comment?  
> 
> Hmm, when I was considering whether to write it in the commit message or in the 
> file, my rationale was that after 3/3 we're still going to end up killing the 
> whole process group if it regresses, so likely the only people who'd run into 
> this distinction are those doing `git bisect`. As such the commit message 
> seemed the best place to me. I'm neutral though :-)

I understand, but I'm not convinced:

1. zsh's test suite aborts a test file as soon as any test group
fails.  Thus, if the bug regresses, the URG test will run but the TERM
test won't.

2. The URG test may be copied and adapted.

3. The reasons for picking URG are of interest to anyone who reads the
code (for whatever reason), even if few people will do so.

> >I've pushed this series to master with prose changes only.  
> 
> Thank you, and thanks for your diligent review!

You're welcome!

Cheers,

Daniel

Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Chris Down]

Daniel Shahaf writes:
>I understand, but I'm not convinced:
>
>1. zsh's test suite aborts a test file as soon as any test group
>fails.  Thus, if the bug regresses, the URG test will run but the TERM
>test won't.

Ah! Ok, I thought I saw that behaviour before but didn't think too much about 
it. In which case yes, it's still valuable.

>2. The URG test may be copied and adapted.
>
>3. The reasons for picking URG are of interest to anyone who reads the
>code (for whatever reason), even if few people will do so.

Cool, you've convinced me. I'll put up a small patch.

Thanks!

Chris

Re: [PATCH v3 2/3] builtins: kill: Add `kill ''` regression test with explicit sigspec

[Daniel Shahaf]

Chris Down wrote on Tue, 18 Feb 2020 13:50 -0500:
> Cool, you've convinced me. I'll put up a small patch.

Applied, thanks!

Cheers,

Daniel