Skip to content
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: remove shell option before fork #15299

Closed
wants to merge 4 commits into from
Closed

child_process: remove shell option before fork #15299

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 9, 2017

Fixes: #13983

Checklist
  • [x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [x ] tests and/or benchmarks are included
  • [x ] documentation is changed or added
Affected core subsystem(s)

child_process

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Sep 9, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Sep 9, 2017

This should have a test. #13983 can probably help with that. We should also document the behavior. It might also be better if we can do this without the Windows check.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

I missed the entire test generation section of the contribution doc. First time at this.

*Edit: I checked in a test. Thanks for the gentle reminder/nudge. In the spirit of this being my first attempt at a contribution it is about as complex as the actual code change but hopefully sufficient. On to documentation.

@ghost
Copy link
Author

ghost commented Sep 9, 2017

RE: Not doing the platform check - do you want me to just explicitly set it false and document that shell isn't supported for fork?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 10, 2017

@agresnel I think that based on #13983 (comment), that should be fine. Thanks for taking care of this.

@ghost ghost changed the title child_process: remove shell option before win32 fork spawn child_process: remove shell option before fork Sep 10, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple comments. I think you might want to consider something like this for the test:

'use strict';
// Refs: https://github.com/nodejs/node/issues/13983
// This test verifies that the shell option is not supported by fork().
require('../common');
const assert = require('assert');
const cp = require('child_process');

if (process.argv[2] === undefined) {
  cp.fork(__filename, ['$foo'], {
    shell: true,
    env: { foo: 'bar' }
  });
} else {
  assert.strictEqual(process.argv[2], '$foo');
}

I only tried this test on macOS. You probably need to conditionally use %foo% instead of $foo on Windows.

@@ -335,7 +335,7 @@ The `child_process.fork()` method is a special case of
Like [`child_process.spawn()`][], a [`ChildProcess`][] object is returned. The returned
[`ChildProcess`][] will have an additional communication channel built-in that
allows messages to be passed back and forth between the parent and child. See
[`subprocess.send()`][] for details.
[`subprocess.send()`][] for details.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace change here.

@@ -0,0 +1,29 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New files don't need this copyright header. You can remove it.

const child_process = require('child_process');
const fixtures = require('../common/fixtures');

//As per: https://github.com/nodejs/node/issues/13983 should produce no error output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after //.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines should be wrapped at 80 characters too. make lint should have caught this.

@Fishrock123
Copy link
Contributor

The PR owner's account appears to have been deleted. @cjihrig if you are interested in this you may want to take their commit and make a new PR from it.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 11, 2017

@Fishrock123 thanks for the heads up. Will do.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 12, 2017

Moved to #15352

@cjihrig cjihrig closed this Sep 12, 2017
cjihrig pushed a commit to cjihrig/node that referenced this pull request Sep 14, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: nodejs#15299
Fixes: nodejs#13983
PR-URL: nodejs#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this pull request Sep 14, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: nodejs#15299
PR-URL: nodejs#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: nodejs/node#15299
Fixes: nodejs/node#13983
PR-URL: nodejs/node#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 17, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: nodejs/node#15299
PR-URL: nodejs/node#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: #15299
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
jasnell pushed a commit that referenced this pull request Sep 20, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: #15299
Fixes: #13983
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: nodejs/node#15299
Fixes: nodejs/node#13983
PR-URL: nodejs/node#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: nodejs/node#15299
PR-URL: nodejs/node#15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: #15299
Fixes: #13983
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: #15299
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: #15299
Fixes: #13983
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: #15299
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
This commit ensures that spawn()'s shell option is unconditionally
set to false when fork() is called.

Refs: #15299
Fixes: #13983
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
This commit verifies that the child_process fork() method does
not honor the shell option.

Refs: #15299
PR-URL: #15352
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants