child_process: validate fork/execFile arguments #7399

Trott · 2016-06-24T05:16:00Z

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

I took the tests that @ChuckLangford wrote in #4508 and implemented code such that they pass. This fixes #2681.

Trott · 2016-06-25T05:24:26Z

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

Trott · 2016-06-25T05:55:09Z

CI is green. /cc @jasnell @bnoordhuis @cjihrig

bnoordhuis · 2016-06-26T10:11:37Z

lib/child_process.js

+ var options = {};
+ var args = [];
+ var pos = 1;
+ if (Array.isArray(arguments[pos])) {


Can you check that pos < arguments.length? I believe V8 still deoptimizes on out-of-bounds arguments access.

Happy to make the change. A couple of questions, though, mostly to make sure I'm understanding things correctly:

The code I'm replacing accessed arguments[1] without any bounds check, so it was already not being optimized. Or was there something else going on there that made that code optimizable?

This isn't going to be something that's benchmark-able because each fork() call is going to spin up a completely separate Node.js instance and any performance improvement in the single call to fork() is going to be lost in the overhead of launching the separate instance. Or am I wrong and this may be reasonably benchmark-able perhaps using some trick I'm not thinking of?

The code I'm replacing accessed arguments[1] without any bounds check, so it was already not being optimized.

Yes, that's quite possibly the case.

This isn't going to be something that's benchmark-able because each fork() call is going to spin up a completely separate Node.js instance

That's right but it would be good for consistency. Elsewhere in lib/child_process.js we're quite diligent about checking arguments.length. Also, it makes --trace_deopt less noisy, which is never a bad thing.

OK, added in the checks for pos < arguments.length.

bnoordhuis · 2016-06-26T10:12:59Z

Mostly LGTM, I think.

jasnell · 2016-06-27T15:50:01Z

LGTM

Fixes: nodejs#2681 Refs: nodejs#4508

Trott · 2016-06-27T23:57:20Z

Addressed optimization nits from @bnoordhuis, rebased, force pushed.

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

And, because this is semver-major, adding the ctc-agenda label.

bnoordhuis · 2016-06-28T07:39:58Z

d07430a LGTM.

Trott · 2016-06-28T18:18:54Z

SInce it's semver-major: @nodejs/ctc (Will be on agenda for tomorrow's meeting.)

rvagg · 2016-06-30T06:48:40Z

no objections from today's CTC meeting for this

Validate fork/execFile arguments. Fixes: nodejs#2681 Refs: nodejs#4508 PR-URL: nodejs#7399 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixes: nodejs#2681 Refs: nodejs#4508 PR-URL: nodejs#7399 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2016-06-30T23:21:37Z

Landed in 0548e5d and 99cfd53. Thanks, @ChuckLangford!

ChuckLangford · 2016-07-01T00:36:33Z

This has been a fantastic learning experience for me. Thanks @Trott

Notable Changes: * Buffer * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs#8946). * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs#8169). * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs#7079). * Child Process * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs#7399). * Cluster * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs#3747). * Deps * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs#8317), [nodejs#8852](nodejs#8852), [nodejs#9253](nodejs#9253). * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs#8808). * File System * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs#7897). * Intl * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs#8908). * Promises * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs#8217). * Punycode * The `punycode` module has been deprecated [nodejs#7941](nodejs#7941). * URL * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs#7448).

