Repository navigation
child_process: match node's option validation for uid, gid, timeout, maxBuffer and windowsHide - #44967
child_process: match node's option validation for uid, gid, timeout, maxBuffer and windowsHide#44967robobun wants to merge 1 commit into
Conversation
…maxBuffer and windowsHide normalizeSpawnArguments, validateTimeout and validateMaxBuffer carried the checks that node removed in nodejs/node#59416 (v25.0.0). Each check built one error for every bad value, so the error did not match node 26: - uid and gid: NaN, a fraction, Infinity or a value outside int32 threw ERR_INVALID_ARG_TYPE. A BigInt or a Symbol threw a TypeError with no code. - timeout and maxBuffer: a value that is not a number threw ERR_OUT_OF_RANGE. The three checks now call validateInt32, validateInteger and validateNumber from internal/validators, as node does. The error class and code now match node. The message matches too, except for the "Received ..." text that the shared error formatter prints for some unusual values. Two more differences on the same path: - execFile() now passes `!!options.windowsHide` and `!!options.windowsVerbatimArguments` to spawn(), as node does. exec() and execFile() threw for a value that is not a boolean. Node accepts it. - A null byte in `cwd` or in the module path of fork() now gives node's text: "must be a string, Uint8Array, or URL without null bytes". The four node tests that pin the uid, gid, timeout and maxBuffer errors are replaced with their v26.3.0 versions.
|
Updated 2:28 AM PT - Oct 11th, 2026
✅ @robobun, your commit 16b6b4ba0a2152215de6a04f572ab5540345395b passed in 🧪 To try this PR locally: bunx bun-pr 44967That installs a local version of the PR into your bun-44967 --bun |
|
Status Reproduced on bun 1.4.3-canary (367d939), Linux x64, against node 26.3.0: const cp = require("child_process");
for (const options of [{ uid: NaN }, { gid: 1.5 }, { timeout: "1" }, { maxBuffer: "1" }]) {
try {
cp.spawnSync(process.execPath, ["-e", "0"], options);
} catch (e) {
console.log(e.name, e.code);
}
}Node prints |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughChild-process option validation now uses shared validators for UID, GID, timeout, and maxBuffer. Exec option coercion and null-byte error text changed. Tests cover validation results, error types, accepted values, and validation order. ChangesChild process validation
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete issue remains that would block merging after normal checks. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly summarizes the main change: aligning child_process option validation with Node for uid, gid, timeout, maxBuffer, and windowsHide. |
| Description check | |
The description is detailed and directly addresses the change, implementation, verification results, limitations, and related pull requests. It does not use the exact template headings, but it provide… |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
One part of this review's analysis that stopped early was run once more. It finished.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/child_process.ts— Windows callers running a .bat or .cmd file with an invalid timeout or maxBuffer still get a silentresult.errorEINVAL instead of node's thrown TypeError, so the error contract this PR advertises is not delivered to them. spawnSync returns early onoptions.windowsBatchFileErrorat src/js/node/child_process.ts:531-549 before validateTimeout at :557 and validateMaxBuffer at :560 run. Node validates timeout and maxBuffer in JS before the batch-file EINVAL is produced, so node throws. Fix: run validateTimeout and validateMaxBuffer before the windowsBatchFileError early return in spawnSync, and check the same ordering in the other sync entry points.Why this was flagged
On Windows, spawnSync("x.bat", { timeout: "x" }) or { maxBuffer: "x" } without shell. normalizeSpawnArguments sets options.windowsBatchFileError and spawnSync returns the EINVAL SystemError at src/js/node/child_process.ts:531-549 before validateTimeout (:557) and validateMaxBuffer (:560) are reached, so the caller gets { error: EINVAL, status: null } rather than a thrown ERR_INVALID_ARG_TYPE. In node the JS validators run before the spawn_sync binding returns EINVAL for a batch file, so node throws. The async spawn path does throw windowsBatchFileError at :162 before validateTimeout at :163 as well. The ordering predates the diff, but the PR's stated goal is to match node's error contract for timeout and maxBuffer on all seven functions, and this Windows population does not get it; the new tests use bunExe() (.exe) so they never exercise it. Remedy: validate before the batch-file early return.
Verification: pre-existing. Triggering condition: on Windows, spawnSync() called with a
.bat/.cmdfile, noshell, and an invalidtimeoutormaxBuffer. In src/js/node/child_process.ts, spawnSync returns the EINVAL result at :531-549 beforevalidateTimeoutat :557 andvalidateMaxBufferat :560 execute. The diff has no hunk touching :531-560, so the base already masks the validation error by the same route.
| it("exec() and execFile() coerce windowsHide and windowsVerbatimArguments, the other functions validate them", async () => { | ||
| const options: any = { windowsHide: "yes", windowsVerbatimArguments: 1 }; | ||
| // node's execFile() passes `!!options.windowsHide` to spawn(), so these two run the child. | ||
| expect(await promisify(execFile)(bunExe(), ["-e", "0"], options)).toEqual({ stdout: "", stderr: "" }); |
There was a problem hiding this comment.
🟡 nit (optional): maintainers get a test that can fail on CI agents where ad-hoc JSC or BUN_DEBUG_* env vars are set, instead of a reliable assertion. The new test at test/js/node/child_process/child_process.test.ts:1587 runs a Bun child through execFile(bunExe(), ...) without env: bunEnv and asserts stderr is exactly "". Fix: spread bunEnv into the options of every bunExe() child in the new block (lines 1351-1354 and 1587), as test/CLAUDE.md requires, so stderr stays empty regardless of the agent's environment.
Why this was flagged
The test at test/js/node/child_process/child_process.test.ts:1587 calls promisify(execFile)(bunExe(), ["-e", "0"], { windowsHide: "yes", windowsVerbatimArguments: 1 }) with no env, so the child inherits the runner's process.env. test/harness.ts:66-72 documents that bunEnv strips JSC_useJIT and sets BUN_DEBUG_QUIET_LOGS because those leak a "WARNING: failed to parse" or debug line to stderr on CI agents. With the inherited env the child can print to stderr, and the assertion toEqual({ stdout: "", stderr: "" }) fails. test/CLAUDE.md says to use bunExe and bunEnv together when spawning Bun; the sibling table at lines 1351-1354 has the same omission, though those children are killed or throw before output is asserted. The base branch has no such test, so this is a new flake source rather than a runtime defect.
Verification: nit — triggers when the runner's environment lacks BUN_DEBUG_QUIET_LOGS=1 or carries an ad-hoc BUN_DEBUG_/JSC_* variable. The new test at test/js/node/child_process/child_process.test.ts:1587 asserts stderr: "" on an execFile(bunExe()) child with no env, so it inherits the runner's environment and visible debug-build loggers print. The CI runner does NOT strip BUN_DEBUG_/JSC_ keys.
| if (timeout != null && !(NumberIsInteger(timeout) && timeout >= 0)) { | ||
| throw $ERR_OUT_OF_RANGE("timeout", "an unsigned integer", timeout); | ||
| if (timeout != null) { | ||
| validateInteger(timeout, "timeout", 0); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Callers who pass a timeout between 231 and 253-1 get a child killed at once or after the wrong delay, and the PR now certifies those values as valid. validateInteger at src/js/node/child_process.ts:1956 admits them, and spawnSync forwards them unchanged at :611. Bun.spawnSync then truncates with (timeout_int as u32) & 0x7FFF_FFFF (src/runtime/api/bun/js_bun_spawn_bindings.rs:764), so 231 becomes 0 and the child is killed immediately. Fix: either cap the accepted range at the value the backend honours or widen the binding to a 64-bit timer so every value the validator accepts is honoured; the new test pins 231 and 2**53-1 as accepted without checking the timer.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
spawnSync(cmd, { timeout: 2 ** 31 }) or any value in 231..253-1 from a user script. validateInteger(timeout, "timeout", 0) at src/js/node/child_process.ts:1956 accepts it (max defaults to maxSafeInteger, NodeValidator.cpp:52). spawnSync passes it to Bun.spawnSync at src/js/node/child_process.ts:611. The binding does validate_integer_range::<u64> then (timeout_int as u32) & 0x7FFF_FFFF at src/runtime/api/bun/js_bun_spawn_bindings.rs:762-766, so 231 -> Some(0) -> user_timespec = now (js_bun_spawn_bindings.rs:1797) -> the child is killed immediately; 232+5000 silently becomes 5000 ms. Node's spawnSync honours an int64 timeout. Async spawn hits setTimeout at :174 with the same values. The truncation predates the diff, but this PR is the one that defines the accepted range as 0..2**53-1 and adds a test (child_process.test.ts, expectAccepted("timeout", [..., 2 ** 31, 2 ** 53 - 1, ...])) that only checks ENOENT, so the broken range is now pinned as correct. The remedy is to make the accepted range and the honoured range the same.
Verification: pre-existing — triggered by spawnSync with timeout in [231, 253-1]. src/js/node/child_process.ts:1956 admits it; src/runtime/api/bun/js_bun_spawn_bindings.rs:764 masks & 0x7FFF_FFFF, so 2**31 becomes 0 and the child is killed at once. The base validator had no upper bound and fails identically. The new test at test/js/node/child_process/child_process.test.ts:1570 pins these as accepted.
This branch has not been deployed
1 participant
Supersedes #33716.
Problem
node:child_processthrows the wrong error for a baduid,gid,timeoutormaxBuffer.spawnSync(cmd, { uid: NaN })throwsTypeError [ERR_INVALID_ARG_TYPE]: The "options.uid" property must be of type int32. Node 26.3.0 throwsRangeError [ERR_OUT_OF_RANGE].timeout: "1"andmaxBuffer: "1"throwERR_OUT_OF_RANGE. Node throwsERR_INVALID_ARG_TYPE.execFile(f, { windowsHide: 1 })throws. Node runs the child.src/js/node/child_process.ts(:989,:1935,:1953) that node replaced in lib: use validators for argument validation nodejs/node#59416 (v25.0.0). No user reported this.Fix
validateInt32,validateIntegerandvalidateNumber, as node'slib/child_process.jsdoes. Class and code now match node.execFilepasses!!options.windowsHideand!!options.windowsVerbatimArgumentstospawn, as node does. A null byte incwdgives node's message.test/js/node/child_process/child_process.test.tsfail on 1.4.3-canary and pass here. The four node tests for these errors are now node's v26.3.0 files.Background
child_process.ts, before a process exists.internal/validatorsholds Bun's native ports of node's validators (src/jsc/bindings/NodeValidator.cpp).uidandgidonly:timeoutandmaxBufferstay wrong, and one node test matches no node version.Downsides
timeoutabove 2^53 - 1 throws. Before, a timer killed the child at once, orspawnSyncreturnedresult.error.execandexecFilemake 3 native validator calls (2Number.isIntegercalls before), the sync functions 1 (0 before). Allocations: 0.Notes
Errors, node 26.3.0 and Bun. Each of the seven functions (
spawn,spawnSync,exec,execSync,execFile,execFileSync,fork) gives the same result unless a row says otherwise.gidbehaves likeuid. "Before" is bun 1.4.3-canary (367d939). After this change Bun prints the node column.maxBufferis an option ofexec,execFileand the three sync functions only. ThewindowsHiderow is the same forwindowsVerbatimArguments. The five other functions still reject a non-boolean there, as node does. Thecwdtext is also the text for the module path offork.Scope of "matches node". The error class and code match node for every value I ran. The message text matches for numbers and for the values in the table. The
Received ...part comes from the shared formatter (determineSpecificType,src/jsc/bindings/ErrorCode.cpp). It still differs from node for a few unusual values: a string that has'and a control character, a long string with a late', an object whoseconstructorhas noname, a bound function. #38642 and #38473 change that formatter.Measurements (Linux x64, debug build of this branch, node 26.3.0 as the reference):
uid,gid,timeoutandmaxBuffer(7 functions x 19uidvalues, the same forgid, 7 x 15timeoutvalues, 5 x 11maxBuffervalues, 4 rows with a countingvalueOf, 5 rows with two bad options). 436 rows differ from node before. 0 rows differ after.timeout: 2 ** 53onspawn,execFile,execandfork. No other accepted value is rejected now. The new tests pin the accepted limits:-(2 ** 31)and2 ** 31 - 1for the ids,2 ** 53 - 1fortimeout.2353e7594f35,f7554e4484b4,b777390156be,27df4e678570. The two*-timeout-kill-signal.jsfiles also take node's switch toassert.andevents.listenerCount.bun run lint: 0 errors.tsc --noEmit -p src/js/tsconfig.json: exit 0. The 32 new cases also pass withBUN_JSC_validateExceptionChecks=1.test/js/node/child_process/child_process.test.tswith--timeout 120000: 115 pass, 1 fail. The failure isspawn still works with more than 10240 fds open. It fails on the unfixed release build here too, because the hard fd limit of my container is 4096. With the default timeout, some older cases that start several debug-build children exceed 5 s on my machine.Cost for callers with valid options.
spawnandforkwith nouid,gidortimeout: 0 before, 0 after.execandexecFile(defaultstimeout: 0,maxBuffer: 1048576): 2validateIntegerand 1validateNumber, where they made 2Number.isIntegercalls.spawnSync,execFileSyncandexecSync: 1validateNumber, 0 before. A validuidorgid: 1validateInt32each, which returns at the int32 tag check.BUN_JSC_logGC=1logs 0 collections during 3,000,000 accepted calls of each validator with these argument shapes (reached throughEventEmitter#setMaxListeners,TLSSocket#setMaxSendFragmentandstream.setDefaultHighWaterMarkon the release build). A control that allocates one object per call logs 49.node:child_processshrinks by 284 bytes (40610 to 40326, production output ofsrc/codegen/bundle-modules.ts). No native code changes, and no host function is added.perf,valgrindandstraceare not installed where I run, so I have no instruction or syscall counts.Still different from node, not changed here.
uid: -0andgid: -0are accepted and mean id 0. Node'svalidateInt32accepts-0too. Node then rejects it in C++:spawnSyncthrowsoptions.uid must be a 32-bit signed integer(src/spawn_sync.cc:860), andspawnaborts the process (CHECK(uid_v->IsInt32()),src/process_wrap.cc:220). A test pins that-0behaves like0.timeoutfrom2 ** 31to2 ** 53 - 1passes the validator, as in node. The sync functions then reportETIMEDOUTat once, because the native parser ofBun.spawnSynckeeps only 31 bits of the value (src/runtime/api/bun/js_bun_spawn_bindings.rs:764). Node runs the child. spawn: honor timeouts of 2**31 ms or more instead of truncating them to 31 bits #37604 changes that parser.spawnSync(cmd, { maxBuffer: 0.5 })sets no limit. Node reportsENOBUFS. The same native parser truncates the value to 0 (js_bun_spawn_bindings.rs:785).new ChildProcess().spawn(options)skips these checks, as in node. Bun.spawn: validate uid and gid as node does instead of treating NaN as id 0 #39682 covers itsuidandgid.stdiovalue throwsERR_INVALID_OPT_VALUE. Node throwsERR_INVALID_ARG_VALUE(child_process: throw ERR_INVALID_ARG_VALUE for invalid stdio values #35226).spawnandforkof a.bator.cmdfile withoutshellthrowEINVALbefore they validatetimeout(read fromchild_process.ts:159, not run). Node validates first. child_process: match Node when spawnSync rejects a Windows batch file #43706 changes thespawnSyncside.execFilepassestimeoutandkillSignaltospawn, sovalidateTimeoutruns twice and two kill timers exist. Node'sexecFilepasses neither.lib/child_process.jsthat are not option validation:exec("exit 3")gives the messageCommand failed: exit 3with no trailing newline,env: { A: Symbol() }does not throw,execdoes not callmodule.exports.execFile, the promisifiedexechas no name, andshell: truewith arguments prints noDEP0190warning.Related pull requests.
timeout,maxBufferandwindowsHidechanges. A stale-PR sweep closed it with no maintainer comment. This pull request addsuid,gidand the fourth node test.validateMaxMemoryto this file, and that function callsNumberIsInteger. This change deletesNumberIsIntegerbecause nothing else uses it andoxlintreports an unused variable as an error. The two changes conflict inexecFile(both touch thewindowsHidelines), so the one that lands second needs a rebase. After that rebase,validateMaxMemorymust callNumber.isIntegeror a shared validator.*-timeout-kill-signal.jsfiles to node v24.14.1, which expects the oldERR_OUT_OF_RANGE.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/child_process/child_process.test.ts