Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d75fdd9

Browse filesBrowse files
thefourtheyethefourtheye
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
Copy full SHA for d75fdd9

File tree

Expand file treeCollapse file tree

4 files changed

+54
-40
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+54
-40
lines changed
Open diff view settings
Collapse file

‎lib/child_process.js‎

Copy file name to clipboardExpand all lines: lib/child_process.js
+7-23Lines 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
}
Collapse file

‎lib/internal/child_process.js‎

Copy file name to clipboardExpand all lines: lib/internal/child_process.js
+3-14Lines 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);
Collapse file

‎lib/internal/util.js‎

Copy file name to clipboardExpand all lines: lib/internal/util.js
+26Lines 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+
};
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-child-process-spawnsync-validation-errors.js
+18-3Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.