Skip to content

child_process: do not extend result for *Sync() #13601

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 13, 2017
Merged

child_process: do not extend result for *Sync() #13601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 19 additions & 47 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,11 @@ const { createPromise,
const debug = util.debuglog('child_process');

const uv = process.binding('uv');
const spawn_sync = process.binding('spawn_sync');
const Buffer = require('buffer').Buffer;
const Pipe = process.binding('pipe_wrap').Pipe;
const { isUint8Array } = process.binding('util');
const child_process = require('internal/child_process');

const errnoException = util._errnoException;
const _validateStdio = child_process._validateStdio;
const setupChannel = child_process.setupChannel;
const ChildProcess = exports.ChildProcess = child_process.ChildProcess;
Expand Down Expand Up @@ -508,8 +506,6 @@ function spawnSync(/*file, args, options*/) {

var options = opts.options;

var i;

debug('spawnSync', opts.args, options);

// Validate the timeout, if present.
Expand All @@ -533,7 +529,7 @@ function spawnSync(/*file, args, options*/) {
}

// We may want to pass data in on any given fd, ensure it is a valid buffer
for (i = 0; i < options.stdio.length; i++) {
for (var i = 0; i < options.stdio.length; i++) {
var input = options.stdio[i] && options.stdio[i].input;
if (input != null) {
var pipe = options.stdio[i] = util._extend({}, options.stdio[i]);
Expand All @@ -549,50 +545,27 @@ function spawnSync(/*file, args, options*/) {
}
}

var result = spawn_sync.spawn(options);

if (result.output && options.encoding && options.encoding !== 'buffer') {
for (i = 0; i < result.output.length; i++) {
if (!result.output[i])
continue;
result.output[i] = result.output[i].toString(options.encoding);
}
}

result.stdout = result.output && result.output[1];
result.stderr = result.output && result.output[2];

if (result.error) {
result.error = errnoException(result.error, 'spawnSync ' + opts.file);
result.error.path = opts.file;
result.error.spawnargs = opts.args.slice(1);
}

util._extend(result, opts);

return result;
return child_process.spawnSync(opts);
}
exports.spawnSync = spawnSync;


function checkExecSyncError(ret) {
if (ret.error || ret.status !== 0) {
var err = ret.error;
ret.error = null;

if (!err) {
var msg = 'Command failed: ';
msg += ret.cmd || ret.args.join(' ');
if (ret.stderr && ret.stderr.length > 0)
msg += '\n' + ret.stderr.toString();
err = new Error(msg);
}

util._extend(err, ret);
return err;
function checkExecSyncError(ret, args, cmd) {
var err;
if (ret.error) {
err = ret.error;
} else if (ret.status !== 0) {
var msg = 'Command failed: ';
msg += cmd || args.join(' ');
if (ret.stderr && ret.stderr.length > 0)
msg += '\n' + ret.stderr.toString();
err = new Error(msg);
}

return false;
if (err) {
err.status = ret.status < 0 ? uv.errname(ret.status) : ret.status;
err.signal = ret.signal;
}
return err;
}


Expand All @@ -605,7 +578,7 @@ function execFileSync(/*command, args, options*/) {
if (inheritStderr && ret.stderr)
process.stderr.write(ret.stderr);

var err = checkExecSyncError(ret);
var err = checkExecSyncError(ret, opts.args, undefined);

if (err)
throw err;
Expand All @@ -620,12 +593,11 @@ function execSync(command /*, options*/) {
var inheritStderr = !opts.options.stdio;

var ret = spawnSync(opts.file, opts.options);
ret.cmd = command;

if (inheritStderr && ret.stderr)
process.stderr.write(ret.stderr);

var err = checkExecSyncError(ret);
var err = checkExecSyncError(ret, opts.args, command);

if (err)
throw err;
Expand Down
28 changes: 27 additions & 1 deletion lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const UDP = process.binding('udp_wrap').UDP;
const SocketList = require('internal/socket_list');
const { isUint8Array } = process.binding('util');
const { convertToValidSignal } = require('internal/util');
const spawn_sync = process.binding('spawn_sync');

const errnoException = util._errnoException;
const SocketListSend = SocketList.SocketListSend;
Expand Down Expand Up @@ -898,9 +899,34 @@ function maybeClose(subprocess) {
}
}

function spawnSync(opts) {
var options = opts.options;
var result = spawn_sync.spawn(options);

if (result.output && options.encoding && options.encoding !== 'buffer') {
for (var i = 0; i < result.output.length; i++) {
if (!result.output[i])
continue;
result.output[i] = result.output[i].toString(options.encoding);
}
}

result.stdout = result.output && result.output[1];
result.stderr = result.output && result.output[2];

if (result.error) {
result.error = errnoException(result.error, 'spawnSync ' + opts.file);
result.error.path = opts.file;
result.error.spawnargs = opts.args.slice(1);
}

return result;
}

module.exports = {
ChildProcess,
setupChannel,
_validateStdio,
getSocketList
getSocketList,
spawnSync
};
39 changes: 23 additions & 16 deletions test/parallel/test-child-process-custom-fds.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
// Flags: --expose_internals
'use strict';
const common = require('../common');
const assert = require('assert');
const internalCp = require('internal/child_process');
const oldSpawnSync = internalCp.spawnSync;

// Verify that customFds is used if stdio is not provided.
{
Expand All @@ -9,25 +12,29 @@ const assert = require('assert');
common.expectWarning('DeprecationWarning', msg);

const customFds = [-1, process.stdout.fd, process.stderr.fd];
const child = common.spawnSyncPwd({ customFds });

assert.deepStrictEqual(child.options.customFds, customFds);
assert.deepStrictEqual(child.options.stdio, [
{ type: 'pipe', readable: true, writable: false },
{ type: 'fd', fd: process.stdout.fd },
{ type: 'fd', fd: process.stderr.fd }
]);
internalCp.spawnSync = common.mustCall(function(opts) {
assert.deepStrictEqual(opts.options.customFds, customFds);
assert.deepStrictEqual(opts.options.stdio, [
{ type: 'pipe', readable: true, writable: false },
{ type: 'fd', fd: process.stdout.fd },
{ type: 'fd', fd: process.stderr.fd }
]);
});
common.spawnSyncPwd({ customFds });
internalCp.spawnSync = oldSpawnSync;
}

// Verify that customFds is ignored when stdio is present.
{
const customFds = [0, 1, 2];
const child = common.spawnSyncPwd({ customFds, stdio: 'pipe' });

assert.deepStrictEqual(child.options.customFds, customFds);
assert.deepStrictEqual(child.options.stdio, [
{ type: 'pipe', readable: true, writable: false },
{ type: 'pipe', readable: false, writable: true },
{ type: 'pipe', readable: false, writable: true }
]);
internalCp.spawnSync = common.mustCall(function(opts) {
assert.deepStrictEqual(opts.options.customFds, customFds);
assert.deepStrictEqual(opts.options.stdio, [
{ type: 'pipe', readable: true, writable: false },
{ type: 'pipe', readable: false, writable: true },
{ type: 'pipe', readable: false, writable: true }
]);
});
common.spawnSyncPwd({ customFds, stdio: 'pipe' });
internalCp.spawnSync = oldSpawnSync;
}
32 changes: 23 additions & 9 deletions test/parallel/test-child-process-spawnsync-kill-signal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Flags: --expose_internals
'use strict';
const common = require('../common');
const assert = require('assert');
Expand All @@ -6,13 +7,22 @@ const cp = require('child_process');
if (process.argv[2] === 'child') {
setInterval(common.noop, 1000);
} else {
const internalCp = require('internal/child_process');
const oldSpawnSync = internalCp.spawnSync;
const { SIGKILL } = process.binding('constants').os.signals;

function spawn(killSignal) {
function spawn(killSignal, beforeSpawn) {
if (beforeSpawn) {
internalCp.spawnSync = common.mustCall(function(opts) {
beforeSpawn(opts);
return oldSpawnSync(opts);
});
}
const child = cp.spawnSync(process.execPath,
[__filename, 'child'],
{killSignal, timeout: 100});

if (beforeSpawn)
internalCp.spawnSync = oldSpawnSync;
assert.strictEqual(child.status, null);
assert.strictEqual(child.error.code, 'ETIMEDOUT');
return child;
Expand All @@ -25,26 +35,30 @@ if (process.argv[2] === 'child') {

// Verify that the default kill signal is SIGTERM.
{
const child = spawn();
const child = spawn(undefined, (opts) => {
assert.strictEqual(opts.options.killSignal, undefined);
});

assert.strictEqual(child.signal, 'SIGTERM');
assert.strictEqual(child.options.killSignal, undefined);
}

// Verify that a string signal name is handled properly.
{
const child = spawn('SIGKILL');
const child = spawn('SIGKILL', (opts) => {
assert.strictEqual(opts.options.killSignal, SIGKILL);
});

assert.strictEqual(child.signal, 'SIGKILL');
assert.strictEqual(child.options.killSignal, SIGKILL);
}

// Verify that a numeric signal is handled properly.
{
const child = spawn(SIGKILL);

assert.strictEqual(typeof SIGKILL, 'number');

const child = spawn(SIGKILL, (opts) => {
assert.strictEqual(opts.options.killSignal, SIGKILL);
});

assert.strictEqual(child.signal, 'SIGKILL');
assert.strictEqual(child.options.killSignal, SIGKILL);
}
}
33 changes: 21 additions & 12 deletions test/parallel/test-child-process-spawnsync-shell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
// Flags: --expose_internals
'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const internalCp = require('internal/child_process');
const oldSpawnSync = internalCp.spawnSync;

// Verify that a shell is, in fact, executed
const doesNotExist = cp.spawnSync('does-not-exist', {shell: true});
Expand All @@ -16,10 +19,14 @@ else
assert.strictEqual(doesNotExist.status, 127); // Exit code of /bin/sh

// Verify that passing arguments works
internalCp.spawnSync = common.mustCall(function(opts) {
assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''),
'echo foo');
return oldSpawnSync(opts);
});
const echo = cp.spawnSync('echo', ['foo'], {shell: true});
internalCp.spawnSync = oldSpawnSync;

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
Expand Down Expand Up @@ -52,16 +59,18 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz');
const shellFlags = platform === 'win32' ? ['/d', '/s', '/c'] : ['-c'];
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: common.isWindows...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it really matters much since we're already explicitly passing 'win32' to this function below here.

const outputCmd = platform === 'win32' ? `"${cmd}"` : cmd;
const windowsVerbatim = platform === 'win32' ? true : undefined;
const result = cp.spawnSync(cmd, { shell });

assert.strictEqual(result.file, shellOutput);
assert.deepStrictEqual(result.args,
[shellOutput, ...shellFlags, outputCmd]);
assert.strictEqual(result.options.shell, shell);
assert.strictEqual(result.options.file, result.file);
assert.deepStrictEqual(result.options.args, result.args);
assert.strictEqual(result.options.windowsVerbatimArguments,
windowsVerbatim);
internalCp.spawnSync = common.mustCall(function(opts) {
assert.strictEqual(opts.file, shellOutput);
assert.deepStrictEqual(opts.args,
[shellOutput, ...shellFlags, outputCmd]);
assert.strictEqual(opts.options.shell, shell);
assert.strictEqual(opts.options.file, opts.file);
assert.deepStrictEqual(opts.options.args, opts.args);
assert.strictEqual(opts.options.windowsVerbatimArguments,
windowsVerbatim);
});
cp.spawnSync(cmd, { shell });
internalCp.spawnSync = oldSpawnSync;
}

// Test Unix platforms with the default shell.
Expand Down