child_process: spawn() and spawnSync() validation#8312

cjihrig · 2016-08-28T18:19:22Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

child_process, src

Description of change

This PR is intended to fix#8096. The first commit prevents the abort from #8096 by verifying that the handle type is correct before trying to close it. This commit is semver-patch.

This leaves an uninformative EINVAL error being thrown due to the bad input. So, the second commit in this PR addresses that by producing more helpful errors in the JavaScript layer. In doing so, it better unifies the spawn() and spawnSync() input checking (which also nicely bubbles up to the other child_process methods). Additional spawnSync() specific checks are also provided. This commit is semver-major due to the changes in error messages.

The third commit removes redundant C++ type checks that are now done in the JS layer.

cc: @bnoordhuis, @targos, and @retrohacker from #8096

Fixes: #8096
Fixes: #8539

Fishrock123 · 2016-08-28T18:39:09Z

lib/child_process.js

Maybe a helper would be better?

Sounds like string, null and undefined are allowed.
On Aug 29, 2016 12:09 AM, "Jeremiah Senkpiel" [email protected]
wrote:
In lib/child_process.js
#8312 (comment):
@@ -329,6 +329,49 @@ function normalizeSpawnArguments(file /, args, options/){
else if (options === null || typeof options !== 'object')
throw new TypeError('"options" argument must be an object');
// Validate the cwd, if present.
if (options.cwd !== undefined &&
options.cwd !== null &&
!typeof options.cwd === 'string')
Maybe a helper would be better? won't !typeof options.cwd === 'string'
cover all these 3 cases?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/8312/files/b4a6558fcbcf6cde50ff29af2d3487896ad91c68#r76536916,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABjB841u9wl3B9jlsDIb7RdsDKU0iWTcks5qkdXdgaJpZM4Ju-4I
.

Do you mean another function like isOptionalString() (where the optional part covers null and undefined)?

!typeof options.cwd === 'string' is always false, it should be !(typeof options.cwd === 'string') or (better) typeof options.cwd !== 'string'.

cjihrig · 2016-09-01T18:49:38Z

@bnoordhuis any opinion on the approach here?

bnoordhuis · 2016-09-02T08:42:47Z

The options.x !== undefined && options.x !== null clauses could be shortened to options.x != null and I'd probably put braces around the body of multi-line if statements.

Moving the validation logic to JS land basically LGTM though.

bnoordhuis · 2016-10-12T09:13:42Z

@cjihrig Ping.

cjihrig · 2016-10-12T13:16:20Z

I plan on getting back to this next week. Sorry for the delay.

cjihrig · 2016-10-18T19:12:08Z

@bnoordhuis updated, PTAL.

cjihrig · 2016-10-26T13:51:04Z

@bnoordhuis ping

cjihrig · 2016-11-15T20:46:02Z

ping @nodejs/collaborators

sam-github · 2016-11-16T01:02:30Z

I'll look at this, but because the test was added in the same commit as the additional checks, its hard to evaluate what is different, i.e., what the behaviour used to be. If the test was added, then the commit that changed the behaviour changed the test... it would be easy to understand the change.

sam-github · 2016-11-16T15:01:43Z

Many thumbs up for solid testing of these APIs, and better input validation.

cwd, detached, uid, gid are all implemented in normalizeSpawnArguments(), why aren't the tests in test/parallel/test-child-process-spawn-typeerror.js, instead of a new file?

Its not clear what tests should go where, and its also very difficult to determine what kind of test coverage there is, not in the line sense, but in the sense of: are the tests against spawnSync here valid for any of the other Sync calls? Any other spawn calls?

Its painful, but maybe the tests should be factored so they can be applied to all the child_process functions that use the same normalize? Or perhaps the tests should all just be in one file, with a comment in the test saying that even though it appears that only one API is tested, the underlying code is the same, so its hitting every API?

Unrelated to this PR, but I'm pretty disappointed that the arg permutation tests in test-child-process-spawn-typeerror.js were never extended to include *Sync, since arg permutations was such a rich source of bugs and inconsistencies, and I'm poking around, but I can't see any tests for their arg permutations.

But even though that problem is unrelated, its why I suggest that having the tests be organized in a way that its (EDIT) "easy" for a reader to verify the coverage scenarios is important.

sam-github · 2016-11-16T15:19:38Z

lib/child_process.js

Does this have to be, literally, boolean? I would expect it to have to be truthy/falsy

It could probably be coerced to a Boolean, but the extra strictness is good IMO. Not that it matters much, but windowsVerbatimArguments isn't documented anyway, IIRC.

I agree, its internal-only, so maybe not so important, and better to start strict. I don't agree strictness is generally good in this case, btw. It creates a bizarre API wherein truthy is sometimes true/false, sometimes null/undefined/0/"" are also false, sometimes not, and sometimes function/[]/{}/non-zero/"strings" are also true, and sometimes null or undefined are a "third way", neither true or false but some kind of other behaviour. Ouch. Anyhow, as a semi-private (but used by modules on npmjs.org none-the-less) API, I'm OK with this restriction.

cjihrig · 2016-11-16T15:35:53Z

cwd, detached, uid, gid are all implemented in normalizeSpawnArguments(), why aren't the tests in test/parallel/test-child-process-spawn-typeerror.js, instead of a new file?

I just did that because test-child-process-spawn-typeerror validates the types of the arguments passed in, but not specific properties (is options an object, but not specific values in options).

Its not clear what tests should go where, and its also very difficult to determine what kind of test coverage there is, not in the line sense, but in the sense of: are the tests against spawnSync here valid for any of the other Sync calls?

I agree that the tests are kind of all over the place. This PR is a step towards consolidating the behavior of spawn() and spawnSync(). Consolidating the tests would be a good followup.

Its painful, but maybe the tests should be factored so they can be applied to all the child_process functions that use the same normalize?

I think just about everything goes through normalizeSpawnArguments(), which is convenient.

sam-github · 2016-11-16T17:20:38Z

I just did that because test-child-process-spawn-typeerror validates the types of the arguments passed in, but not specific properties (is options an object, but not specific values in options).

The tests you add seemed like an obvious candidate to be in a "spawn typeerror" test, but I understand the distinction you draw above.

If you do draw that distinction, my request is that the name of the tests (and a comment at their top) clearly communicate it: the names/comments should indicate what that test is for, so that we don't get diffusion and confusion of tests over time.

For example:

test-child-process-spawn-typeerror could perhaps be test-child-process-argument-permutations (with an XXX comment pointing out that the *Sync APIs are currently untested for permutation).
test-child-process-spawnsync-validation-errors.js could be test-child-process-validation-errors.js (with a comment pointing out that while it doesn't explicitly test all the child_process APIs, that the underlying code is currently the same so they all inherit the same behaviour - or else just explicitly test them all so the tests are robust in the face of future refactors).

I suggest a rename like this because it would be a disaster if someone created a test-child-process-spawn-validation-errors.js (note the lack of sync) that had different validations in it than the spawnsync version you just made... the validations need to be in one place, so that the tests enforce consistent behaviour across the child_process API. For example, uid shouldn't be asserted to be integral for one API, but just a Number for another, and to be any Number except NaN for yet a third (which was the kind of situation we used to have).

sam-github · 2016-11-16T17:22:06Z

test/parallel/test-child-process-spawnsync-validation-errors.js

Assert NaN is not valid. Ditto below.

sam-github · 2016-11-16T17:23:07Z

test/parallel/test-child-process-spawnsync-validation-errors.js

0 and 1 are usually acceptable as truthy values, I think they should be accepted here.

I'd personally go for strict.

bnoordhuis

LGTM. Good test coverage.

bnoordhuis · 2016-12-22T09:24:10Z

test/parallel/test-child-process-spawnsync-validation-errors.js

I'd personally go for strict.

thefourtheye · 2016-12-22T12:03:49Z

lib/child_process.js

Nit: If the input is an empty string, the error message might not be exactly appropriate.

Updated to specify "non-empty string"

cjihrig · 2016-12-22T18:06:39Z

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

New CI because Windows: https://ci.nodejs.org/job/node-test-pull-request/5545/

cjihrig · 2016-12-23T00:10:07Z

CI is all green.

This commit verifies that the child process handle is of the correct type before trying to close it in CloseHandlesAndDeleteLoop(). This catches the case where input validation failed, and the child process was never actually spawned. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

This commit applies stricter input validation in normalizeSpawnArguments(), which is run by all of the child_process methods. Additional checks are added for spawnSync() specific inputs. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

This commit removes C++ checks from spawn() and spawnSync() that are duplicates of the JavaScript type checking. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

cjihrig · 2016-12-25T17:54:30Z

Landed in b374ee8 (semver patch), fc7b0dd, and 45c9ca7.

MylesBorins · 2017-01-16T16:57:18Z

@Fishrock123 the bot should likely not be adding lts labels to anything labelled semver major

Fishrock123 · 2017-01-16T17:05:08Z

@MylesBorins see nodejs/github-bot#105

This commit verifies that the child process handle is of the correct type before trying to close it in CloseHandlesAndDeleteLoop(). This catches the case where input validation failed, and the child process was never actually spawned. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

This commit applies stricter input validation in normalizeSpawnArguments(), which is run by all of the child_process methods. Additional checks are added for spawnSync() specific inputs. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

This commit removes C++ checks from spawn() and spawnSync() that are duplicates of the JavaScript type checking. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

cjihrig added child_process Issues and PRs related to the child_process subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 28, 2016

Fishrock123 reviewed Aug 28, 2016
View reviewed changes

cjihrig mentioned this pull request Sep 15, 2016
child_process.spawnSync is memory unsafe and can be used to dump core #8539
Closed

Trott force-pushed the master branch from b0df363 to c5ce7f4Compare September 21, 2016 00:09

cjihrig force-pushed the spawnsync branch from b4a6558 to 7aede77Compare October 18, 2016 14:55

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:02

cjihrig force-pushed the spawnsync branch 2 times, most recently from af5c2f5 to 76d7181Compare October 18, 2016 17:17

rvagg force-pushed the master branch from 6f558a7 to 0e6750dCompare October 18, 2016 17:23

cjihrig force-pushed the spawnsync branch 3 times, most recently from 50f599e to 146129bCompare October 18, 2016 18:58

cjihrig changed the title ~~WIP: child_process: spawn() and spawnSync() validation~~child_process: spawn() and spawnSync() validationOct 18, 2016

sam-github reviewed Nov 16, 2016
View reviewed changes

test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Copy link
Contributor
sam-githubNov 16, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assert NaN is not valid. Ditto below.

sam-github reviewed Nov 16, 2016
View reviewed changes

bnoordhuis approved these changes Dec 22, 2016
View reviewed changes

test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Copy link
Member
bnoordhuisDec 22, 2016
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally go for strict.

thefourtheye reviewed Dec 22, 2016
View reviewed changes

cjihrig force-pushed the spawnsync branch from fd3750c to 01834d8Compare December 22, 2016 18:05

nodejs-github-bot added the lts-watch-v6.x label Dec 22, 2016

cjihrig force-pushed the spawnsync branch from 01834d8 to a2f31d3Compare December 22, 2016 23:33

nodejs-github-bot added the lts-watch-v6.x label Dec 22, 2016

bnoordhuis mentioned this pull request Dec 23, 2016
child_process: improve killSignal validations #10423
Closed
3 tasks

cjihrig added 3 commits December 25, 2016 12:46

src: remove redundant spawn/spawnSync type checks
45c9ca7
This commit removes C++ checks from spawn() and spawnSync() that are duplicates of the JavaScript type checking. Fixes: nodejs#8096Fixes: nodejs#8539 Refs: nodejs#9722 PR-URL: nodejs#8312 Reviewed-By: Ben Noordhuis <[email protected]>

cjihrig force-pushed the spawnsync branch from a2f31d3 to 45c9ca7Compare December 25, 2016 17:52

nodejs-github-bot added the lts-watch-v6.x label Dec 25, 2016

cjihrig merged commit 45c9ca7 into nodejs:masterDec 25, 2016

cjihrig deleted the spawnsync branch December 25, 2016 17:54

MylesBorins added dont-land-on-v4.x and removed lts-watch-v6.x labels Jan 16, 2017

vsemozhetbyt mentioned this pull request Mar 8, 2017
(v7.x backport) child_process: fix deoptimizing use of arguments #11748
Closed
3 tasks

jasnell mentioned this pull request Apr 4, 2017
8.0.0 Release Proposal #12220
Closed

Uh oh!

child_process: spawn() and spawnSync() validation#8312

child_process: spawn() and spawnSync() validation #8312

Uh oh!

Conversation

cjihrig commented Aug 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123Aug 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 1, 2016

Uh oh!

bnoordhuis commented Sep 2, 2016

Uh oh!

bnoordhuis commented Oct 12, 2016

Uh oh!

cjihrig commented Oct 12, 2016

Uh oh!

cjihrig commented Oct 18, 2016

Uh oh!

cjihrig commented Oct 26, 2016

Uh oh!

cjihrig commented Nov 15, 2016

Uh oh!

sam-github commented Nov 16, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Nov 16, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 16, 2016

Uh oh!

sam-github commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Dec 22, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig commented Dec 23, 2016

Uh oh!

cjihrig commented Dec 25, 2016

Uh oh!

MylesBorins commented Jan 16, 2017

Uh oh!

Fishrock123 commented Jan 16, 2017

Uh oh!

Reviewers

Assignees

Labels

cjihrig commented Aug 28, 2016•
edited
Loading

Fishrock123Aug 28, 2016•
edited
Loading

sam-github commented Nov 16, 2016•
edited
Loading

sam-github commented Nov 16, 2016•
edited
Loading

cjihrig commented Dec 22, 2016•
edited
Loading