child_process: internal cleanup and input validation improvements #12348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjihrig
added
child_process
nodejs-github-bot
added
the
child_process
|
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
reviewed
Apr 11, 2017
lib/internal/child_process.js
Outdated
| @@ -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) => { | |||
There was a problem hiding this comment.
What about just replacing map() with a for loop?
Trott
reviewed
Apr 11, 2017
lib/internal/child_process.js
Outdated
| @@ -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; | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
@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.
jasnell
approved these changes
Apr 12, 2017
cjihrig
force-pushed
the
cp
branch
2 times, most recently
from
cf6b022
to
892cab3
Compare
|
@mscdex updated based on your feedback. Good catch on the |
jseijas
approved these changes
Apr 16, 2017
santigimeno
approved these changes
Apr 16, 2017
|
|
||
| if (exitCode < 0) { | ||
| if (self.spawnfile) | ||
| err.path = self.spawnfile; | ||
| var syscall = this.spawnfile ? 'spawn ' + this.spawnfile : 'spawn'; |
There was a problem hiding this comment.
I thought I saw that const could trigger deopts in functions, cc/ @mscdex
There was a problem hiding this comment.
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).
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]>
jasnell
added a commit
to jasnell/node
that referenced
this pull request
May 29, 2017
* **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).
jasnell
added a commit
that referenced
this pull request
May 30, 2017
* **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).
jasnell
added a commit
that referenced
this pull request
May 30, 2017
* **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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is split up into three commits:
self = thisassignments and replaces some chained array iteration functions with one simpleforloop.ChildProcesstest in preparation for the third commit.ChildProcesswasn'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), orvcbuild test(Windows) passesAffected core subsystem(s)
child_process, test