From d75fdd96aac39f5432777a9763e7290419d401bd Mon Sep 17 00:00:00 2001 From: "Sakthipriyan Vairamani (thefourtheye)" Date: Fri, 23 Dec 2016 14:36:20 +0530 Subject: [PATCH] child_process: improve killSignal validations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/10423 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- lib/child_process.js | 30 +++++-------------- lib/internal/child_process.js | 17 ++--------- lib/internal/util.js | 26 ++++++++++++++++ ...ild-process-spawnsync-validation-errors.js | 21 +++++++++++-- 4 files changed, 54 insertions(+), 40 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index c2811109b7bbee..34e420eadab72a 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -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'); @@ -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, @@ -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; @@ -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); @@ -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; @@ -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; } diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 104ac4f6363ff9..05e1cd63b02f70 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -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; @@ -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; @@ -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); diff --git a/lib/internal/util.js b/lib/internal/util.js index 3de57040f90381..7bf92566d8b1a5 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -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']; @@ -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); +}; diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js index c49e4960b0f7b1..83b114f7ba76b2 100644 --- a/test/parallel/test-child-process-spawnsync-validation-errors.js +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -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, @@ -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()); + } }