-
Notifications
You must be signed in to change notification settings - Fork 7.3k
process: throw TypeError if kill pid not a number #7991
Conversation
Currently, invalid usage such as: process.kill('SIGTERM') process.kill(null) process.kill(undefined); all coerce the pid to 0, and signal the current process.
@@ -716,6 +716,10 @@ | |||
process.kill = function(pid, sig) { | |||
var r; | |||
|
|||
if (typeof pid !== 'number') { // Copy of util.isNumber() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're going to verify that pid
is a number, then it might be worth adding an isFinite()
check to rule out Infinity
and NaN
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively you can do a type coercion if the string can be properly coerced to a string. For example:
if (typeof pid !== 'number' || !isNaN(pid)) {
// This forces a uint coercion. So might also want to check
// if the pid is > 0.
pid = pid >>> 0;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Updated the first example for syntax error, and &&
should have been ||
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"if the string can be properly coerced to a string", did you mean to a number?
I explicitly disallow string forms of numbers, if you grep 'TypeError.*number' lib/*.js
, I think you'll find that (mostly) util.isNumber() is used for similar checks, and it doesn't allow string forms of numbers.
It also passes for NaN and Infinity.... but that's another story :-)
That doesn't seem like an appropriate change for the stable branch. I think that the check should be EDIT: Replace |
I totally agree with @bnoordhuis , let's do it in v0.11 |
Landed in 832ec1c, thank you! |
@bnoordhuis good point about being able to pass
|
Not allowing string was a change from v0.10 behaviour, commented on in #7991. Allow them again, but still check that argument is numberish. Also, simplify the fragile and non-portable test code introduced in 832ec1c that required fixups 2a41535, and ef3c4ed. PR-URL: #8531 Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Fix #7962
Note that this PR disallows string forms of process ids, so will break code like:
called as
node kill.js 6543
.I could allow string forms of numbers, but disallow strings that are NOT numbers, I could also allow both null and undefined to convert to 0, meaning self, but that seems weird. Note that
+null
is0
in js, and+undefined
isNaN
, though how much sense that makes, I'm not sure.Node, in general, does precious little argument checking, its hard to know what would be expected (other than calling APIs in contradiction of their docs is not going to work well).