diff --git a/lib/child_process.js b/lib/child_process.js index 2f5dda870d66d7..03956a999e1141 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -315,6 +315,9 @@ function _convertCustomFds(options) { function normalizeSpawnArguments(file /*, args, options*/) { var args, options; + if (typeof file !== 'string' || file.length === 0) + throw new TypeError('"file" argument must be a non-empty string'); + if (Array.isArray(arguments[1])) { args = arguments[1].slice(0); options = arguments[2]; @@ -331,6 +334,47 @@ function normalizeSpawnArguments(file /*, args, options*/) { else if (options === null || typeof options !== 'object') throw new TypeError('"options" argument must be an object'); + // Validate the cwd, if present. + if (options.cwd != null && + typeof options.cwd !== 'string') { + throw new TypeError('"cwd" must be a string'); + } + + // Validate detached, if present. + if (options.detached != null && + typeof options.detached !== 'boolean') { + throw new TypeError('"detached" must be a boolean'); + } + + // Validate the uid, if present. + if (options.uid != null && !Number.isInteger(options.uid)) { + throw new TypeError('"uid" must be an integer'); + } + + // Validate the gid, if present. + if (options.gid != null && !Number.isInteger(options.gid)) { + throw new TypeError('"gid" must be an integer'); + } + + // Validate the shell, if present. + if (options.shell != null && + typeof options.shell !== 'boolean' && + typeof options.shell !== 'string') { + throw new TypeError('"shell" must be a boolean or string'); + } + + // Validate argv0, if present. + if (options.argv0 != null && + typeof options.argv0 !== 'string') { + throw new TypeError('"argv0" must be a string'); + } + + // Validate windowsVerbatimArguments, if present. + if (options.windowsVerbatimArguments != null && + typeof options.windowsVerbatimArguments !== 'boolean') { + throw new TypeError('"windowsVerbatimArguments" must be a boolean'); + } + // Make a shallow copy so we don't clobber the user's options object. options = Object.assign({}, options); @@ -420,13 +464,33 @@ function spawnSync(/*file, args, options*/) { debug('spawnSync', opts.args, options); + // Validate the timeout, if present. + if (options.timeout != null && + !(Number.isInteger(options.timeout) && options.timeout >= 0)) { + throw new TypeError('"timeout" must be an unsigned integer'); + } + + // Validate maxBuffer, if present. + if (options.maxBuffer != null && + !(Number.isInteger(options.maxBuffer) && options.maxBuffer >= 0)) { + throw new TypeError('"maxBuffer" must be an unsigned integer'); + } + options.file = opts.file; options.args = opts.args; options.envPairs = opts.envPairs; - if (options.killSignal) + // Validate the kill signal, if present. + if (typeof options.killSignal === 'string' || + typeof options.killSignal === 'number') { options.killSignal = lookupSignal(options.killSignal); + if (options.killSignal === 0) + throw new RangeError('"killSignal" cannot be 0'); + } else if (options.killSignal != null) { + throw new TypeError('"killSignal" must be a string or number'); + } + options.stdio = _validateStdio(options.stdio || 'pipe', true).stdio; if (options.input) { diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 3dcde0962af080..8c8e4704be4f82 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -121,35 +121,29 @@ class ProcessWrap : public HandleWrap { // options.uid Local uid_v = js_options->Get(env->uid_string()); - if (uid_v->IsInt32()) { + if (!uid_v->IsUndefined() && !uid_v->IsNull()) { + CHECK(uid_v->IsInt32()); const int32_t uid = uid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETUID; options.uid = static_cast(uid); - } else if (!uid_v->IsUndefined() && !uid_v->IsNull()) { - return env->ThrowTypeError("options.uid should be a number"); } // options.gid Local gid_v = js_options->Get(env->gid_string()); - if (gid_v->IsInt32()) { + if (!gid_v->IsUndefined() && !gid_v->IsNull()) { + CHECK(gid_v->IsInt32()); const int32_t gid = gid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETGID; options.gid = static_cast(gid); - } else if (!gid_v->IsUndefined() && !gid_v->IsNull()) { - return env->ThrowTypeError("options.gid should be a number"); } // TODO(bnoordhuis) is this possible to do without mallocing ? // options.file Local file_v = js_options->Get(env->file_string()); - node::Utf8Value file(env->isolate(), - file_v->IsString() ? file_v : Local()); - if (file.length() > 0) { - options.file = *file; - } else { - return env->ThrowTypeError("Bad argument"); - } + CHECK(file_v->IsString()); + node::Utf8Value file(env->isolate(), file_v); + options.file = *file; // options.args Local argv_v = js_options->Get(env->args_string()); diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 79f10a0ea2594d..68e78147578ff1 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -501,7 +501,12 @@ void SyncProcessRunner::CloseHandlesAndDeleteLoop() { // Close the process handle when ExitCallback was not called. uv_handle_t* uv_process_handle = reinterpret_cast(&uv_process_); - if (!uv_is_closing(uv_process_handle)) + + // Close the process handle if it is still open. The handle type also + // needs to be checked because TryInitializeAndRunLoop() won't spawn a + // process if input validation fails. + if (uv_process_handle->type == UV_PROCESS && + !uv_is_closing(uv_process_handle)) uv_close(uv_process_handle, nullptr); // Give closing watchers a chance to finish closing and get their close @@ -729,8 +734,7 @@ int SyncProcessRunner::ParseOptions(Local js_value) { } Local js_uid = js_options->Get(env()->uid_string()); if (IsSet(js_uid)) { - if (!js_uid->IsInt32()) - return UV_EINVAL; + CHECK(js_uid->IsInt32()); const int32_t uid = js_uid->Int32Value(env()->context()).FromJust(); uv_process_options_.uid = static_cast(uid); uv_process_options_.flags |= UV_PROCESS_SETUID; @@ -738,8 +742,7 @@ int SyncProcessRunner::ParseOptions(Local js_value) { Local js_gid = js_options->Get(env()->gid_string()); if (IsSet(js_gid)) { - if (!js_gid->IsInt32()) - return UV_EINVAL; + CHECK(js_gid->IsInt32()); const int32_t gid = js_gid->Int32Value(env()->context()).FromJust(); uv_process_options_.gid = static_cast(gid); uv_process_options_.flags |= UV_PROCESS_SETGID; @@ -755,28 +758,21 @@ int SyncProcessRunner::ParseOptions(Local js_value) { Local js_timeout = js_options->Get(env()->timeout_string()); if (IsSet(js_timeout)) { - if (!js_timeout->IsNumber()) - return UV_EINVAL; + CHECK(js_timeout->IsNumber()); int64_t timeout = js_timeout->IntegerValue(); - if (timeout < 0) - return UV_EINVAL; timeout_ = static_cast(timeout); } Local js_max_buffer = js_options->Get(env()->max_buffer_string()); if (IsSet(js_max_buffer)) { - if (!js_max_buffer->IsUint32()) - return UV_EINVAL; + CHECK(js_max_buffer->IsUint32()); max_buffer_ = js_max_buffer->Uint32Value(); } Local js_kill_signal = js_options->Get(env()->kill_signal_string()); if (IsSet(js_kill_signal)) { - if (!js_kill_signal->IsInt32()) - return UV_EINVAL; + CHECK(js_kill_signal->IsInt32()); kill_signal_ = js_kill_signal->Int32Value(); - if (kill_signal_ == 0) - return UV_EINVAL; } Local js_stdio = js_options->Get(env()->stdio_string()); diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 7b98cedf8ba117..a3ae3d36f5e80e 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -9,6 +9,8 @@ const cmd = common.isWindows ? 'rundll32' : 'ls'; const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; const invalidArgsMsg = /Incorrect value of args option/; const invalidOptionsMsg = /"options" argument must be an object/; +const invalidFileMsg = + /^TypeError: "file" argument must be a non-empty string$/; const empty = common.fixturesDir + '/empty.js'; assert.throws(function() { @@ -36,7 +38,16 @@ assert.doesNotThrow(function() { // verify that invalid argument combinations throw assert.throws(function() { spawn(); -}, /Bad argument/); +}, invalidFileMsg); + +assert.throws(function() { + spawn(''); +}, invalidFileMsg); + +assert.throws(function() { + const file = { toString() { throw new Error('foo'); } }; + spawn(file); +}, invalidFileMsg); assert.throws(function() { spawn(cmd, null); diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js new file mode 100644 index 00000000000000..e10137624863a6 --- /dev/null +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -0,0 +1,201 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const spawnSync = require('child_process').spawnSync; +const noop = function() {}; + +function pass(option, value) { + // Run the command with the specified option. Since it's not a real command, + // spawnSync() should run successfully but return an ENOENT error. + const child = spawnSync('not_a_real_command', { [option]: value }); + + assert.strictEqual(child.error.code, 'ENOENT'); +} + +function fail(option, value, message) { + assert.throws(() => { + spawnSync('not_a_real_command', { [option]: value }); + }, message); +} + +{ + // Validate the cwd option + const err = /^TypeError: "cwd" must be a string$/; + + pass('cwd', undefined); + pass('cwd', null); + pass('cwd', __dirname); + fail('cwd', 0, err); + fail('cwd', 1, err); + fail('cwd', true, err); + fail('cwd', false, err); + fail('cwd', [], err); + fail('cwd', {}, err); + fail('cwd', noop, err); +} + +{ + // Validate the detached option + const err = /^TypeError: "detached" must be a boolean$/; + + pass('detached', undefined); + pass('detached', null); + pass('detached', true); + pass('detached', false); + fail('detached', 0, err); + fail('detached', 1, err); + fail('detached', __dirname, err); + fail('detached', [], err); + fail('detached', {}, err); + fail('detached', noop, err); +} + +if (!common.isWindows) { + { + // Validate the uid option + if (process.getuid() !== 0) { + const err = /^TypeError: "uid" must be an integer$/; + + pass('uid', undefined); + pass('uid', null); + pass('uid', process.getuid()); + fail('uid', __dirname, err); + fail('uid', true, err); + fail('uid', false, err); + fail('uid', [], err); + fail('uid', {}, err); + fail('uid', noop, err); + fail('uid', NaN, err); + fail('uid', Infinity, err); + fail('uid', 3.1, err); + fail('uid', -3.1, err); + } + } + + { + // Validate the gid option + if (process.getgid() !== 0) { + const err = /^TypeError: "gid" must be an integer$/; + + pass('gid', undefined); + pass('gid', null); + pass('gid', process.getgid()); + fail('gid', __dirname, err); + fail('gid', true, err); + fail('gid', false, err); + fail('gid', [], err); + fail('gid', {}, err); + fail('gid', noop, err); + fail('gid', NaN, err); + fail('gid', Infinity, err); + fail('gid', 3.1, err); + fail('gid', -3.1, err); + } + } +} + +{ + // Validate the shell option + const err = /^TypeError: "shell" must be a boolean or string$/; + + pass('shell', undefined); + pass('shell', null); + pass('shell', false); + fail('shell', 0, err); + fail('shell', 1, err); + fail('shell', [], err); + fail('shell', {}, err); + fail('shell', noop, err); +} + +{ + // Validate the argv0 option + const err = /^TypeError: "argv0" must be a string$/; + + pass('argv0', undefined); + pass('argv0', null); + pass('argv0', 'myArgv0'); + fail('argv0', 0, err); + fail('argv0', 1, err); + fail('argv0', true, err); + fail('argv0', false, err); + fail('argv0', [], err); + fail('argv0', {}, err); + fail('argv0', noop, err); +} + +{ + // Validate the windowsVerbatimArguments option + const err = /^TypeError: "windowsVerbatimArguments" must be a boolean$/; + + pass('windowsVerbatimArguments', undefined); + pass('windowsVerbatimArguments', null); + pass('windowsVerbatimArguments', true); + pass('windowsVerbatimArguments', false); + fail('windowsVerbatimArguments', 0, err); + fail('windowsVerbatimArguments', 1, err); + fail('windowsVerbatimArguments', __dirname, err); + fail('windowsVerbatimArguments', [], err); + fail('windowsVerbatimArguments', {}, err); + fail('windowsVerbatimArguments', noop, err); +} + +{ + // Validate the timeout option + const err = /^TypeError: "timeout" must be an unsigned integer$/; + + pass('timeout', undefined); + pass('timeout', null); + pass('timeout', 1); + pass('timeout', 0); + fail('timeout', -1, err); + fail('timeout', true, err); + fail('timeout', false, err); + fail('timeout', __dirname, err); + fail('timeout', [], err); + fail('timeout', {}, err); + fail('timeout', noop, err); + fail('timeout', NaN, err); + fail('timeout', Infinity, err); + fail('timeout', 3.1, err); + fail('timeout', -3.1, err); +} + +{ + // Validate the maxBuffer option + const err = /^TypeError: "maxBuffer" must be an unsigned integer$/; + + pass('maxBuffer', undefined); + pass('maxBuffer', null); + pass('maxBuffer', 1); + pass('maxBuffer', 0); + fail('maxBuffer', 3.14, err); + fail('maxBuffer', -1, err); + fail('maxBuffer', NaN, err); + fail('maxBuffer', Infinity, err); + fail('maxBuffer', true, err); + fail('maxBuffer', false, err); + fail('maxBuffer', __dirname, err); + fail('maxBuffer', [], err); + fail('maxBuffer', {}, err); + fail('maxBuffer', noop, err); +} + +{ + // 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', noop, typeErr); +}