diff --git a/lib/child_process.js b/lib/child_process.js index 887f5668e37a..44658af535f7 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -27,12 +27,10 @@ const { ArrayPrototypeIncludes, ArrayPrototypeJoin, ArrayPrototypeLastIndexOf, - ArrayPrototypePush, ArrayPrototypePushApply, ArrayPrototypeSlice, ArrayPrototypeSort, ArrayPrototypeSplice, - ArrayPrototypeUnshift, ObjectAssign, ObjectDefineProperty, ObjectPrototypeHasOwnProperty, @@ -46,6 +44,7 @@ const { } = primordials; const { + arrayAppend, assignFunctionName, convertToValidSignal, getSystemErrorName, @@ -466,7 +465,7 @@ function execFile(file, args, options, callback) { child.stdout.on('data', function onChildStdout(chunk) { // Do not need to count the length if (options.maxBuffer === Infinity) { - ArrayPrototypePush(_stdout, chunk); + arrayAppend(_stdout, chunk); return; } const encoding = child.stdout.readableEncoding; @@ -479,12 +478,12 @@ function execFile(file, args, options, callback) { if (stdoutLen > options.maxBuffer) { const truncatedLen = options.maxBuffer - (stdoutLen - length); - ArrayPrototypePush(_stdout, slice(chunk, 0, truncatedLen)); + arrayAppend(_stdout, slice(chunk, 0, truncatedLen)); ex = new ERR_CHILD_PROCESS_STDIO_MAXBUFFER('stdout'); kill(); } else { - ArrayPrototypePush(_stdout, chunk); + arrayAppend(_stdout, chunk); } }); } @@ -496,7 +495,7 @@ function execFile(file, args, options, callback) { child.stderr.on('data', function onChildStderr(chunk) { // Do not need to count the length if (options.maxBuffer === Infinity) { - ArrayPrototypePush(_stderr, chunk); + arrayAppend(_stderr, chunk); return; } const encoding = child.stderr.readableEncoding; @@ -507,13 +506,12 @@ function execFile(file, args, options, callback) { if (stderrLen > options.maxBuffer) { const truncatedLen = options.maxBuffer - (stderrLen - length); - ArrayPrototypePush(_stderr, - chunk.slice(0, truncatedLen)); + arrayAppend(_stderr, chunk.slice(0, truncatedLen)); ex = new ERR_CHILD_PROCESS_STDIO_MAXBUFFER('stderr'); kill(); } else { - ArrayPrototypePush(_stderr, chunk); + arrayAppend(_stderr, chunk); } }); } @@ -691,11 +689,14 @@ function normalizeSpawnArguments(file, args, options) { } } - if (typeof options.argv0 === 'string') { - ArrayPrototypeUnshift(args, options.argv0); - } else { - ArrayPrototypeUnshift(args, file); + // Not %Array.prototype.unshift%: it shifts the existing elements up by + // assigning them, which a userland index accessor on %Array.prototype% can + // swallow, silently dropping arguments from the spawned command. + const spawnArgs = [typeof options.argv0 === 'string' ? options.argv0 : file]; + for (let i = 0; i < args.length; i++) { + arrayAppend(spawnArgs, args[i]); } + args = spawnArgs; // Shallow copy to guarantee changes won't impact process.env const env = options.env || { ...process.env }; @@ -725,7 +726,7 @@ function normalizeSpawnArguments(file, args, options) { let envKeys = []; // Prototype values are intentionally included. for (const key in env) { - ArrayPrototypePush(envKeys, key); + arrayAppend(envKeys, key); } if (process.platform === 'win32') { @@ -750,7 +751,7 @@ function normalizeSpawnArguments(file, args, options) { if (value !== undefined) { validateArgumentNullCheck(key, `options.env['${key}']`); validateArgumentNullCheck(value, `options.env['${key}']`); - ArrayPrototypePush(envPairs, `${key}=${value}`); + arrayAppend(envPairs, `${key}=${value}`); } } diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index a12b2954db81..67342826750a 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -55,7 +55,7 @@ const { TTY } = internalBinding('tty_wrap'); const { UDP } = internalBinding('udp_wrap'); const SocketList = require('internal/socket_list'); const { owner_symbol } = require('internal/async_hooks').symbols; -const { convertToValidSignal } = require('internal/util'); +const { arrayAppend, convertToValidSignal } = require('internal/util'); const { isArrayBufferView } = require('internal/util/types'); const spawn_sync = internalBinding('spawn_sync'); const { kStateSymbol } = require('internal/dgram'); @@ -236,18 +236,18 @@ const handleConversion = { }; function stdioStringToArray(stdio, channel) { - const options = []; + let options; switch (stdio) { case 'ignore': case 'overlapped': - case 'pipe': ArrayPrototypePush(options, stdio, stdio, stdio); break; - case 'inherit': ArrayPrototypePush(options, 0, 1, 2); break; + case 'pipe': options = [stdio, stdio, stdio]; break; + case 'inherit': options = [0, 1, 2]; break; default: throw new ERR_INVALID_ARG_VALUE('stdio', stdio); } - if (channel) ArrayPrototypePush(options, channel); + if (channel) arrayAppend(options, channel); return options; } @@ -510,8 +510,8 @@ ChildProcess.prototype.spawn = function spawn(options) { this.stdio = []; for (i = 0; i < stdio.length; i++) - ArrayPrototypePush(this.stdio, - stdio[i].socket === undefined ? null : stdio[i].socket); + arrayAppend(this.stdio, + stdio[i].socket === undefined ? null : stdio[i].socket); // Add .send() method and start listening for IPC data if (ipc !== undefined) setupChannel(this, ipc, serialization); @@ -1020,7 +1020,7 @@ function getValidStdio(stdio, sync) { // Don't concat() a new Array() because it would be sparse, and // stdio.reduce() would skip the sparse elements of stdio. // See https://stackoverflow.com/a/5501711/3561 - while (stdio.length < 3) ArrayPrototypePush(stdio, undefined); + while (stdio.length < 3) arrayAppend(stdio, undefined); // Translate stdio into C++-readable form // (i.e. PipeWraps or fds) @@ -1036,7 +1036,7 @@ function getValidStdio(stdio, sync) { stdio ??= i < 3 ? 'pipe' : 'ignore'; if (stdio === 'ignore') { - ArrayPrototypePush(acc, { type: 'ignore' }); + arrayAppend(acc, { type: 'ignore' }); } else if (stdio === 'pipe' || stdio === 'overlapped' || (typeof stdio === 'number' && stdio < 0)) { const a = { @@ -1048,7 +1048,7 @@ function getValidStdio(stdio, sync) { if (!sync) a.handle = new Pipe(PipeConstants.SOCKET); - ArrayPrototypePush(acc, a); + arrayAppend(acc, a); } else if (stdio === 'ipc') { if (sync || ipc !== undefined) { // Cleanup previously created pipes @@ -1062,18 +1062,18 @@ function getValidStdio(stdio, sync) { ipc = new Pipe(PipeConstants.IPC); ipcFd = i; - ArrayPrototypePush(acc, { + arrayAppend(acc, { type: 'pipe', handle: ipc, ipc: true, }); } else if (stdio === 'inherit') { - ArrayPrototypePush(acc, { + arrayAppend(acc, { type: 'inherit', fd: i, }); } else if (typeof stdio === 'number' || typeof stdio.fd === 'number') { - ArrayPrototypePush(acc, { + arrayAppend(acc, { type: 'fd', fd: typeof stdio === 'number' ? stdio : stdio.fd, }); @@ -1083,7 +1083,7 @@ function getValidStdio(stdio, sync) { stdio : getHandleWrapType(stdio.handle) ? stdio.handle : stdio._handle; - ArrayPrototypePush(acc, { + arrayAppend(acc, { type: 'wrap', wrapType: getHandleWrapType(handle), handle: handle, diff --git a/lib/internal/util.js b/lib/internal/util.js index 2f72e636ab90..6c4102589e26 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -925,8 +925,28 @@ function assignFunctionName(name, fn, descriptor = kEmptyObject) { }); } +/** + * Appends a value to an array without going through the prototype chain. + * %Array.prototype.push% assigns the element, so an index accessor installed on + * %Array.prototype% by userland code can swallow the value and leave a hole + * behind. Use this where such a hole would be observable by the C++ layer or + * would silently corrupt data. Refs: https://github.com/nodejs/node/issues/56531 + * @param {any[]} array + * @param {any} value + */ +function arrayAppend(array, value) { + ObjectDefineProperty(array, array.length, { + __proto__: null, + value, + writable: true, + enumerable: true, + configurable: true, + }); +} + module.exports = { getLazy, + arrayAppend, assertCrypto, assertTypeScript, assignFunctionName, diff --git a/test/parallel/test-child-process-array-prototype-index-accessor.js b/test/parallel/test-child-process-array-prototype-index-accessor.js new file mode 100644 index 000000000000..38329888018a --- /dev/null +++ b/test/parallel/test-child-process-array-prototype-index-accessor.js @@ -0,0 +1,66 @@ +'use strict'; +// Regression test for https://github.com/nodejs/node/issues/56531. +// +// Userland can install an index accessor on %Array.prototype% (for example +// `Object.defineProperty(Array.prototype, '2', { set() {} })`). Any subsequent +// `push()`/`unshift()` targeting that index assigns through the prototype +// chain, so the value is swallowed and a hole is left behind. child_process +// used to build its stdio descriptor list, its argument list and its +// environment that way, which lost arguments and environment entries and made +// the C++ layer read an `undefined` stdio descriptor. + +require('../common'); +const assert = require('assert'); +const { spawnSync } = require('child_process'); + +// The pollution has to happen in a separate process: it would otherwise break +// the test runner itself. +if (process.argv[2] === 'child') { + const index = process.argv[3]; + Object.defineProperty(Array.prototype, index, { + get() { return undefined; }, + set() {}, + }); + + const cp = require('child_process'); + const printOk = ['-e', 'process.stdout.write("ok")']; + const opts = { encoding: 'utf8' }; + + if (cp.execFileSync(process.execPath, printOk, opts) !== 'ok') { + process.exit(1); + } + + if (cp.spawnSync(process.execPath, printOk, opts).stdout !== 'ok') { + process.exit(2); + } + + // Four entries so that a swallowed one is observable at every tested index. + const env = { A: 'a', B: 'b', C: 'c', D: 'd' }; + const printEnv = [ + '-e', 'const { A, B, C, D } = process.env;' + + 'process.stdout.write(A + B + C + D);', + ]; + if (cp.execFileSync(process.execPath, printEnv, { ...opts, env }) !== 'abcd') { + process.exit(3); + } + + // Goes through the shell, i.e. through `[shell, '-c', command]`. + cp.exec('echo ok', opts, (err, stdout) => { + process.exit(err || stdout.trim() !== 'ok' ? 4 : 0); + }); + return; +} + +// `[shell, '-c', command]` and the three default stdio descriptors mean the +// interesting indices are 0 to 3. +for (let index = 0; index <= 3; index++) { + const result = spawnSync(process.execPath, [__filename, 'child', `${index}`], { + encoding: 'utf8', + }); + assert.strictEqual( + result.status, + 0, + `Array.prototype[${index}] accessor: exited with ${result.status}, ` + + `signal ${result.signal}\n${result.stderr}`, + ); +}