Skip to content

Commit

Permalink
child_process: do not extend result for *Sync()
Browse files Browse the repository at this point in the history 
PR-URL: #13601
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
@mscdex
mscdex committed Jun 13, 2017
1 parent 9dc3f93 commit 448c4c6
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 85 deletions.
66 changes: 19 additions & 47 deletions lib/child_process.js
View file
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;

This comment has been minimized.

Sign in to view

Copy link
@Trott

Trott Jun 18, 2017 •
edited
Loading

Member

The change on this line is not fully covered by tests because the situation where ret.status < 0 is true is not covered by tests.
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
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
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
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
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'];
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

0 comments on commit 448c4c6

Please sign in to comment.