Notable Changes: * Buffer * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946). * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169). * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079). * Child Process * The fork and execFile methods now have stronger argument validation [#7399](#7399). * Cluster * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747). * Deps * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253). * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808). * File System * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897). * Intl * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908). * Promises * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217). * Punycode * The `punycode` module has been deprecated [#7941](#7941). * URL * An Experimental WHATWG URL Parser has been introduced [#7448](#7448). PR-URL: #9099

Notable Changes: * Buffer * Passing invalid input to Buffer.byteLength will now throw an error [#8946](nodejs/node#8946). * Calling Buffer without new is now deprecated and will emit a process warning [#8169](nodejs/node#8169). * Passing a negative number to allocUnsafe will now throw an error [#7079](nodejs/node#7079). * Child Process * The fork and execFile methods now have stronger argument validation [#7399](nodejs/node#7399). * Cluster * The worker.suicide method is deprecated and will emit a process warning [#3747](nodejs/node#3747). * Deps * V8 has been updated to 5.4.500.36 [#8317](nodejs/node#8317), [#8852](nodejs/node#8852), [#9253](nodejs/node#9253). * NODE_MODULE_VERSION has been updated to 51 [#8808](nodejs/node#8808). * File System * A process warning is emitted if a callback is not passed to async file system methods [#7897](nodejs/node#7897). * Intl * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](nodejs/node#8908). * Promises * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](nodejs/node#8217). * Punycode * The `punycode` module has been deprecated [#7941](nodejs/node#7941). * URL * An Experimental WHATWG URL Parser has been introduced [#7448](nodejs/node#7448). Signed-off-by: Ilkka Myller <[email protected]>

Trott added the child_process Issues and PRs related to the child_process subsystem. label Jun 24, 2016

Trott mentioned this pull request Jun 24, 2016

child_process: execFile and fork arg parsing ambiguity #2681

Closed

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 24, 2016

Trott added this to the 7.0.0 milestone Jun 24, 2016

bnoordhuis reviewed Jun 26, 2016
View reviewed changes

Trott force-pushed the arg-val branch from 073435f to a4842c2 Compare June 26, 2016 15:48

Trott and others added 3 commits June 27, 2016 16:48

child_process: validate fork/execFile arguments

359a79d

Fixes: nodejs#2681 Refs: nodejs#4508

test: validate execFile & fork arguments

7c77fd4

squash: optimization nits

d07430a

Trott force-pushed the arg-val branch from a4842c2 to d07430a Compare June 27, 2016 23:55

Trott added the ctc-agenda label Jun 27, 2016

rvagg mentioned this pull request Jun 29, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-06-29 #7474

Closed

rvagg removed the ctc-agenda label Jun 30, 2016

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 30, 2016

test: test execFile/fork arg validation

99cfd53

Fixes: nodejs#2681 Refs: nodejs#4508 PR-URL: nodejs#7399 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott closed this Jun 30, 2016

jasnell mentioned this pull request Oct 14, 2016

v7.0.0 Proposal #9099

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

Trott deleted the arg-val branch January 13, 2022 22:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: validate fork/execFile arguments #7399

child_process: validate fork/execFile arguments #7399

Trott commented Jun 24, 2016

Trott commented Jun 25, 2016

Trott commented Jun 25, 2016

bnoordhuis Jun 26, 2016

Trott Jun 26, 2016

bnoordhuis Jun 27, 2016

Trott Jun 27, 2016

bnoordhuis commented Jun 26, 2016

jasnell commented Jun 27, 2016

Trott commented Jun 27, 2016

bnoordhuis commented Jun 28, 2016

Trott commented Jun 28, 2016

rvagg commented Jun 30, 2016

Trott commented Jun 30, 2016

ChuckLangford commented Jul 1, 2016

child_process: validate fork/execFile arguments #7399

child_process: validate fork/execFile arguments #7399

Conversation

Trott commented Jun 24, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Jun 25, 2016

Trott commented Jun 25, 2016

bnoordhuis Jun 26, 2016

Choose a reason for hiding this comment

Trott Jun 26, 2016

Choose a reason for hiding this comment

bnoordhuis Jun 27, 2016

Choose a reason for hiding this comment

Trott Jun 27, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jun 26, 2016

jasnell commented Jun 27, 2016

Trott commented Jun 27, 2016

bnoordhuis commented Jun 28, 2016

Trott commented Jun 28, 2016

rvagg commented Jun 30, 2016

Trott commented Jun 30, 2016

ChuckLangford commented Jul 1, 2016