child_process: don't drop stdio and args on array pollution by hammad-iftikhar · Pull Request #65544 · nodejs/node

hammad-iftikhar · 2026-08-25T13:17:03Z

Problem

Userland can install an index accessor on Array.prototype:

Object.defineProperty(Array.prototype, '2', { set() {} });

Array.prototype.push() and Array.prototype.unshift() write their elements
with Set, which walks the prototype chain. When the target index has an
inherited accessor, the setter runs, the value is silently discarded, and only
length is updated leaving a hole behind.

child_process built three separate lists that way:

the stdio descriptor list: stdioStringToArray(), getValidStdio(),
ChildProcess.prototype.spawn();
the argument list: normalizeSpawnArguments(), via unshift();
envKeys / envPairs, plus the exec() stdout/stderr chunk buffers.

So the reproduction from #56531 lost the command it was told to run and handed
the C++ layer an undefined stdio descriptor:

const { exec } = require('child_process');
Object.defineProperty(Array.prototype, '2', { set() {} });
exec('pwd', (err, stdout) => { console.log(stdout); });

On the version reported in the issue this aborted the process:

FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal

On current main the abort is gone, but the bug is not ParseStdioOptions()
reads .type off the hole and the call fails with an internal-looking error:

TypeError: Cannot read properties of undefined (reading 'type')
    at ChildProcess.spawn (node:internal/child_process:408:28)
    at spawn (node:child_process:813:9)
    at Object.execFile (node:child_process:349:17)
    at exec (node:child_process:236:25)

The throw is not the worst case, though. Indices 0 - 2 also make
spawnSync('echo', ['ok']) run echo undefined and execSync() fail with
EINVAL, and at index 3 nothing throws at all, the child is simply spawned
with an environment variable missing. See the table below for the measured
behaviour at each index.

Fix

Add arrayAppend() to internal/util. It appends with
ObjectDefineProperty(array, array.length, …), which is a
[[DefineOwnProperty]] and therefore never consults the prototype chain:

function arrayAppend(array, value) {
  ObjectDefineProperty(array, array.length, {
    __proto__: null,
    value,
    writable: true,
    enumerable: true,
    configurable: true,
  });
}

It is used in child_process at the places where a hole would either reach the
C++ layer or silently corrupt data. stdioStringToArray() now uses array
literals (element definition, not assignment), and the two unshift() calls
in normalizeSpawnArguments() are replaced by building the array
front-to-back.

How to reproduce and verify

1. The issue's original reproduction.

// poc.js
const { exec } = require('child_process');
Object.defineProperty(Array.prototype, '2', { set() {} });
exec('pwd', (err, stdout) => { console.log(err || stdout); });

$ ./node poc.js   # before
TypeError: Cannot read properties of undefined (reading 'type')

$ ./node poc.js   # after
/path/to/cwd

2. Sweep every affected index. Indices 0 - 3 are the interesting range:
three default stdio descriptors, and [shell, '-c', command] for the shell
path.

// probe.js — run as: ./node probe.js <index>
Object.defineProperty(Array.prototype, process.argv[2], { set() {} });
const cp = require('child_process');
const out = (label, fn) => {
  try { console.log(`  ${label}: ${JSON.stringify(fn())}`); }
  catch (e) { console.log(`  ${label}: THREW ${e.message}`); }
};
out('execSync("echo ok")', () => String(cp.execSync('echo ok')));
out('spawnSync("echo",["ok"])', () => String(cp.spawnSync('echo', ['ok']).stdout));
out('env round-trip', () => String(cp.execFileSync(process.execPath,
  ['-e', 'process.stdout.write(`${process.env.A}${process.env.B}${process.env.C}${process.env.D}`)'],
  { env: { A: 'a', B: 'b', C: 'c', D: 'd' } })));
cp.exec('echo ok', (e, so) =>
  console.log(`  exec("echo ok"): ${e ? 'THREW ' + e.message : JSON.stringify(so)}`));

Before, on main @ 54b4e372f39:

index	`execSync("echo ok")`	`spawnSync("echo", ["ok"])`	env round-trip	`exec("echo ok")`
0	`THREW spawnSync /bin/sh EINVAL`	`"undefined"`	`THREW spawnSync … EINVAL`	`THREW Cannot read properties of undefined (reading 'type')`
1	`THREW spawnSync /bin/sh EINVAL`	`"undefined"`	`THREW spawnSync … EINVAL`	`THREW Cannot read properties of undefined (reading 'type')`
2	`THREW spawnSync /bin/sh EINVAL`	`"undefined"`	`THREW spawnSync … EINVAL`	`THREW Cannot read properties of undefined (reading 'type')`
3	`"ok\n"`	`"ok\n"`	`"abcundefined"`	`"ok\n"`

Index 3 is worth calling out separately: nothing throws, nothing looks wrong,
and the child just silently runs with D missing from its environment.

After:

index	`execSync("echo ok")`	`spawnSync("echo", ["ok"])`	env round-trip	`exec("echo ok")`
0	`"ok\n"`	`"ok\n"`	`"abcd"`	`"ok\n"`
1	`"ok\n"`	`"ok\n"`	`"abcd"`	`"ok\n"`
2	`"ok\n"`	`"ok\n"`	`"abcd"`	`"ok\n"`
3	`"ok\n"`	`"ok\n"`	`"abcd"`	`"ok\n"`

