From 358bd79af1e542b8419dcdff7d4663ef863b5030 Mon Sep 17 00:00:00 2001 From: Giovanni Date: Thu, 5 Dec 2024 17:45:22 +0100 Subject: [PATCH] child_process: improve docs and validate strings in exec and spawn --- lib/child_process.js | 60 +++++++++-------- .../parallel/test-child-process-exec-error.js | 64 +++++++++++++++++-- test/parallel/test-child-process-execfile.js | 54 ++++++++++++++++ .../test-child-process-spawn-error.js | 28 ++++++++ test/parallel/test-child-process-spawnsync.js | 27 ++++++++ 5 files changed, 202 insertions(+), 31 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 3fb21f755be3d7..af10b5b86c7c1c 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -186,8 +186,7 @@ function _forkChild(fd, serializationMode) { } function normalizeExecArgs(command, options, callback) { - validateString(command, 'command'); - validateArgumentNullCheck(command, 'command'); + validateStringParam(command, 'command'); if (typeof options === 'function') { callback = options; @@ -260,6 +259,8 @@ ObjectDefineProperty(exec, promisify.custom, { }); function normalizeExecFileArgs(file, args, options, callback) { + validateStringParam(file, 'file'); + if (ArrayIsArray(args)) { args = ArrayPrototypeSlice(args); } else if (args != null && typeof args === 'object') { @@ -535,12 +536,17 @@ function copyProcessEnvToEnv(env, name, optionEnv) { } } -function normalizeSpawnArguments(file, args, options) { - validateString(file, 'file'); - validateArgumentNullCheck(file, 'file'); +function validateStringParam(param, paramName) { + validateString(param, paramName); + validateArgumentNullCheck(param, paramName); - if (file.length === 0) - throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty'); + if (param.length === 0) { + throw new ERR_INVALID_ARG_VALUE(paramName, param, 'cannot be empty'); + } +} + +function normalizeSpawnArguments(command, args, options) { + validateStringParam(command, 'command'); if (ArrayIsArray(args)) { args = ArrayPrototypeSlice(args); @@ -611,35 +617,35 @@ function normalizeSpawnArguments(file, args, options) { if (options.shell) { validateArgumentNullCheck(options.shell, 'options.shell'); - const command = ArrayPrototypeJoin([file, ...args], ' '); + const fullCommand = ArrayPrototypeJoin([command, ...args], ' '); // Set the shell, switches, and commands. if (process.platform === 'win32') { if (typeof options.shell === 'string') - file = options.shell; + command = options.shell; else - file = process.env.comspec || 'cmd.exe'; + command = process.env.comspec || 'cmd.exe'; // '/d /s /c' is used only for cmd.exe. - if (RegExpPrototypeExec(/^(?:.*\\)?cmd(?:\.exe)?$/i, file) !== null) { - args = ['/d', '/s', '/c', `"${command}"`]; + if (RegExpPrototypeExec(/^(?:.*\\)?cmd(?:\.exe)?$/i, command) !== null) { + args = ['/d', '/s', '/c', `"${fullCommand}"`]; windowsVerbatimArguments = true; } else { - args = ['-c', command]; + args = ['-c', fullCommand]; } } else { if (typeof options.shell === 'string') - file = options.shell; + command = options.shell; else if (process.platform === 'android') - file = '/system/bin/sh'; + command = '/system/bin/sh'; else - file = '/bin/sh'; - args = ['-c', command]; + command = '/bin/sh'; + args = ['-c', fullCommand]; } } if (typeof options.argv0 === 'string') { ArrayPrototypeUnshift(args, options.argv0); } else { - ArrayPrototypeUnshift(args, file); + ArrayPrototypeUnshift(args, command); } const env = options.env || process.env; @@ -702,7 +708,7 @@ function normalizeSpawnArguments(file, args, options) { cwd, detached: !!options.detached, envPairs, - file, + file: command, windowsHide: !!options.windowsHide, windowsVerbatimArguments: !!windowsVerbatimArguments, }; @@ -721,8 +727,8 @@ function abortChildProcess(child, killSignal, reason) { } /** - * Spawns a new process using the given `file`. - * @param {string} file + * Spawns a new process using the given `command`. + * @param {string} command * @param {string[]} [args] * @param {{ * cwd?: string | URL; @@ -742,8 +748,8 @@ function abortChildProcess(child, killSignal, reason) { * }} [options] * @returns {ChildProcess} */ -function spawn(file, args, options) { - options = normalizeSpawnArguments(file, args, options); +function spawn(command, args, options) { + options = normalizeSpawnArguments(command, args, options); validateTimeout(options.timeout); validateAbortSignal(options.signal, 'options.signal'); const killSignal = sanitizeKillSignal(options.killSignal); @@ -791,8 +797,8 @@ function spawn(file, args, options) { } /** - * Spawns a new process synchronously using the given `file`. - * @param {string} file + * Spawns a new process synchronously using the given `command`. + * @param {string} command * @param {string[]} [args] * @param {{ * cwd?: string | URL; @@ -820,11 +826,11 @@ function spawn(file, args, options) { * error: Error; * }} */ -function spawnSync(file, args, options) { +function spawnSync(command, args, options) { options = { __proto__: null, maxBuffer: MAX_BUFFER, - ...normalizeSpawnArguments(file, args, options), + ...normalizeSpawnArguments(command, args, options), }; debug('spawnSync', options); diff --git a/test/parallel/test-child-process-exec-error.js b/test/parallel/test-child-process-exec-error.js index cd45f3071c2920..17fd2c23fd892d 100644 --- a/test/parallel/test-child-process-exec-error.js +++ b/test/parallel/test-child-process-exec-error.js @@ -22,7 +22,7 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const child_process = require('child_process'); +const {exec, execSync, execFile} = require('child_process'); function test(fn, code, expectPidType = 'number') { const child = fn('does-not-exist', common.mustCall(function(err) { @@ -35,10 +35,66 @@ function test(fn, code, expectPidType = 'number') { // With `shell: true`, expect pid (of the shell) if (common.isWindows) { - test(child_process.exec, 1, 'number'); // Exit code of cmd.exe + test(exec, 1, 'number'); // Exit code of cmd.exe } else { - test(child_process.exec, 127, 'number'); // Exit code of /bin/sh + test(exec, 127, 'number'); // Exit code of /bin/sh } // With `shell: false`, expect no pid -test(child_process.execFile, 'ENOENT', 'undefined'); +test(execFile, 'ENOENT', 'undefined'); + + +// Verify that the exec() function throws when command parameter is not a valid string +{ + assert.throws(() => { + exec(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "command" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + exec('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' cannot be empty. Received ''" + }); + + assert.throws(() => { + exec('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' must be a string without null bytes. Received '\\x00'" + }); +} + + +// Verify that the execSync() function throws when command parameter is not a valid string +{ + assert.throws(() => { + execSync(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "command" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + execSync('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' cannot be empty. Received ''" + }); + + assert.throws(() => { + execSync('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' must be a string without null bytes. Received '\\x00'" + }); +} diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index c4dba6b3f9466f..6fac7301b07a39 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -125,3 +125,57 @@ const execOpts = { encoding: 'utf8', shell: true, env: { ...process.env, NODE: p })); }); } + +// Verify that the execFile() function throws when file parameter is not a valid string +{ + assert.throws(() => { + execFile(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "file" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + execFile('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'file' cannot be empty. Received ''" + }); + + assert.throws(() => { + execFile('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'file' must be a string without null bytes. Received '\\x00'" + }); +} + +// Verify that the execFileSync() function throws when file parameter is not a valid string +{ + assert.throws(() => { + execFileSync(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "file" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + execFileSync('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'file' cannot be empty. Received ''" + }); + + assert.throws(() => { + execFileSync('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'file' must be a string without null bytes. Received '\\x00'" + }); +} diff --git a/test/parallel/test-child-process-spawn-error.js b/test/parallel/test-child-process-spawn-error.js index ed1c8fac9f97ed..208d6ed22fb69e 100644 --- a/test/parallel/test-child-process-spawn-error.js +++ b/test/parallel/test-child-process-spawn-error.js @@ -53,3 +53,31 @@ enoentChild.on('error', common.mustCall(function(err) { assert.strictEqual(err.path, enoentPath); assert.deepStrictEqual(err.spawnargs, spawnargs); })); + + +// Verify that the spawn() function throws when command parameter is not a valid string +{ + assert.throws(() => { + spawn(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "command" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + spawn('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' cannot be empty. Received ''" + }); + + assert.throws(() => { + spawn('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' must be a string without null bytes. Received '\\x00'" + }); +} diff --git a/test/parallel/test-child-process-spawnsync.js b/test/parallel/test-child-process-spawnsync.js index 9ec125ea891689..672db36023185f 100644 --- a/test/parallel/test-child-process-spawnsync.js +++ b/test/parallel/test-child-process-spawnsync.js @@ -65,3 +65,30 @@ assert.deepStrictEqual(ret_err.spawnargs, ['bar']); ]; assert.deepStrictEqual(retUTF8.output, stringifiedDefault); } + +// Verify that the spawnSync() function throws when command parameter is not a valid string +{ + assert.throws(() => { + spawnSync(123, common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "command" argument must be of type string. Received type number (123)' + }); + + assert.throws(() => { + spawnSync('', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' cannot be empty. Received ''" + }); + + assert.throws(() => { + spawnSync('\u0000', common.mustNotCall()); + }, { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'command' must be a string without null bytes. Received '\\x00'" + }); +}