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.

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

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

Merged
merged 3 commits into from Dec 25, 2016

Conversation

Projects
None yet
10 participants
@cjihrig
Contributor

cjihrig commented Aug 28, 2016

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

View changes

Show outdated Hide outdated lib/child_process.js Outdated
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 1, 2016

Contributor

@bnoordhuis any opinion on the approach here?

Contributor

cjihrig commented Sep 1, 2016

@bnoordhuis any opinion on the approach here?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 2, 2016

Member

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.

Member

bnoordhuis commented Sep 2, 2016

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

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis
Member

bnoordhuis commented Oct 12, 2016

@cjihrig Ping.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 12, 2016

Contributor

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

Contributor

cjihrig commented Oct 12, 2016

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

@cjihrig cjihrig changed the title from WIP: child_process: spawn() and spawnSync() validation to child_process: spawn() and spawnSync() validation Oct 18, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 18, 2016

Contributor

@bnoordhuis updated, PTAL.

Contributor

cjihrig commented Oct 18, 2016

@bnoordhuis updated, PTAL.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Oct 26, 2016

Contributor
Contributor

cjihrig commented Oct 26, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig
Contributor

cjihrig commented Nov 15, 2016

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 16, 2016

Member

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.

Member

sam-github commented Nov 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 16, 2016

Member

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.

Member

sam-github commented Nov 16, 2016

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.

// Validate windowsVerbatimArguments, if present.
if (options.windowsVerbatimArguments != null &&
typeof options.windowsVerbatimArguments !== 'boolean') {

This comment has been minimized.

@sam-github

sam-github Nov 16, 2016

Member

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

@sam-github

sam-github Nov 16, 2016

Member

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

This comment has been minimized.

@cjihrig

cjihrig Nov 16, 2016

Contributor

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.

@cjihrig

cjihrig Nov 16, 2016

Contributor

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.

This comment has been minimized.

@sam-github

sam-github Nov 16, 2016

Member

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.

@sam-github

sam-github Nov 16, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Nov 16, 2016

Contributor

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.

Contributor

cjihrig commented Nov 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 16, 2016

Member

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).

Member

sam-github commented Nov 16, 2016

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).

Show outdated Hide outdated test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
pass('shell', undefined);
pass('shell', null);
pass('shell', false);
fail('shell', 0, err);

This comment has been minimized.

@sam-github

sam-github Nov 16, 2016

Member

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

@sam-github

sam-github Nov 16, 2016

Member

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

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

I'd personally go for strict.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

I'd personally go for strict.

fail('cwd', true, err);
fail('cwd', false, err);
fail('cwd', [], err);
fail('cwd', {}, err);

This comment has been minimized.

@sam-github

sam-github Nov 16, 2016

Member

Here and below I would add Function to the fail() cases (demonstrating it doesn't get coerced to a string).

@sam-github

sam-github Nov 16, 2016

Member

Here and below I would add Function to the fail() cases (demonstrating it doesn't get coerced to a string).

pass('killSignal', undefined);
pass('killSignal', null);
pass('killSignal', 'SIGKILL');
pass('killSignal', 500);

This comment has been minimized.

@sam-github
@sam-github

sam-github Nov 16, 2016

Member

This comment has been minimized.

@cjihrig

cjihrig Dec 21, 2016

Contributor

I think the docs are wrong. It allows signal numbers from here -

options.killSignal = lookupSignal(options.killSignal);
.

@cjihrig

cjihrig Dec 21, 2016

Contributor

I think the docs are wrong. It allows signal numbers from here -

options.killSignal = lookupSignal(options.killSignal);
.

This comment has been minimized.

@thefourtheye

thefourtheye Dec 23, 2016

Contributor

Submitted #10424

@thefourtheye

thefourtheye Dec 23, 2016

Contributor

Submitted #10424

Show outdated Hide outdated test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Show outdated Hide outdated test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Show outdated Hide outdated lib/child_process.js Outdated
Show outdated Hide outdated lib/child_process.js Outdated
Show outdated Hide outdated test/parallel/test-child-process-spawn-typeerror.js Outdated
@jasnell

Few nits, almost there tho

Show outdated Hide outdated lib/child_process.js Outdated
Show outdated Hide outdated lib/child_process.js Outdated
Show outdated Hide outdated lib/child_process.js Outdated
Show outdated Hide outdated lib/child_process.js Outdated
pass('timeout', null);
pass('timeout', 1);
pass('timeout', 0);
fail('timeout', -1, err);

This comment has been minimized.

@targos

targos Nov 19, 2016

Member

suggestion: add a fail with a decimal number

@targos

targos Nov 19, 2016

Member

suggestion: add a fail with a decimal number

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 21, 2016

Contributor

Sorry for the delay. I just updated this.

Contributor

cjihrig commented Dec 21, 2016

Sorry for the delay. I just updated this.

@bnoordhuis

LGTM. Good test coverage.

pass('shell', undefined);
pass('shell', null);
pass('shell', false);
fail('shell', 0, err);

This comment has been minimized.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

I'd personally go for strict.

@bnoordhuis

bnoordhuis Dec 22, 2016

Member

I'd personally go for strict.

Show outdated Hide outdated lib/child_process.js Outdated
@cjihrig

This comment has been minimized.

Show comment
Hide comment
Contributor

cjihrig commented Dec 22, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 23, 2016

Contributor

CI is all green.

Contributor

cjihrig commented Dec 23, 2016

CI is all green.

@bnoordhuis bnoordhuis referenced this pull request Dec 23, 2016

Closed

child_process: improve killSignal validations #10423

3 of 3 tasks complete

cjihrig added some commits Aug 28, 2016

src: add handle check to spawn_sync
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: #8096
Fixes: #8539
Refs: #9722
PR-URL: #8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
child_process: improve input validation
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: #8096
Fixes: #8539
Refs: #9722
PR-URL: #8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
src: remove redundant spawn/spawnSync type checks
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: #8096
Fixes: #8539
Refs: #9722
PR-URL: #8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@cjihrig cjihrig merged commit 45c9ca7 into nodejs:master Dec 25, 2016

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Dec 25, 2016

Contributor

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

Contributor

cjihrig commented Dec 25, 2016

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

@cjihrig cjihrig deleted the cjihrig:spawnsync branch Dec 25, 2016

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jan 16, 2017

Member

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

Member

MylesBorins commented Jan 16, 2017

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

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123
Member

Fishrock123 commented Jan 16, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

src: add handle check to spawn_sync
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#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

child_process: improve input validation
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#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

src: remove redundant spawn/spawnSync type checks
This commit removes C++ checks from spawn() and spawnSync()
that are duplicates of the JavaScript type checking.

Fixes: nodejs#8096
Fixes: nodejs#8539
Refs: nodejs#9722
PR-URL: nodejs#8312
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment