test: add coverage for child_process bounds check#11800

Trott · 2017-03-11T06:44:43Z

Make sure that monkey-patching process.execArgv doesn't cause
child_process to incorrectly munge execArgv in fork().

This basically is adding coverage for an index > 0 check (see comment
in this commit). Previously, that condition was never false in any of
the tests.

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)

test child_process

cjihrig

LGTM. I think the link and ESLint comments could be dropped though.

Trott · 2017-03-12T16:45:44Z

Link and ESLint comments removed.

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

Trott · 2017-03-12T20:50:15Z

CI again with adjustment for Windows CLI: https://ci.nodejs.org/job/node-test-pull-request/6815/

Trott · 2017-03-14T05:50:42Z

Test still failing on Windows. Marking in progress until that gets sorted...

Trott · 2017-03-17T00:20:05Z

Sample failure on Windows: https://ci.nodejs.org/job/node-test-binary-windows/7064/RUN_SUBSET=1,VS_VERSION=vs2015,label=win2012r2/console

not ok 31 parallel/test-cli-eval --- duration_ms: 3.517 severity: fail stack: |- assert.js:81 throw new assert.AssertionError({ ^ AssertionError: '' === '42\n' at child.exec.common.mustCall (c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2012r2\test\parallel\test-cli-eval.js:154:14) at c:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vs2015\label\win2012r2\test\common.js:473:15 at ChildProcess.exithandler (child_process.js:238:7) at emitTwo (events.js:127:13) at ChildProcess.emit (events.js:215:7) at maybeClose (internal/child_process.js:893:16) at Socket.<anonymous> (internal/child_process.js:335:11) at emitOne (events.js:117:13) at Socket.emit (events.js:212:7) at Pipe._handle.close [as _onclose] (net.js:534:12)

Trott · 2017-03-17T00:23:04Z

So, Windows is not getting the output from console.log(42) but everything else is...

Perhaps related to nodejs/node-v0.x-archive#3871?

Maybe this is some "this OS blocks stdio but that one doesn't" thing? Maybe I'm reaching here...

Relevant code:

child.exec(`${nodejs} -e "process.execArgv = ['-e', 'console.log(42)', 'thirdArg']; require('child_process').fork('${emptyFile}')"`,common.mustCall((err,stdout,stderr)=>{assert.ifError(err);assert.strictEqual(stdout,'42\n');// <- Raises AssertionError on Windows Onlyassert.strictEqual(stderr,'');}));

/cc @nodejs/platform-windows

Make sure that monkey-patching process.execArgv doesn't cause child_process to incorrectly munge execArgv in fork(). This basically is adding coverage for an `index > 0` check (see Refs). Previously, that condition was never false in any of the tests. Refs: https://github.com/nodejs/node/blob/c67207731f16a78f6cae90e49c53b10728241ecf/lib/child_process.js#L76

Trott · 2017-03-17T19:59:26Z

Trying to squash everything into a single line to see if that makes a difference (which makes sense if so). New CI: https://ci.nodejs.org/job/node-test-pull-request/6909/

Trott · 2017-03-17T20:49:50Z

OMG, that did it! Landing soon...

Make sure that monkey-patching process.execArgv doesn't cause child_process to incorrectly munge execArgv in fork(). This basically is adding coverage for an `index > 0` check (see Refs). Previously, that condition was never false in any of the tests. PR-URL: nodejs#11800 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Refs: https://github.com/nodejs/node/blob/c67207731f16a78f6cae90e49c53b10728241ecf/lib/child_process.js#L76

Trott · 2017-03-17T21:00:56Z

Landed in df69d95

Make sure that monkey-patching process.execArgv doesn't cause child_process to incorrectly munge execArgv in fork(). This basically is adding coverage for an `index > 0` check (see Refs). Previously, that condition was never false in any of the tests. PR-URL: nodejs#11800 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Refs: https://github.com/nodejs/node/blob/c67207731f16a78f6cae90e49c53b10728241ecf/lib/child_process.js#L76

MylesBorins · 2017-04-18T17:06:11Z

not landing cleanly on v6.x feel free to backport

edit: nvm got it

Make sure that monkey-patching process.execArgv doesn't cause child_process to incorrectly munge execArgv in fork(). This basically is adding coverage for an `index > 0` check (see Refs). Previously, that condition was never false in any of the tests. PR-URL: #11800 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Refs: https://github.com/nodejs/node/blob/c67207731f16a78f6cae90e49c53b10728241ecf/lib/child_process.js#L76

Make sure that monkey-patching process.execArgv doesn't cause child_process to incorrectly munge execArgv in fork(). This basically is adding coverage for an `index > 0` check (see Refs). Previously, that condition was never false in any of the tests. PR-URL: nodejs/node#11800 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Refs: https://github.com/nodejs/node/blob/c67207731f16a78f6cae90e49c53b10728241ecf/lib/child_process.js#L76

Trott added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Mar 11, 2017

jasnell approved these changes Mar 12, 2017
View reviewed changes

cjihrig approved these changes Mar 12, 2017
View reviewed changes

Trott force-pushed the index-check branch from e9daa34 to c34f2a0Compare March 12, 2017 16:44

Trott force-pushed the index-check branch from 1aba55b to c34f2a0Compare March 12, 2017 21:28

gibfahn approved these changes Mar 13, 2017
View reviewed changes

hiroppy approved these changes Mar 13, 2017
View reviewed changes

Trott force-pushed the index-check branch from c34f2a0 to 2e4b7aeCompare March 14, 2017 04:22

Trott added the wip Issues and PRs that are still a work in progress. label Mar 14, 2017

Trott added 3 commits March 17, 2017 12:56

squash: hopeful fix for Windows
c2a2dc2

squash: make it one line and see if that fixes it on Windows
f28a356

Trott force-pushed the index-check branch from 2e4b7ae to f28a356Compare March 17, 2017 19:58

Trott removed the wip Issues and PRs that are still a work in progress. label Mar 17, 2017

Trott closed this Mar 17, 2017

italoacasas mentioned this pull request Mar 20, 2017
v7.7.4 Release Proposal #11941
Merged

MylesBorins added the dont-land-on-v6.x label Apr 18, 2017

MylesBorins added land-on-v6.x and removed dont-land-on-v6.x labels Apr 18, 2017

MylesBorins mentioned this pull request Apr 19, 2017
v6.10.3 proposal #12498
Merged

Trott deleted the index-check branch January 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

test: add coverage for child_process bounds check#11800

test: add coverage for child_process bounds check #11800

Uh oh!

Trott commented Mar 11, 2017

Uh oh!

cjihrig left a comment

Uh oh!

Trott commented Mar 12, 2017

Uh oh!

Trott commented Mar 12, 2017

Uh oh!

Trott commented Mar 14, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017•
edited
Loading

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

MylesBorins commented Apr 18, 2017•
edited
Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

test: add coverage for child_process bounds check#11800

test: add coverage for child_process bounds check #11800

Uh oh!

Conversation

Trott commented Mar 11, 2017

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Mar 12, 2017

Uh oh!

Trott commented Mar 12, 2017

Uh oh!

Trott commented Mar 14, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

Trott commented Mar 17, 2017

Uh oh!

MylesBorins commented Apr 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Trott commented Mar 17, 2017•
edited
Loading

MylesBorins commented Apr 18, 2017•
edited
Loading