child_process: add shell option to spawn()#4598

cjihrig · 2016-01-09T01:38:11Z

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for
compatibility with exec()'s shell option.

Closes#1009

bnoordhuis · 2016-01-09T14:11:31Z

lib/child_process.js

#4593 is about deprecating util._extend(). Maybe use Object.assign() here?
EDIT: Just a suggestion, feel free to ignore.

I'd prefer to use Object.assign(). I was waiting to see how #4593 played out, but the reaction seems to be good, so I'll replace this.

cjihrig · 2016-01-11T17:55:19Z

@bnoordhuis I've addressed your comments.

jasnell · 2016-01-11T18:42:17Z

LGTM

cjihrig · 2016-01-11T20:28:22Z

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

EDIT: Test failed on Windows. Trying again. CI: https://ci.nodejs.org/job/node-test-pull-request/1198/

cjihrig · 2016-01-11T22:10:31Z

CI is all green, but I'd like sign off from @bnoordhuis

cjihrig · 2016-01-18T15:54:25Z

@bnoordhuis hope you're feeling better. Ping for a review.

cjihrig · 2016-01-26T17:22:49Z

@bnoordhuis sorry, one more ping.

bnoordhuis · 2016-01-26T17:40:19Z

lib/child_process.js

Shouldn't double quotes be escaped on Windows?

Probably. I'll add a test for it too.

Actually, maybe not:
C:\Users\Colin>cmd.exe /s /c "echo "foo"" "foo" C:\Users\Colin>cmd.exe /s /c "echo \"foo\"" \"foo\"

Huh, I wonder what cmd.exe's algorithm for that is. Counting quotes?

Ah, looks like the /s enables that behavior - http://stackoverflow.com/questions/9866962/what-is-cmd-s-for

Learned something new today. Thanks.

bnoordhuis · 2016-01-26T17:50:46Z

Sorry for the delay. Left some comments.

cjihrig · 2016-01-26T18:26:40Z

@bnoordhuis, I think I've addressed all of your comments.

bnoordhuis · 2016-01-26T22:13:45Z

LGTM if the CI is happy.

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: nodejs#1009 PR-URL: nodejs#4598 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2016-12-29T17:40:53Z

/cc @nodejs/lts
Would we want to include this in a future LTS release for v4.x?

sam-github · 2016-12-29T18:32:01Z

I'm +1 for back-port, it improves compatibility between the LTS versions.

This commit adds a shell option, to spawn() and spawnSync(). This option allows child processes to be spawned with or without a shell. The option also allows a custom shell to be defined, for compatibility with exec()'s shell option. Fixes: #1009 PR-URL: #4598 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796

Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796 PR-URL: #10973

Notable Changes: * child_process: add shell option to spawn() (cjihrig) nodejs/node#4598 * crypto: * add ALPN Support (Shigeki Ohtsu) nodejs/node#2564 * allow adding extra certs to well-known CAs (Sam Roberts) nodejs/node#9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) nodejs/node#4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) nodejs/node#5333 * process: * add `externalMemory` to `process` (Fedor Indutny) nodejs/node#9587 * add process.cpuUsage() (Patrick Mueller) nodejs/node#10796 Signed-off-by: Ilkka Myller <[email protected]>

Use the shell option of spawn introduced in Node.js 4.8 (see nodejs/node#4598) to pass the command to the OS shell. Supersedes kentcdodds#77. BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.

* feat(spawn): add support for quoted scripts Use the shell option of spawn introduced in Node.js 4.8 (see nodejs/node#4598) to pass the command to the OS shell. Supersedes #77. * docs(readme): add gotchas Add Gotchas paragraph about passing variables to multiple scripts. * docs(readme): add required node version badge Replace the Prerequisite paragraph by a badge showing the require version of Node.js required to run cross-env. * test(spawn): add test for quoted scripts See #89 (review). BREAKING CHANGE: Changes the behavior when passed quoted scripts or special characters interpreted by the shell.

mohd-akram · 2025-02-19T07:09:01Z

I have no idea why this and #18237 were added. It seems really dangerous to give the impression that, since these functions accept an array of arguments, that they will be passed as-is (safely) to the command. The user should concatenate the arguments themselves to make clear that no escaping/isolation of arguments will happen. Right now all it takes is changing shell to true to eg. workaround the Windows .bat/.cmd issue and break proper handling of arguments at best or introduce a gaping security hole at worst.

mscdex added the child_process Issues and PRs related to the child_process subsystem. label Jan 9, 2016

bnoordhuis reviewed Jan 9, 2016
View reviewed changes

janicduplessis mentioned this pull request Jan 11, 2016
Fix launching text editor from a redbox stacktrace on windows facebook/react-native#5238
Closed

cjihrig force-pushed the shell branch 3 times, most recently from ce56f29 to a7c5918Compare January 11, 2016 17:52

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 11, 2016

cjihrig force-pushed the shell branch from a7c5918 to aea316eCompare January 11, 2016 21:24

bnoordhuis reviewed Jan 26, 2016
View reviewed changes

cjihrig force-pushed the shell branch from aea316e to 78f8a5fCompare January 26, 2016 18:00

cjihrig force-pushed the shell branch from 78f8a5f to f2e70a5Compare January 26, 2016 18:07

MylesBorins added lts-agenda labels Dec 29, 2016

This was referenced Jan 4, 2017
spawn() fails to properly exercise terminal applications nodejs/citgm#136
Closed
Potential Semver Minor Backports nodejs/Release#177
Closed

MylesBorins added land-on-v4.x and removed lts-agenda labels Jan 13, 2017

MylesBorins mentioned this pull request Jan 24, 2017
v4.8.0 proposal #10973
Merged

This was referenced Feb 28, 2017
Switches to cross-spawn to fix issues with Windows shells workshopper/learn-sass#50
Merged
child_process.spawn shell option is available in Node v5 and v4 too moxystudio/node-cross-spawn#68
Closed

chexxor mentioned this pull request Mar 14, 2017
Initialize .purs-repl file. purescript-contrib/pulp#260
Merged

bnoordhuis mentioned this pull request Mar 29, 2017
child_process#spawn breaks backward compatibility in node v4.8.0 #12117
Closed

Vanuan mentioned this pull request Sep 12, 2017
Error: Couldn't find the binary node install.js in 1.0.1 yarnpkg/yarn#4337
Closed

idanpa mentioned this pull request Apr 11, 2018
child_process.spawn adds flags that might cause non-native shells to fail on windows #19947
Closed

Uh oh!

child_process: add shell option to spawn()#4598

child_process: add shell option to spawn() #4598

Uh oh!

Conversation

cjihrig commented Jan 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

jasnell commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

cjihrig commented Jan 18, 2016

Uh oh!

cjihrig commented Jan 26, 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!

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 commented Jan 26, 2016

Uh oh!

cjihrig commented Jan 26, 2016

Uh oh!

bnoordhuis commented Jan 26, 2016

Uh oh!

MylesBorins commented Dec 29, 2016

Uh oh!

sam-github commented Dec 29, 2016

Uh oh!

mohd-akram commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants