child_process: check accessing a null stderr #6877

robertchiras · 2016-05-19T14:26:13Z

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

child_process, test

Description of change

Fix for child_process when accessing a null stderr and addition test path to exercise this false path.

cjihrig · 2016-05-19T14:36:12Z

LGTM

cjihrig · 2016-05-19T14:37:00Z

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

eljefedelrodeodeljefe · 2016-05-19T15:42:34Z

test/sequential/test-child-process-execsync.js

+    execSync('exit -1', {shell: 'no_shell'});
+  }, /spawnSync no_shell ENOENT/);
+})();
+


nits: Why the closure? If just for scope maybe use block scope. Also we prefer arrow functions (I guess)

{ assert.throws(() => { execSync('exit -1', {shell: 'no_shell'}); }, /spawnSync no_shell ENOENT/); }

I am not familiar with Node.js code, so just copied the test from the end of this file and modified to thi particular use case.
But I can edit the comit and use your suggestion.
Thanks!

In this case it's okay then. Don't bother.

It doesn't even need the closure at all I think?

(Should probably be fixed before landing)

There's a similar test further down below. This new test should probably be added there.

...that and I guess everyone before just copy-pasted tests, which is why the closure has propagated :)

Yes, there is a similar test down below, but I put this test the first one, because it has to be ran before other calls to execSync.
For example, if the below test, with timeout, is ran before the one added in this patch, it could throw another exception, masking the real one.

I'm not sure I follow your logic. If any test in this file fails the test runner marks it as failed. I don't see where exception masking comes in.

If the actual exception is masked, the one who runs the test won't be able to easily identify the problem.
If I move this test at the end, then there is no reason for adding it, since the first test will clearly fail.
The problem with accessing a null stderr is that the test with timeout will:

fail in ETIMEOUT assert, this way err won't be assigned

in it's finally clause, the assert for err.status will also throw another exception, since err is null

The test will fail telling that it cannot access the 'status' of an undefined member at "assert(err.status > 128 || err.signal)", which apparently appears to be a problem in the test itself.

eljefedelrodeodeljefe · 2016-05-19T15:43:58Z

~~Left a comment,~~ LGTM otherwise

jasnell · 2016-05-19T22:43:20Z

LGTM but agree that the test should be cleaned up a bit.

robertchiras · 2016-05-20T11:42:58Z

I cleaned the test as suggested. Should I also clean the similar test from the end of this test file in this patch too?

bnoordhuis · 2016-05-20T11:56:07Z

LGTM with a question: #6877 (comment)

Should I also clean the similar test from the end of this test file in this patch too?

No, that's fine. Let's see what the CI says: https://ci.nodejs.org/job/node-test-pull-request/2711/

bnoordhuis · 2016-06-06T17:30:14Z

CI one more time: https://ci.nodejs.org/job/node-test-pull-request/2936/

Fishrock123 · 2016-06-15T23:01:31Z

lib/child_process.js

@@ -489,7 +489,7 @@ function execFileSync(/*command, args, options*/) {

  var ret = spawnSync(opts.file, opts.args.slice(1), opts.options);

-  if (inheritStderr)
+  if (inheritStderr && ret.stderr)


The test doesn't catch this one.

Fishrock123 · 2016-06-15T23:04:47Z

LGTM minus the missing test.

I will say error output when the test fails isn't the best, it could perhaps use a message parameter.

Fishrock123 · 2016-06-17T20:32:00Z

ping @robertchiras

robertchiras · 2016-06-21T07:47:40Z

@Fishrock123 sorry for my late response. I was gone, without access to a PC.
I don't fully understand your concern here: "I will say error output when the test fails isn't the best, it could perhaps use a message parameter."
The scope of the test I added is to verify that the correct exception is thrown when trying to run an invalid command. Because, that's what the initial problem was: by not checking the strerr before writing to it, it will cause another exception, masking the initial one.

Fishrock123 · 2016-06-29T12:24:49Z

@robertchiras You also need a test for execFileSync().

As for message, you may want to add a message argument as per the docs for assert.throws(), as that may make the failure output a bit easier to decipher.

Fishrock123 · 2016-07-07T12:14:42Z

ping @robertchiras

robertchiras · 2016-07-11T07:59:05Z

@Fishrock123 sorry for the long delay. I updated the test according to your suggestions.

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. Signed-off-by: Robert Chiras <[email protected]>

thefourtheye · 2016-07-11T08:21:07Z

@robertchiras One last thing. The commits don't reflect your GitHub account. You might want to follow these steps https://help.github.com/articles/setting-your-username-in-git/

Fishrock123 · 2016-07-11T13:32:04Z

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

Fishrock123 · 2016-07-18T16:39:07Z

@robertchiras Mind updating your Git info for the commits? When you do I'll land this. :)

$ git config --global user.email "[email protected]"
$ git config --global user.name "J. Random User"

Then

$ git commit --amend --reset-author
$ git push origin head --force

robertchiras · 2016-07-19T10:59:38Z

Hi @Fishrock123: I already had my git config set up. I did it again and pushed the changes. What do you see wrong in my commit that do not reflect my Github account? Thanks!

Exercise the call to execSync with a bad shell passed through options. Verify that the right exception is thrown. Signed-off-by: Robert Chiras <[email protected]>

targos · 2016-07-19T11:13:32Z

@robertchiras You need to add the email address you have setup in git to your GitHub profile: https://github.com/settings/emails

robertchiras · 2016-07-19T11:18:48Z

I used my work email address, probably this is why it didn't saw that it was me. I updated my work email account on Github. Now it should look good.

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. PR-URL: #6877 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

Fishrock123 · 2016-07-19T11:29:26Z

Thanks! Landed in 765de1a 🎉 :D

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. PR-URL: #6877 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

MylesBorins · 2016-09-30T16:42:23Z

This change is causing tests to fail on v4.x-staging. Is this something we should be putting time into getting working on v4.x?

robertchiras · 2016-10-11T11:13:48Z

@thealphanerd, sorry for the late response.
I pulled v4.x-staging, but I see that the test I made changes into, is working just fine. Indeed, I see come other tests failing on v4.x-staging, but not related to my change.
Can you give me more info about the failing test on v4.x-staging?

thefourtheye · 2016-10-11T13:48:24Z

@robertchiras This is the error I am getting

=== release test-child-process-execsync ===
Path: sequential/test-child-process-execsync
assert.js:353
    throw actual;
    ^

Error: spawnSync exit -1 ENOENT
    at exports._errnoException (util.js:911:11)
    at spawnSync (child_process.js:443:20)
    at execFileSync (child_process.js:480:13)
    at test/sequential/test-child-process-execsync.js:22:16
    at _tryBlock (assert.js:312:5)
    at _throws (assert.js:331:12)
    at Function.assert.throws (assert.js:361:3)
    at Object.<anonymous> (test/sequential/test-child-process-execsync.js:21:8)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
Command: out/Release/node test/sequential/test-child-process-execsync.js
[00:53|% 100|+ 1058|-   1]: Done
make: *** [test] Error 1

MylesBorins · 2016-10-11T16:21:46Z

@robertchiras the v4.x-staging branch is working fine for me without this fix applied. What OS are you on?

robertchiras · 2016-10-17T12:16:43Z

Sorry, but for some reason, I assumed that my patch is already in v4.x-staging. I applied it and saw the issue.
I will upload a new pull request with the patch.

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. PR-URL: nodejs#6877 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. Ref: #9152 PR-URL: #6877 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Jefe Lindstädt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 19, 2016

robertchiras force-pushed the test_fixes branch from b00e5ca to c2fd588 Compare May 19, 2016 14:29

eljefedelrodeodeljefe reviewed May 19, 2016
View reviewed changes

eljefedelrodeodeljefe changed the title ~~Fix for child_process~~ child_process: check accessing a null stderr May 19, 2016

robertchiras force-pushed the test_fixes branch from c2fd588 to c21401e Compare May 20, 2016 11:41

Fishrock123 reviewed Jun 15, 2016
View reviewed changes

Fishrock123 self-assigned this Jun 29, 2016

robertchiras force-pushed the test_fixes branch 2 times, most recently from a6e6b6f to 2c1c798 Compare July 11, 2016 07:57

child_process: Check stderr before accessing it

646a211

If something bad happens in spawnSync, stderr might be null. Therefore, we have to check it before using it, so we won't mask the actual exception. Signed-off-by: Robert Chiras <[email protected]>

robertchiras force-pushed the test_fixes branch from 2c1c798 to 91ff704 Compare July 19, 2016 10:58

test: Additional test for execSync

91ff704

Exercise the call to execSync with a bad shell passed through options. Verify that the right exception is thrown. Signed-off-by: Robert Chiras <[email protected]>

Fishrock123 closed this Jul 19, 2016

evanlucas mentioned this pull request Jul 21, 2016

v6.3.1 Release Proposal #7782

Merged

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

robertchiras mentioned this pull request Oct 18, 2016

child_process: Check stderr before accessing it #9152

Closed

4 tasks

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

child_process: check accessing a null stderr #6877

child_process: check accessing a null stderr #6877

Conversation

robertchiras commented May 19, 2016 • edited by eljefedelrodeodeljefe Loading

Checklist

Affected core subsystem(s)

Description of change

cjihrig commented May 19, 2016

cjihrig commented May 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 May 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljefedelrodeodeljefe commented May 19, 2016 • edited Loading

jasnell commented May 19, 2016

robertchiras commented May 20, 2016

bnoordhuis commented May 20, 2016

bnoordhuis commented Jun 6, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 15, 2016

Fishrock123 commented Jun 17, 2016

robertchiras commented Jun 21, 2016

Fishrock123 commented Jun 29, 2016

Fishrock123 commented Jul 7, 2016

robertchiras commented Jul 11, 2016

thefourtheye commented Jul 11, 2016

Fishrock123 commented Jul 11, 2016

Fishrock123 commented Jul 18, 2016

robertchiras commented Jul 19, 2016

targos commented Jul 19, 2016

robertchiras commented Jul 19, 2016

Fishrock123 commented Jul 19, 2016

MylesBorins commented Sep 30, 2016

robertchiras commented Oct 11, 2016

thefourtheye commented Oct 11, 2016 • edited Loading

MylesBorins commented Oct 11, 2016

robertchiras commented Oct 17, 2016

robertchiras commented May 19, 2016 •

edited by eljefedelrodeodeljefe

Loading

Fishrock123 May 19, 2016 •

edited

Loading

eljefedelrodeodeljefe commented May 19, 2016 •

edited

Loading

thefourtheye commented Oct 11, 2016 •

edited

Loading