child_process: don't drop stdio and args on array pollution

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}`);
     }
   }
 

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,

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,

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}`,
+  );
+}