3. The regression test.

$ ./node test/parallel/test-child-process-array-prototype-index-accessor.js

test/parallel/test-child-process-array-prototype-index-accessor.js installs
the accessor at each of indices 0 - 3 in a child process, then checks
exec(), execFileSync() and spawnSync() return the expected stdout and
that a four-entry env round-trips intact. The pollution has to happen in a
subprocess, it breaks the test runner itself otherwise.

Confirmed red before the change:

AssertionError [ERR_ASSERTION]: Array.prototype[0] accessor: exited with 1, signal null

and green after.

Test results

Built Release on macOS 15.5 / arm64 (Apple silicon).

All tests passed

Fixes: #56531

An index accessor installed on %Array.prototype% by userland code makes `push()` and `unshift()` assign through the prototype chain, so the value is swallowed and a hole is left behind. child_process built its stdio descriptor list, its argument list and its environment pairs that way, so `Object.defineProperty(Array.prototype, '2', { set() {} })` was enough to drop the command passed to `exec()`, lose an environment entry, and hand the C++ layer an `undefined` stdio descriptor - which used to abort with `FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal` and more recently threw `TypeError: Cannot read properties of undefined (reading 'type')`. Add an `arrayAppend()` helper to `internal/util` that defines the element instead of assigning it, and use it where a hole would reach the C++ layer or silently corrupt data. Fixes: nodejs#56531

Renegade334 · 2026-08-25T13:43:41Z

Userland can install an index accessor on Array.prototype:
Object.defineProperty(Array.prototype, '2', { set() {} });

Userland can indeed define such a property, at which point all bets are off and something crashing is pretty much inevitable.

It is not reasonable to desire that the Node.js environment will continue to function in this case, nor is it reasonable to add overhead to mitigate against it.

hammad-iftikhar · 2026-08-25T14:02:16Z

Thanks!
The overhead concern is fair, and I'd rather answer it with numbers
than with argument.

On "something crashing is pretty much inevitable"

A crash isn't what actually happens in most of these cases, and that's my
reason for filing. With Array.prototype['3'] defined, nothing throws at all —
child_process spawns the child with an environment variable silently missing:

env round-trip, main @ 54b4e372f39:  "abcundefined"

At indices 0-2, spawnSync('echo', ['ok']) runs echo undefined, wrong
argv, no error raised. A process-spawning API quietly executing something other
than what it was asked to execute feels like a different category from "the
process crashed", which is at least self-announcing.

On precedent

Core already asserts this exact robustness, including for index accessors on
Array.prototype specifically. test/parallel/test-repl-array-prototype-tempering.js
installs

Object.defineProperty(Array.prototype, "-1", { get() { return this[this.length - 1]; } });

and asserts the REPL keeps working rather than crashing. And
test/parallel/test-child-process-prototype-tampering.mjs tampers
Object.prototype.cwd and asserts exec / execSync / execFile / spawn
still return correct results, in this very module.

So I read the existing policy as "core defends against this", and this PR as
filling a gap in it rather than proposing a new obligation. If that reading is
wrong I'd genuinely like to know, because doc/contributing/primordials.md
reads the same way to me.

On overhead

You're right that the version I pushed isn't free. Measured on this branch,
macOS 15.5 / arm64:

	3-element stdio list	64-entry envPairs
`push`	68 ns	29.9 µs
`ObjectDefineProperty`	649 ns	46.0 µs

The env loop is the only one with unbounded N, and 16 µs is ~1.6% of a
spawnSync (~1032 µs for /usr/bin/true on this machine).

That is avoidable. Pushing and repairing only when the prototype actually
swallowed the write costs almost nothing on the untampered path:

	64-entry envPairs
`push`	30334 ns
push + `hasOwn` check, repair if swallowed	30667 ns (+1.1%)
unconditional `ObjectDefineProperty`	46278 ns (+52.6%)

+332 ns on a ~1 ms spawn is roughly 0.03%. I'm happy to switch the PR to that
shape if the objection is cost rather than principle.

(These are all from one machine; the absolute numbers will move elsewhere, but
the ratio between the two mitigations should hold.)

If it's principle

That's your call and I won't relitigate it. In that case, would you prefer:

(a) narrowing this to just the stdio descriptor list — the part that
produced the original FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal in the
report; or

(b) closing this, and taking a docs patch to primordials.md instead,
noting that ArrayPrototypePush is not safe against index accessors on
Array.prototype, so future contributors don't assume it is?

Either is fine by me.

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Aug 25, 2026

Navigation Menu

Uh oh!

Uh oh!

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

child_process: don't drop stdio and args on array pollution#65544
hammad-iftikhar wants to merge 1 commit into
nodejs:mainfrom
hammad-iftikhar:child-process-array-prototype-accessor

hammad-iftikhar commented Aug 25, 2026

Uh oh!

Renegade334 commented Aug 25, 2026

Uh oh!

hammad-iftikhar commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

hammad-iftikhar commented Aug 25, 2026

Problem

Fix

How to reproduce and verify

Test results

Uh oh!

Renegade334 commented Aug 25, 2026

Uh oh!

hammad-iftikhar commented Aug 25, 2026

On "something crashing is pretty much inevitable"

On precedent

On overhead

If it's principle

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants