kill builtin argument parsing

kill builtin argument parsing

[Matthew Malcomson]

Hello there,

I recently mistakenly used arguments for the /bin/kill binary with the 
zsh kill builtin and noticed something strange that I think should be 
improved.

The command line I used was:
kill -1 -- <any pid>
which ends up killing the current Zsh process.

This is because the pid valid argument check at Src/builtin.c:2572 uses 
isanum() that just asserts that all characters in the string are either 
'-' or a digit.
When the string "--" is parsed as a digit with atoi() in Src/jobs.c:2572 
it returns '0' which is passed to kill(2) syscall.

I'm not sure whether to suggest changing the isanum() function, or the 
check in bin_kill() that relies on it.
I would personally change the isanum() function, as a cursory reading of 
the two places it's used appear to not require this particular behaviour 
(the other place it's used in Src/jobs.c:2193 doesn't appear to have any 
negative consequences either way), but the comment above this isanum() 
function clearly shows it was a known behaviour, so there may be a 
reason I'm missing.

Either way I think the behaviour is surprising enough and possible 
enough to be worth changing.


Cheers

MM

Re: kill builtin argument parsing

[Bart Schaefer]

On Jun 5,  5:35pm, Matthew Malcomson wrote:
}
} The command line I used was:
} kill -1 -- <any pid>
} which ends up killing the current Zsh process.

This is a little odd, because the code that examines argv[1] to see if
it is -l or -s or -n or -<NUM> or -<NAME> also checks whether it is
"--", but then simply discards that.  So you can do

    kill -- 12345

to send the default signal to process 12345, you just can't precede
the "--" with any of the other options.  I suspect this is so that
you can do

    kill -- -12345

i.e. force a negative PID in order to kill a process group instead of
having that interpreted as signal number 12345.

Anyway this is clearly incomplete handling of "--".  The patch below
means that

    kill -- --

will report "not enough arguments" rather than complaining about either
an invalid signal or an invalid PID, but that's probably OK.

diff --git a/Src/jobs.c b/Src/jobs.c
index 2a9dbe7..04cb6b3 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -2527,6 +2527,10 @@ bin_kill(char *nam, char **argv, UNUSED(Options ops), UNUSED(int func))
 	argv++;
     }
 
+    /* Discard the standard "-" and "--" option breaks */
+    if (*argv && (*argv)[0] == '-' && (!(*argv)[1] || (*argv)[1] == '-'))
+	argv++;
+
     if (!*argv) {
     	zwarnnam(nam, "not enough arguments");
 	return 1;