FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

child_process: improve killSignal validations · nodejs/node@d75fdd9 · GitHub

/ node Public

Commit d75fdd9

Browse files
thefourtheye
committed
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>
1 parent a5f91ab commit d75fdd9

4 files changed

Lines changed: 54 additions & 40 deletions

File tree

‎lib/child_process.js‎

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,8 @@
2222
'use strict';
2323

2424
const util = require('util');
25-
const internalUtil = require('internal/util');
25+
const { deprecate, convertToValidSignal } = require('internal/util');
2626
const debug = util.debuglog('child_process');
27-
const constants = process.binding('constants').os.signals;
2827

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

183+
options.killSignal = sanitizeKillSignal(options.killSignal);
184+
184185
var child = spawn(file, args, {
185186
cwd: options.cwd,
186187
env: options.env,
@@ -332,7 +333,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
332333
return child;
333334
};
334335

335-
const _deprecatedCustomFds = internalUtil.deprecate(
336+
const _deprecatedCustomFds = deprecate(
336337
function deprecateCustomFds(options) {
337338
options.stdio = options.customFds.map(function mapCustomFds(fd) {
338339
return fd === -1 ? 'pipe' : fd;
@@ -474,18 +475,6 @@ var spawn = exports.spawn = function(/*file, args, options*/) {
474475
return child;
475476
};
476477

477-
478-
function lookupSignal(signal) {
479-
if (typeof signal === 'number')
480-
return signal;
481-
482-
if (!(signal in constants))
483-
throw new Error('Unknown signal: ' + signal);
484-
485-
return constants[signal];
486-
}
487-
488-
489478
function spawnSync(/*file, args, options*/) {
490479
var opts = normalizeSpawnArguments.apply(null, arguments);
491480

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

508497
// Validate and translate the kill signal, if present.
509-
options.killSignal = validateKillSignal(options.killSignal);
498+
options.killSignal = sanitizeKillSignal(options.killSignal);
510499

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

@@ -632,15 +621,10 @@ function validateMaxBuffer(maxBuffer) {
632621
}
633622

634623

635-
function validateKillSignal(killSignal) {
624+
function sanitizeKillSignal(killSignal) {
636625
if (typeof killSignal === 'string' || typeof killSignal === 'number') {
637-
killSignal = lookupSignal(killSignal);
638-
639-
if (killSignal === 0)
640-
throw new RangeError('"killSignal" cannot be 0');
626+
return convertToValidSignal(killSignal);
641627
} else if (killSignal != null) {
642628
throw new TypeError('"killSignal" must be a string or number');
643629
}
644-
645-
return killSignal;
646630
}

‎lib/internal/child_process.js‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ const EventEmitter = require('events');
55
const net = require('net');
66
const dgram = require('dgram');
77
const util = require('util');
8-
const constants = process.binding('constants').os.signals;
98
const assert = require('assert');
109

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

2121
const errnoException = util._errnoException;
2222
const SocketListSend = SocketList.SocketListSend;
@@ -362,19 +362,8 @@ function onErrorNT(self, err) {
362362

363363

364364
ChildProcess.prototype.kill = function(sig) {
365-
var signal;
366-
367-
if (sig === 0) {
368-
signal = 0;
369-
} else if (!sig) {
370-
signal = constants['SIGTERM'];
371-
} else {
372-
signal = constants[sig];
373-
}
374-
375-
if (signal === undefined) {
376-
throw new Error('Unknown signal: ' + sig);
377-
}
365+
const signal = sig === 0 ? sig :
366+
convertToValidSignal(sig === undefined ? 'SIGTERM' : sig);
378367

379368
if (this._handle) {
380369
var err = this._handle.kill(signal);

‎lib/internal/util.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
'use strict';
22

33
const binding = process.binding('util');
4+
const signals = process.binding('constants').os.signals;
45

56
const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
67
const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol'];
@@ -179,3 +180,28 @@ exports.createClassWrapper = function createClassWrapper(type) {
179180
fn.prototype = type.prototype;
180181
return fn;
181182
};
183+
184+
let signalsToNamesMapping;
185+
function getSignalsToNamesMapping() {
186+
if (signalsToNamesMapping !== undefined)
187+
return signalsToNamesMapping;
188+
189+
signalsToNamesMapping = Object.create(null);
190+
for (const key in signals) {
191+
signalsToNamesMapping[signals[key]] = key;
192+
}
193+
194+
return signalsToNamesMapping;
195+
}
196+
197+
exports.convertToValidSignal = function convertToValidSignal(signal) {
198+
if (typeof signal === 'number' && getSignalsToNamesMapping()[signal])
199+
return signal;
200+
201+
if (typeof signal === 'string') {
202+
const signalName = signals[signal.toUpperCase()];
203+
if (signalName) return signalName;
204+
}
205+
206+
throw new Error('Unknown signal: ' + signal);
207+
};

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
const common = require('../common');
33
const assert = require('assert');
44
const spawnSync = require('child_process').spawnSync;
5+
const signals = process.binding('constants').os.signals;
56

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

190190
pass('killSignal', undefined);
191191
pass('killSignal', null);
192192
pass('killSignal', 'SIGKILL');
193-
pass('killSignal', 500);
194-
fail('killSignal', 0, rangeErr);
195193
fail('killSignal', 'SIGNOTAVALIDSIGNALNAME', unknownSignalErr);
196194
fail('killSignal', true, typeErr);
197195
fail('killSignal', false, typeErr);
198196
fail('killSignal', [], typeErr);
199197
fail('killSignal', {}, typeErr);
200198
fail('killSignal', common.noop, typeErr);
199+
200+
// Invalid signal names and numbers should fail
201+
fail('killSignal', 500, unknownSignalErr);
202+
fail('killSignal', 0, unknownSignalErr);
203+
fail('killSignal', -200, unknownSignalErr);
204+
fail('killSignal', 3.14, unknownSignalErr);
205+
206+
Object.getOwnPropertyNames(Object.prototype).forEach((property) => {
207+
fail('killSignal', property, unknownSignalErr);
208+
});
209+
210+
// Valid signal names and numbers should pass
211+
for (const signalName in signals) {
212+
pass('killSignal', signals[signalName]);
213+
pass('killSignal', signalName);
214+
pass('killSignal', signalName.toLowerCase());
215+
}
201216
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL