child_process: internal cleanup and input validation improvements #12348

cjihrig · 2017-04-11T20:46:32Z

This PR is split up into three commits:

Cleanup of the internal child_process source file. Removes some self = this assignments and replaces some chained array iteration functions with one simple for loop.
Cleanup of the ChildProcess test in preparation for the third commit.
Add input validation. ChildProcess wasn't always an official public API. When we made it official, we didn't add any input validation. It was pretty easy to pass bad input to it. Got the idea from (Refs) Type confusion bugs in process_wrap.cc. #12177.

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, test

mscdex · 2017-04-11T21:35:22Z

As far as the error messages go, I think we are (at least at one time) trying to use double quotes around parameter/property names (at least in new error messages)?

mscdex · 2017-04-11T21:37:38Z

lib/internal/child_process.js

@@ -345,7 +359,7 @@ ChildProcess.prototype.spawn = function(options) {
  this.stderr = stdio.length >= 3 && stdio[2].socket !== undefined ?
      stdio[2].socket : null;

-  this.stdio = stdio.map(function(stdio) {
+  this.stdio = stdio.map((stdio) => {


What about just replacing map() with a for loop?

Trott · 2017-04-11T22:35:06Z

lib/internal/child_process.js

@@ -182,30 +180,30 @@ function ChildProcess() {
    //
    // - spawn failures are reported with exitCode < 0
    //
-    var syscall = self.spawnfile ? 'spawn ' + self.spawnfile : 'spawn';
+    var syscall = this.spawnfile ? 'spawn ' + this.spawnfile : 'spawn';
    var err = (exitCode < 0) ? errnoException(exitCode, syscall) : null;


Does this and the other exitCode < 0 check below (line 201 or 199 depending on whether you're looking at it before the change or after) make the code < 0 check in lib/child_process.js unnecessary? (The code < 0 check is the only part of lib/child_process.js that doesn't currently get exercised in tests. It is run, but is always false so the true condition is untested.)

@Trott it looks that way. I want to investigate a little more because that code is a tangled mess. I should either be able to add a test or remove the code later today.

cjihrig · 2017-04-12T15:05:25Z

@mscdex updated based on your feedback. Good catch on the map().

santigimeno

LGTM with a question.

santigimeno · 2017-04-16T20:56:31Z

lib/internal/child_process.js


    if (exitCode < 0) {
-      if (self.spawnfile)
-        err.path = self.spawnfile;
+      var syscall = this.spawnfile ? 'spawn ' + this.spawnfile : 'spawn';


can this be a const too?

I thought I saw that const could trigger deopts in functions, cc/ @mscdex

It can but it's not always immediately obvious if it actually will or not without checking during runtime (e.g. by adding --trace-opt and grepping for 'aborted' and/or 'disabled' in the resulting output).

cjihrig · 2017-04-17T20:01:42Z

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

This commit removes self = this style assignments and replaces some array iteration functions with a simpler for loop. PR-URL: nodejs#12348 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

PR-URL: nodejs#12348 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

This commit improves input validation for the ChildProcess internals. It became officially supported API a while back, but never had any validation. Refs: nodejs#12177 PR-URL: nodejs#12348 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

* **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).

cjihrig added child_process Issues and PRs related to the child_process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 11, 2017

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Apr 11, 2017

mscdex reviewed Apr 11, 2017

View reviewed changes

Trott reviewed Apr 11, 2017

View reviewed changes

jasnell approved these changes Apr 12, 2017

View reviewed changes

cjihrig force-pushed the cp branch 2 times, most recently from cf6b022 to 892cab3 Compare April 12, 2017 14:59

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

jseijas approved these changes Apr 16, 2017

View reviewed changes

santigimeno approved these changes Apr 16, 2017

View reviewed changes

cjihrig added 3 commits April 17, 2017 16:18

child_process: minor cleanup of internals

a9111f9

This commit removes self = this style assignments and replaces some array iteration functions with a simpler for loop. PR-URL: nodejs#12348 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

test: cleanup test-child-process-constructor.js

59c6230

PR-URL: nodejs#12348 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

cjihrig force-pushed the cp branch from 892cab3 to 97a7728 Compare April 17, 2017 20:25

cjihrig merged commit 97a7728 into nodejs:master Apr 17, 2017

cjihrig deleted the cp branch April 17, 2017 20:26

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

abenhamdine mentioned this pull request May 31, 2017

--max-executable-size not recognized after upgrade 7.10.0 => 8.0.0 #13341

Closed

apapirovski mentioned this pull request Apr 14, 2018

Type confusion bugs in process_wrap.cc. #12177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: internal cleanup and input validation improvements #12348

child_process: internal cleanup and input validation improvements #12348

cjihrig commented Apr 11, 2017

mscdex commented Apr 11, 2017 •

edited

Loading

mscdex Apr 11, 2017

Trott Apr 11, 2017

cjihrig Apr 12, 2017

cjihrig commented Apr 12, 2017

santigimeno left a comment

santigimeno Apr 16, 2017

gibfahn Apr 16, 2017

mscdex Apr 17, 2017 •

edited

Loading

cjihrig commented Apr 17, 2017

child_process: internal cleanup and input validation improvements #12348

child_process: internal cleanup and input validation improvements #12348

Conversation

cjihrig commented Apr 11, 2017

Checklist

Affected core subsystem(s)

mscdex commented Apr 11, 2017 • edited Loading

mscdex Apr 11, 2017

Choose a reason for hiding this comment

Trott Apr 11, 2017

Choose a reason for hiding this comment

cjihrig Apr 12, 2017

Choose a reason for hiding this comment

cjihrig commented Apr 12, 2017

santigimeno left a comment

Choose a reason for hiding this comment

santigimeno Apr 16, 2017

Choose a reason for hiding this comment

gibfahn Apr 16, 2017

Choose a reason for hiding this comment

mscdex Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

cjihrig commented Apr 17, 2017

mscdex commented Apr 11, 2017 •

edited

Loading

mscdex Apr 17, 2017 •

edited

Loading