-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: add shell option to spawn() #4598
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,7 +71,8 @@ exports._forkChild = function(fd) { | |
|
||
|
||
function normalizeExecArgs(command /*, options, callback*/) { | ||
var file, args, options, callback; | ||
let options; | ||
let callback; | ||
|
||
if (typeof arguments[1] === 'function') { | ||
options = undefined; | ||
|
@@ -81,25 +82,12 @@ function normalizeExecArgs(command /*, options, callback*/) { | |
callback = arguments[2]; | ||
} | ||
|
||
if (process.platform === 'win32') { | ||
file = process.env.comspec || 'cmd.exe'; | ||
args = ['/s', '/c', '"' + command + '"']; | ||
// Make a shallow copy before patching so we don't clobber the user's | ||
// options object. | ||
options = util._extend({}, options); | ||
options.windowsVerbatimArguments = true; | ||
} else { | ||
file = '/bin/sh'; | ||
args = ['-c', command]; | ||
} | ||
|
||
if (options && options.shell) | ||
file = options.shell; | ||
// Make a shallow copy so we don't clobber the user's options object. | ||
options = Object.assign({}, options); | ||
options.shell = typeof options.shell === 'string' ? options.shell : true; | ||
|
||
return { | ||
cmd: command, | ||
file: file, | ||
args: args, | ||
file: command, | ||
options: options, | ||
callback: callback | ||
}; | ||
|
@@ -109,7 +97,6 @@ function normalizeExecArgs(command /*, options, callback*/) { | |
exports.exec = function(command /*, options, callback*/) { | ||
var opts = normalizeExecArgs.apply(null, arguments); | ||
return exports.execFile(opts.file, | ||
opts.args, | ||
opts.options, | ||
opts.callback); | ||
}; | ||
|
@@ -123,7 +110,8 @@ exports.execFile = function(file /*, args, options, callback*/) { | |
maxBuffer: 200 * 1024, | ||
killSignal: 'SIGTERM', | ||
cwd: null, | ||
env: null | ||
env: null, | ||
shell: false | ||
}; | ||
|
||
// Parse the optional positional parameters. | ||
|
@@ -153,6 +141,7 @@ exports.execFile = function(file /*, args, options, callback*/) { | |
env: options.env, | ||
gid: options.gid, | ||
uid: options.uid, | ||
shell: options.shell, | ||
windowsVerbatimArguments: !!options.windowsVerbatimArguments | ||
}); | ||
|
||
|
@@ -331,7 +320,23 @@ function normalizeSpawnArguments(file /*, args, options*/) { | |
else if (options === null || typeof options !== 'object') | ||
throw new TypeError('"options" argument must be an object'); | ||
|
||
options = util._extend({}, options); | ||
// Make a shallow copy so we don't clobber the user's options object. | ||
options = Object.assign({}, options); | ||
|
||
if (options.shell) { | ||
const command = [file].concat(args).join(' '); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't double quotes be escaped on Windows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably. I'll add a test for it too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, maybe not:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, I wonder what cmd.exe's algorithm for that is. Counting quotes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, looks like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Learned something new today. Thanks. |
||
|
||
if (process.platform === 'win32') { | ||
file = typeof options.shell === 'string' ? options.shell : | ||
process.env.comspec || 'cmd.exe'; | ||
args = ['/s', '/c', '"' + command + '"']; | ||
options.windowsVerbatimArguments = true; | ||
} else { | ||
file = typeof options.shell === 'string' ? options.shell : '/bin/sh'; | ||
args = ['-c', command]; | ||
} | ||
} | ||
|
||
args.unshift(file); | ||
|
||
var env = options.env || process.env; | ||
|
@@ -491,12 +496,12 @@ function execFileSync(/*command, args, options*/) { | |
exports.execFileSync = execFileSync; | ||
|
||
|
||
function execSync(/*command, options*/) { | ||
function execSync(command /*, options*/) { | ||
var opts = normalizeExecArgs.apply(null, arguments); | ||
var inheritStderr = opts.options ? !opts.options.stdio : true; | ||
|
||
var ret = spawnSync(opts.file, opts.args, opts.options); | ||
ret.cmd = opts.cmd; | ||
var ret = spawnSync(opts.file, opts.options); | ||
ret.cmd = command; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose the change on this line is harmless but I don't quite see why you made it. Because the format of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed it because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed that. Carry on. |
||
|
||
if (inheritStderr) | ||
process.stderr.write(ret.stderr); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const cp = require('child_process'); | ||
|
||
// Verify that a shell is, in fact, executed | ||
const doesNotExist = cp.spawn('does-not-exist', {shell: true}); | ||
|
||
assert.notEqual(doesNotExist.spawnfile, 'does-not-exist'); | ||
doesNotExist.on('error', common.fail); | ||
doesNotExist.on('exit', common.mustCall((code, signal) => { | ||
assert.strictEqual(signal, null); | ||
|
||
if (common.isWindows) | ||
assert.strictEqual(code, 1); // Exit code of cmd.exe | ||
else | ||
assert.strictEqual(code, 127); // Exit code of /bin/sh | ||
})); | ||
|
||
// Verify that passing arguments works | ||
const echo = cp.spawn('echo', ['foo'], { | ||
encoding: 'utf8', | ||
shell: true | ||
}); | ||
let echoOutput = ''; | ||
|
||
assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''), | ||
'echo foo'); | ||
echo.stdout.on('data', data => { | ||
echoOutput += data; | ||
}); | ||
echo.on('close', common.mustCall((code, signal) => { | ||
assert.strictEqual(echoOutput.trim(), 'foo'); | ||
})); | ||
|
||
// Verify that shell features can be used | ||
const cmd = common.isWindows ? 'echo bar | more' : 'echo bar | cat'; | ||
const command = cp.spawn(cmd, { | ||
encoding: 'utf8', | ||
shell: true | ||
}); | ||
let commandOutput = ''; | ||
|
||
command.stdout.on('data', data => { | ||
commandOutput += data; | ||
}); | ||
command.on('close', common.mustCall((code, signal) => { | ||
assert.strictEqual(commandOutput.trim(), 'bar'); | ||
})); | ||
|
||
// Verify that the environment is properly inherited | ||
const env = cp.spawn(`"${process.execPath}" -pe process.env.BAZ`, { | ||
env: Object.assign({}, process.env, {BAZ: 'buzz'}), | ||
encoding: 'utf8', | ||
shell: true | ||
}); | ||
let envOutput = ''; | ||
|
||
env.stdout.on('data', data => { | ||
envOutput += data; | ||
}); | ||
env.on('close', common.mustCall((code, signal) => { | ||
assert.strictEqual(envOutput.trim(), 'buzz'); | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const cp = require('child_process'); | ||
|
||
// Verify that a shell is, in fact, executed | ||
const doesNotExist = cp.spawnSync('does-not-exist', {shell: true}); | ||
|
||
assert.notEqual(doesNotExist.file, 'does-not-exist'); | ||
assert.strictEqual(doesNotExist.error, undefined); | ||
assert.strictEqual(doesNotExist.signal, null); | ||
|
||
if (common.isWindows) | ||
assert.strictEqual(doesNotExist.status, 1); // Exit code of cmd.exe | ||
else | ||
assert.strictEqual(doesNotExist.status, 127); // Exit code of /bin/sh | ||
|
||
// Verify that passing arguments works | ||
const echo = cp.spawnSync('echo', ['foo'], {shell: true}); | ||
|
||
assert.strictEqual(echo.args[echo.args.length - 1].replace(/"/g, ''), | ||
'echo foo'); | ||
assert.strictEqual(echo.stdout.toString().trim(), 'foo'); | ||
|
||
// Verify that shell features can be used | ||
const cmd = common.isWindows ? 'echo bar | more' : 'echo bar | cat'; | ||
const command = cp.spawnSync(cmd, {shell: true}); | ||
|
||
assert.strictEqual(command.stdout.toString().trim(), 'bar'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add one or two more tests to verify that the environment is inherited correctly? |
||
|
||
// Verify that the environment is properly inherited | ||
const env = cp.spawnSync(`"${process.execPath}" -pe process.env.BAZ`, { | ||
env: Object.assign({}, process.env, {BAZ: 'buzz'}), | ||
shell: true | ||
}); | ||
|
||
assert.strictEqual(env.stdout.toString().trim(), 'buzz'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work?
execFile()
ends up callingspawn()
but I can't figure out whereopts.args
is used as the arguments array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand your question. The
args
toexecFile()
is optional. Before this change,args
would contain the arguments to the shell and the command. With this change, that happens innormalizeSpawnArguments()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is what confuses me:
opts.args
from that object is passed to child.spawn().IOW, I get the impression that the
opts.args
from normalizeExecArgs() is lost. I assume that's not the case but I don't understand why not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to this change,
args
would have been something like['-c', command]
. That would have been sent throughexecFile()
,spawn()
, etc.With this change,
normalizeExecArgs()
setsshell
in options instead. This is passed through untilnormalizeSpawnArguments()
, where the same['-c', command]
is created and passed tochild.spawn()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, that makes sense. Objection withdrawn.