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 Only
    assert.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 c34f2a0 Compare March 12, 2017 16:44

Trott force-pushed the index-check branch from 1aba55b to c34f2a0 Compare 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 2e4b7ae Compare 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 f28a356 Compare 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

test: add coverage for child_process bounds check #11800

test: add coverage for child_process bounds check #11800

Trott commented Mar 11, 2017

cjihrig left a comment

Trott commented Mar 12, 2017

Trott commented Mar 12, 2017

Trott commented Mar 14, 2017

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017 •

edited

Loading

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017

MylesBorins commented Apr 18, 2017 •

edited

Loading

test: add coverage for child_process bounds check #11800

test: add coverage for child_process bounds check #11800

Conversation

Trott commented Mar 11, 2017

Checklist

Affected core subsystem(s)

cjihrig left a comment

Choose a reason for hiding this comment

Trott commented Mar 12, 2017

Trott commented Mar 12, 2017

Trott commented Mar 14, 2017

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017 • edited Loading

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017

Trott commented Mar 17, 2017

MylesBorins commented Apr 18, 2017 • edited Loading

Trott commented Mar 17, 2017 •

edited

Loading

MylesBorins commented Apr 18, 2017 •

edited

Loading