child_process: improve killSignal validations

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 <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Apr 4, 2017 · d75fdd96aac39f5432777a9763e7290419d401bd · d75fdd9
1 parent a5f91ab
commit d75fdd96aac39f5432777a9763e7290419d401bd
Unified Split

Showing with 54 additions and 40 deletions.

+7 −23 lib/child_process.js

+3 −14 lib/internal/child_process.js

+26 −0 lib/internal/util.js

+18 −3 test/parallel/test-child-process-spawnsync-validation-errors.js
diff --git 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
@@ -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
@@ -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
@@ -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());
+  }
 }