Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

child_process: validate fork/execFile arguments #7399

Closed
wants to merge 3 commits into from
Closed

child_process: validate fork/execFile arguments #7399

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 24, 2016

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 Trott added the child_process Issues and PRs related to the child_process subsystem. label Jun 24, 2016
@Trott Trott mentioned this pull request Jun 24, 2016
Closed
@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 24, 2016
@Trott Trott added this to the 7.0.0 milestone Jun 24, 2016
@Trott
Copy link
Member Author

Trott commented Jun 25, 2016

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

@Trott
Copy link
Member Author

Trott commented Jun 25, 2016

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

bnoordhuis
bnoordhuis reviewed Jun 26, 2016
View reviewed changes
lib/child_process.js
var options = {};
var args = [];
var pos = 1;
if (Array.isArray(arguments[pos])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bnoordhuis
Copy link
Member

Mostly LGTM, I think.

@jasnell
Copy link
Member

jasnell commented Jun 27, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Jun 27, 2016

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
Copy link
Member

d07430a LGTM.

@Trott
Copy link
Member Author

Trott commented Jun 28, 2016

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

@rvagg rvagg mentioned this pull request Jun 29, 2016
Closed
@rvagg
Copy link
Member

rvagg commented Jun 30, 2016

no objections from today's CTC meeting for this

@rvagg rvagg removed the ctc-agenda label Jun 30, 2016
Trott added a commit to Trott/io.js that referenced this pull request Jun 30, 2016
@Trott
child_process: add fork/execFile arg validation
0548e5d 
Validate fork/execFile arguments.

Fixes: nodejs#2681
Refs: nodejs#4508
PR-URL: nodejs#7399
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 30, 2016
@ChuckLangford @Trott
test: test execFile/fork arg validation
99cfd53 
Fixes: nodejs#2681
Refs: nodejs#4508
PR-URL: nodejs#7399
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jun 30, 2016

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

@Trott Trott closed this Jun 30, 2016
@ChuckLangford
Copy link
Contributor

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

@jasnell jasnell mentioned this pull request Oct 14, 2016
Closed
jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016
@jasnell
2016-10-25, Version 7.0.0 (Current)
c1e20df 
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).
jasnell added a commit that referenced this pull request Oct 25, 2016
@jasnell
2016-10-25, Version 7.0.0 (Current)
362fe01 
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
jasnell added a commit that referenced this pull request Oct 25, 2016
@jasnell
2016-10-25, Version 7.0.0 (Current)
1e4fafc 
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 25, 2016
@imyller
2016-10-25, Version 7.0.0 (Current)
7aae498 
    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 <ilkka.myller@nodefield.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@Trott Trott deleted the arg-val branch January 13, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

child_process: execFile and fork arg parsing ambiguity
5 participants
@Trott @bnoordhuis @jasnell @rvagg @ChuckLangford