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>

Author: thefourtheye

lib/child_process.js

@@ -22,9 +22,8 @@
 'use strict';
 
 constutil=require('util');
-constinternalUtil=require('internal/util');
+const{ deprecate, convertToValidSignal }=require('internal/util');
 constdebug=util.debuglog('child_process');
-constconstants=process.binding('constants').os.signals;
 
 constuv=process.binding('uv');
 constspawn_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);
+
 varchild=spawn(file,args,{
 cwd: options.cwd,
 env: options.env,
@@ -332,7 +333,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
 returnchild;
 };
 
-const_deprecatedCustomFds=internalUtil.deprecate(
+const_deprecatedCustomFds=deprecate(
 functiondeprecateCustomFds(options){
 options.stdio=options.customFds.map(functionmapCustomFds(fd){
 returnfd===-1 ? 'pipe' : fd;
@@ -474,18 +475,6 @@ var spawn = exports.spawn = function(/*file, args, options*/) {
 returnchild;
 };
 
-
-functionlookupSignal(signal){
-if(typeofsignal==='number')
-returnsignal;
-
-if(!(signalinconstants))
-thrownewError('Unknown signal: '+signal);
-
-returnconstants[signal];
-}
-
-
 functionspawnSync(/*file, args, options*/){
 varopts=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) {
 }
 
 
-functionvalidateKillSignal(killSignal){
+functionsanitizeKillSignal(killSignal){
 if(typeofkillSignal==='string'||typeofkillSignal==='number'){
-killSignal=lookupSignal(killSignal);
-
-if(killSignal===0)
-thrownewRangeError('"killSignal" cannot be 0');
+returnconvertToValidSignal(killSignal);
 }elseif(killSignal!=null){
 thrownewTypeError('"killSignal" must be a string or number');
 }
-
-returnkillSignal;
 }

lib/internal/child_process.js

@@ -5,7 +5,6 @@ const EventEmitter = require('events');
 constnet=require('net');
 constdgram=require('dgram');
 constutil=require('util');
-constconstants=process.binding('constants').os.signals;
 constassert=require('assert');
 
 constProcess=process.binding('process_wrap').Process;
@@ -17,6 +16,7 @@ const TCP = process.binding('tcp_wrap').TCP;
 constUDP=process.binding('udp_wrap').UDP;
 constSocketList=require('internal/socket_list');
 const{ isUint8Array }=process.binding('util');
+const{ convertToValidSignal }=require('internal/util');
 
 consterrnoException=util._errnoException;
 constSocketListSend=SocketList.SocketListSend;
@@ -362,19 +362,8 @@ function onErrorNT(self, err) {
 
 
 ChildProcess.prototype.kill=function(sig){
-varsignal;
-
-if(sig===0){
-signal=0;
-}elseif(!sig){
-signal=constants['SIGTERM'];
-}else{
-signal=constants[sig];
-}
-
-if(signal===undefined){
-thrownewError('Unknown signal: '+sig);
-}
+constsignal=sig===0 ? sig :
+convertToValidSignal(sig===undefined ? 'SIGTERM' : sig);
 
 if(this._handle){
 varerr=this._handle.kill(signal);

lib/internal/util.js

@@ -1,6 +1,7 @@
 'use strict';
 
 constbinding=process.binding('util');
+constsignals=process.binding('constants').os.signals;
 
 constkArrowMessagePrivateSymbolIndex=binding['arrow_message_private_symbol'];
 constkDecoratedPrivateSymbolIndex=binding['decorated_private_symbol'];
@@ -179,3 +180,28 @@ exports.createClassWrapper = function createClassWrapper(type) {
 fn.prototype=type.prototype;
 returnfn;
 };
+
+letsignalsToNamesMapping;
+functiongetSignalsToNamesMapping(){
+if(signalsToNamesMapping!==undefined)
+returnsignalsToNamesMapping;
+
+signalsToNamesMapping=Object.create(null);
+for(constkeyinsignals){
+signalsToNamesMapping[signals[key]]=key;
+}
+
+returnsignalsToNamesMapping;
+}
+
+exports.convertToValidSignal=functionconvertToValidSignal(signal){
+if(typeofsignal==='number'&&getSignalsToNamesMapping()[signal])
+returnsignal;
+
+if(typeofsignal==='string'){
+constsignalName=signals[signal.toUpperCase()];
+if(signalName)returnsignalName;
+}
+
+thrownewError('Unknown signal: '+signal);
+};

test/parallel/test-child-process-spawnsync-validation-errors.js

@@ -2,6 +2,7 @@
 constcommon=require('../common');
 constassert=require('assert');
 constspawnSync=require('child_process').spawnSync;
+constsignals=process.binding('constants').os.signals;
 
 functionpass(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
 consttypeErr=/^TypeError:"killSignal"mustbeastringornumber$/;
-constrangeErr=/^RangeError:"killSignal"cannotbe0$/;
 constunknownSignalErr=/^Error:Unknownsignal:/;
 
 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(constsignalNameinsignals){
+pass('killSignal',signals[signalName]);
+pass('killSignal',signalName);
+pass('killSignal',signalName.toLowerCase());
+}
 }