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: fork() with shell is impossible? #13983

Closed
vsemozhetbyt opened this issue Jun 29, 2017 · 2 comments
Closed

child_process: fork() with shell is impossible? #13983

vsemozhetbyt opened this issue Jun 29, 2017 · 2 comments
Labels
child_process Issues and PRs related to the child_process subsystem. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 29, 2017

  • Version: from v4. x up to v9.0
  • Platform: Windows 7 x64
  • Subsystem: child_process

Currently, the doc says nothing if fork() is executed with shell, also no shell option is mentioned. However, fork() is based upon spawn() and almost all the options are transferred as is. So, without shell option we have the default spawn() behavior (without shell):

if (!process.argv[2]) {
  require('child_process').fork(__filename, ['%temp%'], { });
} else {
  console.log(process.argv[2]);
}
%temp%

However, if shell option is set to true, fork() becomes broken in at least two ways:

  1. If a path to the executable has spaces, we have this error:
if (!process.argv[2]) {
  require('child_process').fork(__filename, ['%temp%'], { shell: true });
} else {
  console.log(process.argv[2]);
}
>node test.js
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
  1. If a path to the executable has no spaces, we have this error:
>node.8.1.3.exe test.js
child_process.js:106
  p.open(fd);
    ^

Error: EBADF: bad file descriptor, uv_pipe_open
    at Object.exports._forkChild (child_process.js:106:5)
    at Object.setupChannel (internal/process.js:247:8)
    at startup (bootstrap_node.js:53:16)
    at bootstrap_node.js:575:3

So there are some questions:

  1. Should we document fork() and shell interaction (and shell option) and fix these issues?
  2. If not, should we strip shell option before spawning (and maybe somehow document this)?
@vsemozhetbyt vsemozhetbyt added the child_process Issues and PRs related to the child_process subsystem. label Jun 29, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Jul 1, 2017

FWIW, I can't reproduce with master on macOS. Perhaps this is Windows specific.

@vsemozhetbyt vsemozhetbyt added the windows Issues and PRs related to the Windows platform. label Jul 1, 2017
@bnoordhuis bnoordhuis added the good first issue Issues that are suitable for first-time contributors. label Aug 3, 2017
@bnoordhuis
Copy link
Member

If not, should we strip shell option before spawning (and maybe somehow document this)?

Let's do this; fork() + shell doesn't make sense. I've added a good-first-contribution label.

cjihrig pushed a commit to cjihrig/node that referenced this issue 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>
addaleax pushed a commit to addaleax/ayo that referenced this issue 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>
jasnell pushed a commit that referenced this issue 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 issue 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>
MylesBorins pushed a commit that referenced this issue 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 issue 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 issue 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>
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. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

3 participants