child_process: improve killSignal validations by thefourtheye · Pull Request #10423 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

As it is, the killSignal is just retrieved from an object and used.
If the signal passed is actually one of the inherited properties of
that object, Node.js will die. For example,

➜  node -e "child_process.spawnSync('ls', {killSignal: 'toString'})"
Assertion failed: (0), function uv_close, file ....core.c, line 166.
[1]    58938 abort      node -e "child_process.spawnSync(...)"

This patch makes sure that the signal is actually a own property of
the constants object.
Extends the killSignal validation to all the other functions.

How much does this overlap or conflict with #8312?

#8312 validates the type of data passed as input to killSignal, but this patch fixes the problem in lookupSignal method itself.

We should avoid in, hasOwnProperty(), and unnecessarily checking the type again when the secondary condition fails. How about something like this:

exports.convertToValidSignal = function convertToValidSignal(signal) {
  if (typeof signal === 'number') {
    if (OS_SIGNALS_TO_NAMES_MAPPING[signal])
      return signal;
  } else if (typeof signal === 'string') {
    var val = OS_SIGNALS[signal];
    if (val !== undefined)
      return val;
  }
  throw new Error('Unknown signal: ' + signal);
};

unnecessarily checking the type again when the secondary condition fails

If the if fails, then typeof will be checked again, right?

I used changes in the else if part.

Wait, hasOwnProperty check is necessary here. The undefined check will pass if the key is an inherited property of OS_SIGNALS

Set the prototype for OS_SIGNALS to Object.create(null) and it won't be a problem.

@mscdex I think it would be possible if something like #10458 lands.

I don't think the test is necessary, there should be no inherited properties.

More to the point...add a unit test asserting that there are no inherited properties - try to .kill() with some strings which you think might be inherited method names, for example.

@sam-github You mean this test?

@thefourtheye so, we are in agreement then that the has own property check is not necessary. 👍

@sam-github The test which I mentioned above asserts that an error is thrown if an inherited property is used. But if #10458 lands, then we can get rid of the hasOwnProperty check.

What about combining the ternary expressions?:

!sig && sig !== 0 ? 'SIGTERM' : sig

Ack

Rebased. CI Run: https://ci.nodejs.org/job/node-test-pull-request/5877/

Could you change this to camelCase?

Ack!

Same comment here regarding camelCase.

Ack!

Maybe we should create this mapping lazily. It might help to switch to a for loop to speed things up too.

Ack!

Can you also add a case that passes with a number passed in.

Ack!

We could probably get away with dropping this test and add any necessary value checks to test-child-process-spawnsync-validation-errors.js.

Ack!

I think we should take this chance to change the check to sig == undefined or sig === undefined for defaulting to SIGTERM.

It falls back to SIGTERM now also, right? !sig will be true when sig is undefined and it is not zero, so SIGTERM.

I meant we could tighten this up to not convert falsy values like false or NaN to 'SIGTERM' if we wrote this like:

const signal = convertToValidSignal(sig === undefined ? 'SIGTERM' : sig);

I love it. Simple and easy to reason. Updated the PR now.

When you rebase this, can you move the convertToValidSignal() calls into validateKillSignal().

Ack!

@thefourtheye this is looking pretty good. Are you still interested in working on it?

@cjihrig Yes :-) And I addressed your comments. PTAL again :-)

Prior to this, zero could be passed to ChildProcess#kill(), but not as the kill signal in execFile() and company. That seems like something we might want to maintain.

I think zero is a special case only in ChildProcess#kill. So I special cased it there.

Can you call this osSignals, or just signals?

Ack!

I know there were previous comments about using in and hasOwnProperty().

Ack! But this depends on #10458 now

Only a couple small nits left from me.

Could you reverse the if logic and return if signalsToNamesMapping exists. That can save some indentation on the following lines.

Ack!

Is this one ready to land?

CI: https://ci.nodejs.org/job/node-test-pull-request/6986/

As it is, the `killSignal` is just retrieved from an object and used. If the signal passed is actually one of the inherited properties of that object, Node.js will die. For example, ➜ node -e "child_process.spawnSync('ls', {killSignal: 'toString'})" Assertion failed: (0), function uv_close, file ....core.c, line 166. [1] 58938 abort node -e "child_process.spawnSync(...)" 1. This patch makes sure that the signal is actually a own property of the constants object. 2. Extends the killSignal validation to all the other functions.

Rebased. CI: https://ci.nodejs.org/job/node-test-pull-request/7149/

@jasnell @cjihrig I updated the PR so that the signal names are case insensitive. PTAL.

@thefourtheye Is this still blocked? By what?

@addaleax It was blocked by #10458, now that it landed, this is okay.

@thefourtheye Do you want to go ahead and land this then? :)

Landed in d75fdd9

As it is, the `killSignal` is just retrieved from an object and used. If the signal passed is actually one of the inherited properties of that object, Node.js will die. For example, ➜ node -e "child_process.spawnSync('ls', {killSignal: 'toString'})" Assertion failed: (0), function uv_close, file ....core.c, line 166. [1] 58938 abort node -e "child_process.spawnSync(...)" 1. This patch makes sure that the signal is actually a own property of the constants object. 2. Extends the killSignal validation to all the other functions. PR-URL: #10423 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. util Issues and PRs related to the built-in util module. labels Dec 23, 2016

thefourtheye force-pushed the child-process-signal-validation branch 2 times, most recently from c6fb81b to 4aab8fa Compare December 23, 2016 12:32

mscdex reviewed Dec 23, 2016

View reviewed changes

thefourtheye mentioned this pull request Dec 26, 2016

doc: killSignal option accepts integer values #10424

Closed

2 tasks

thefourtheye force-pushed the child-process-signal-validation branch from 4aab8fa to 22d71b5 Compare December 26, 2016 06:43

nodejs-github-bot added the dont-land-on-v7.x label Dec 26, 2016

thefourtheye force-pushed the child-process-signal-validation branch from 22d71b5 to 25ded59 Compare December 26, 2016 06:51

nodejs-github-bot added the dont-land-on-v7.x label Dec 26, 2016

thefourtheye force-pushed the child-process-signal-validation branch from 25ded59 to 492260a Compare December 26, 2016 07:04

nodejs-github-bot added the dont-land-on-v7.x label Dec 26, 2016

thefourtheye mentioned this pull request Dec 26, 2016

lib,src: make constants not inherit from Object #10458

Closed

3 tasks

thefourtheye force-pushed the child-process-signal-validation branch from 492260a to 0b51a05 Compare January 15, 2017 18:46

nodejs-github-bot added the dont-land-on-v7.x label Jan 15, 2017

targos removed the dont-land-on-v7.x label Jan 28, 2017

cjihrig requested changes Feb 11, 2017

View reviewed changes

cjihrig mentioned this pull request Feb 11, 2017

test: improve child_process test coverage #11278

Closed

3 tasks

thefourtheye force-pushed the child-process-signal-validation branch from 0b51a05 to 9c46e6b Compare February 12, 2017 19:00

thefourtheye force-pushed the child-process-signal-validation branch from 9c46e6b to 227bfd5 Compare February 13, 2017 16:12

cjihrig reviewed Feb 13, 2017

View reviewed changes

cjihrig requested changes Feb 13, 2017

View reviewed changes

cjihrig approved these changes Feb 24, 2017

View reviewed changes

thefourtheye mentioned this pull request Mar 21, 2017

child_process: support numeric signal argument to kill. #9651

Closed

4 tasks

thefourtheye force-pushed the child-process-signal-validation branch from 3956b2e to 174a984 Compare April 1, 2017 08:56

addaleax approved these changes Apr 1, 2017

View reviewed changes

thefourtheye removed the blocked PRs that are blocked by other issues or PRs. label Apr 2, 2017

addaleax added this to the 8.0.0 milestone Apr 2, 2017

thefourtheye closed this Apr 4, 2017

thefourtheye deleted the child-process-signal-validation branch April 4, 2017 07:36

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

richardlau mentioned this pull request Jul 16, 2017

n.kill(19) throws error #9519

Closed

ehmicky mentioned this pull request May 2, 2024

Make signal names case-sensitive sindresorhus/execa#1013

Closed

child_process: improve killSignal validations by thefourtheye · Pull Request #10423 · nodejs/node · GitHub

Uh oh!

Uh oh!

child_process: improve killSignal validations - #10423

child_process: improve killSignal validations#10423thefourtheye wants to merge 1 commit intonodejs:masternodejs/node:masterfrom thefourtheye:child-process-signal-validationthefourtheye/io.js:child-process-signal-validationCopy head branch name to clipboard

Conversation

thefourtheye commented Dec 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Dec 23, 2016

Uh oh!

thefourtheye commented Dec 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Dec 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Jan 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 11, 2017

Uh oh!

thefourtheye commented Feb 12, 2017

Uh oh!

Choose a reason for hiding this comment

child_process: improve killSignal validations#10423
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:child-process-signal-validation

mscdex Dec 26, 2016 •
edited
Loading

addaleax commented Apr 2, 2017 •
edited
Loading