Skip to content

Commit

Permalink
child_process: improve killSignal validations
Browse files Browse the repository at this point in the history
As it is, the `killSignal` is just retrieved from an object and used.
If the signal passed is actually one of the inherited properties of
that object, Node.js will die. For example,

    ➜  node -e "child_process.spawnSync('ls', {killSignal: 'toString'})"
    Assertion failed: (0), function uv_close, file ....core.c, line 166.
    [1]    58938 abort      node -e "child_process.spawnSync(...)"

1. This patch makes sure that the signal is actually a own property of
   the constants object.

2. Extends the killSignal validation to all the other functions.

PR-URL: #10423

Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
thefourtheye committed Apr 4, 2017
1 parent a5f91ab commit d75fdd9
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 40 deletions.
30 changes: 7 additions & 23 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@
'use strict';

const util = require('util');
const internalUtil = require('internal/util');
const { deprecate, convertToValidSignal } = require('internal/util');
const debug = util.debuglog('child_process');
const constants = process.binding('constants').os.signals;

const uv = process.binding('uv');
const spawn_sync = process.binding('spawn_sync');
Expand Down Expand Up @@ -181,6 +180,8 @@ exports.execFile = function(file /*, args, options, callback*/) {
// Validate maxBuffer, if present.
validateMaxBuffer(options.maxBuffer);

options.killSignal = sanitizeKillSignal(options.killSignal);

var child = spawn(file, args, {
cwd: options.cwd,
env: options.env,
Expand Down Expand Up @@ -332,7 +333,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
return child;
};

const _deprecatedCustomFds = internalUtil.deprecate(
const _deprecatedCustomFds = deprecate(
function deprecateCustomFds(options) {
options.stdio = options.customFds.map(function mapCustomFds(fd) {
return fd === -1 ? 'pipe' : fd;
Expand Down Expand Up @@ -474,18 +475,6 @@ var spawn = exports.spawn = function(/*file, args, options*/) {
return child;
};


function lookupSignal(signal) {
if (typeof signal === 'number')
return signal;

if (!(signal in constants))
throw new Error('Unknown signal: ' + signal);

return constants[signal];
}


function spawnSync(/*file, args, options*/) {
var opts = normalizeSpawnArguments.apply(null, arguments);

Expand All @@ -506,7 +495,7 @@ function spawnSync(/*file, args, options*/) {
options.envPairs = opts.envPairs;

// Validate and translate the kill signal, if present.
options.killSignal = validateKillSignal(options.killSignal);
options.killSignal = sanitizeKillSignal(options.killSignal);

options.stdio = _validateStdio(options.stdio || 'pipe', true).stdio;

Expand Down Expand Up @@ -632,15 +621,10 @@ function validateMaxBuffer(maxBuffer) {
}


function validateKillSignal(killSignal) {
function sanitizeKillSignal(killSignal) {
if (typeof killSignal === 'string' || typeof killSignal === 'number') {
killSignal = lookupSignal(killSignal);

if (killSignal === 0)
throw new RangeError('"killSignal" cannot be 0');
return convertToValidSignal(killSignal);
} else if (killSignal != null) {
throw new TypeError('"killSignal" must be a string or number');
}

return killSignal;
}
17 changes: 3 additions & 14 deletions lib/internal/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const EventEmitter = require('events');
const net = require('net');
const dgram = require('dgram');
const util = require('util');
const constants = process.binding('constants').os.signals;
const assert = require('assert');

const Process = process.binding('process_wrap').Process;
Expand All @@ -17,6 +16,7 @@ const TCP = process.binding('tcp_wrap').TCP;
const UDP = process.binding('udp_wrap').UDP;
const SocketList = require('internal/socket_list');
const { isUint8Array } = process.binding('util');
const { convertToValidSignal } = require('internal/util');

const errnoException = util._errnoException;
const SocketListSend = SocketList.SocketListSend;
Expand Down Expand Up @@ -362,19 +362,8 @@ function onErrorNT(self, err) {


ChildProcess.prototype.kill = function(sig) {
var signal;

if (sig === 0) {
signal = 0;
} else if (!sig) {
signal = constants['SIGTERM'];
} else {
signal = constants[sig];
}

if (signal === undefined) {
throw new Error('Unknown signal: ' + sig);
}
const signal = sig === 0 ? sig :
convertToValidSignal(sig === undefined ? 'SIGTERM' : sig);

if (this._handle) {
var err = this._handle.kill(signal);
Expand Down
26 changes: 26 additions & 0 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const binding = process.binding('util');
const signals = process.binding('constants').os.signals;

const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol'];
Expand Down Expand Up @@ -179,3 +180,28 @@ exports.createClassWrapper = function createClassWrapper(type) {
fn.prototype = type.prototype;
return fn;
};

let signalsToNamesMapping;
function getSignalsToNamesMapping() {
if (signalsToNamesMapping !== undefined)
return signalsToNamesMapping;

signalsToNamesMapping = Object.create(null);
for (const key in signals) {
signalsToNamesMapping[signals[key]] = key;
}

return signalsToNamesMapping;
}

exports.convertToValidSignal = function convertToValidSignal(signal) {
if (typeof signal === 'number' && getSignalsToNamesMapping()[signal])
return signal;

if (typeof signal === 'string') {
const signalName = signals[signal.toUpperCase()];
if (signalName) return signalName;
}

throw new Error('Unknown signal: ' + signal);
};
21 changes: 18 additions & 3 deletions test/parallel/test-child-process-spawnsync-validation-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../common');
const assert = require('assert');
const spawnSync = require('child_process').spawnSync;
const signals = process.binding('constants').os.signals;

function pass(option, value) {
// Run the command with the specified option. Since it's not a real command,
Expand Down Expand Up @@ -184,18 +185,32 @@ if (!common.isWindows) {
{
// Validate the killSignal option
const typeErr = /^TypeError: "killSignal" must be a string or number$/;
const rangeErr = /^RangeError: "killSignal" cannot be 0$/;
const unknownSignalErr = /^Error: Unknown signal:/;

pass('killSignal', undefined);
pass('killSignal', null);
pass('killSignal', 'SIGKILL');
pass('killSignal', 500);
fail('killSignal', 0, rangeErr);
fail('killSignal', 'SIGNOTAVALIDSIGNALNAME', unknownSignalErr);
fail('killSignal', true, typeErr);
fail('killSignal', false, typeErr);
fail('killSignal', [], typeErr);
fail('killSignal', {}, typeErr);
fail('killSignal', common.noop, typeErr);

// Invalid signal names and numbers should fail
fail('killSignal', 500, unknownSignalErr);
fail('killSignal', 0, unknownSignalErr);
fail('killSignal', -200, unknownSignalErr);
fail('killSignal', 3.14, unknownSignalErr);

Object.getOwnPropertyNames(Object.prototype).forEach((property) => {
fail('killSignal', property, unknownSignalErr);
});

// Valid signal names and numbers should pass
for (const signalName in signals) {
pass('killSignal', signals[signalName]);
pass('killSignal', signalName);
pass('killSignal', signalName.toLowerCase());
}
}

0 comments on commit d75fdd9

Please sign in to comment.