spawn: don't close caller-owned fds passed as extra stdio#29606
Conversation
When a raw fd number is passed in stdio[3+], the descriptor belongs to the caller. #29416 made finalizeStreams unconditionally close every entry in stdio_pipes, which now includes that caller-owned fd — so once the Subprocess is GC'd, the parent's fd is closed out from under it and the number gets recycled for something else. Store bun.invalid_fd in the slot instead (matching the IPC-neutralization pattern from the same PR). finalizeStreams and getStdio already skip invalid entries, and index alignment with the options array is preserved for the IPC channel lookup. Windows already records .unavailable for this case and was unaffected.
|
Updated 3:05 PM PT - Apr 22nd, 2026
❌ @cirospaciari, your commit 8bc5485 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 29606That installs a local version of the PR into your bun-29606 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughspawnProcessPosix now marks non-owned extra stdio slots with a sentinel ( Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Follow-up from review: - .inherit/.ignore/.path arms of the extra_fds loop now also append bun.invalid_fd so spawned.extra_pipes is 1:1 with options.extra_fds, matching Windows. Fixes an OOB read of the IPC fd when a non-pipe extra slot precedes the IPC channel. - Subprocess.stdio type widened to (number | null)[] for slots >=3 since raw-fd / inherit / ignore / path inputs now surface as null (Node.js semantics; Windows already did this).
There was a problem hiding this comment.
LGTM — all prior feedback addressed (type updated, .inherit/.ignore/.path arms now aligned, test uses await using).
Extended reasoning...
Overview
This PR fixes a regression from #29416 where finalizeStreams would close caller-owned fds passed as stdio[N] (N≥3). The fix stores bun.invalid_fd as a sentinel in extra_pipes for slots that Bun does not own (.pipe, .inherit, .ignore, .path), so finalizeStreams and getStdio skip them. The change touches spawnProcessPosix in process.zig (~6 lines), updates the published Subprocess.stdio type in bun.d.ts to ...(number | null)[], and adds two regression tests plus a type-fixture update.
Security risks
None. The change narrows what Bun closes — it stops closing descriptors it doesn't own, which is strictly safer than the prior behavior. No new fds are opened or exposed; the sentinel is never passed to a syscall.
Level of scrutiny
Moderate — process spawning is load-bearing, but the edit is mechanical and mirrors the Windows path, which already records .unavailable for these slots. I verified the two downstream consumers: finalizeStreams (subprocess.zig:752) and getStdio (subprocess.zig:488-491) both branch on item.isValid() and skip the sentinel, and the IPC lookup at js_bun_spawn_bindings.zig:681/798 now indexes correctly because every options.extra_fds arm appends exactly one entry to the result array.
Other factors
I left three comments on earlier revisions; all have been addressed: the bun.d.ts type was widened (with the fixture updated), the .inherit/.ignore/.path arms now also append the sentinel (with a new IPC alignment test in spawn.ipc.test.ts), and the IPC test was switched to await using so the child is reaped on failure. The new spawn.test.ts case directly exercises the original bug (fd reused across 8 spawns + GC). No bugs flagged by the bug-hunting system on the current revision.
) ## What does this PR do? When a raw fd number is passed as `stdio[N]` (N≥3), that descriptor is owned by the caller, not by `Bun.spawn`. oven-sh#29416 made `finalizeStreams` unconditionally close every entry in `stdio_pipes`, which on POSIX includes the caller's fd — so after the `Subprocess` is GC'd, the parent's fd is closed and the number gets recycled. Callers that cache an fd and reuse it across many spawns (e.g. a self-exec handle passed as `stdio[3]`) start seeing `EACCES`/`EBADF` once the first batch of subprocesses is collected. Fix: store `bun.invalid_fd` in the `extra_pipes` slot for the `.pipe` case instead of the caller's fd. `finalizeStreams` and `getStdio` already skip invalid entries (added in oven-sh#29416 for the IPC slot), and this keeps index alignment for the IPC channel lookup. Windows already records `.unavailable` here and was unaffected. ## How did you verify your code works? - [ ] New test in `test/js/bun/spawn/spawn.test.ts` — opens an fd, passes it as `stdio[3]` across 8 spawns, GCs, then asserts `fstatSync(fd)` still succeeds and the fd is still usable as `stdio[3]` in a fresh spawn - [ ] `bun bd test test/js/bun/spawn/spawn.test.ts`
) ## What does this PR do? When a raw fd number is passed as `stdio[N]` (N≥3), that descriptor is owned by the caller, not by `Bun.spawn`. oven-sh#29416 made `finalizeStreams` unconditionally close every entry in `stdio_pipes`, which on POSIX includes the caller's fd — so after the `Subprocess` is GC'd, the parent's fd is closed and the number gets recycled. Callers that cache an fd and reuse it across many spawns (e.g. a self-exec handle passed as `stdio[3]`) start seeing `EACCES`/`EBADF` once the first batch of subprocesses is collected. Fix: store `bun.invalid_fd` in the `extra_pipes` slot for the `.pipe` case instead of the caller's fd. `finalizeStreams` and `getStdio` already skip invalid entries (added in oven-sh#29416 for the IPC slot), and this keeps index alignment for the IPC channel lookup. Windows already records `.unavailable` here and was unaffected. ## How did you verify your code works? - [ ] New test in `test/js/bun/spawn/spawn.test.ts` — opens an fd, passes it as `stdio[3]` across 8 spawns, GCs, then asserts `fstatSync(fd)` still succeeds and the fd is still usable as `stdio[3]` in a fresh spawn - [ ] `bun bd test test/js/bun/spawn/spawn.test.ts`
3 participants
What does this PR do?
When a raw fd number is passed as
stdio[N](N≥3), that descriptor is owned by the caller, not byBun.spawn. #29416 madefinalizeStreamsunconditionally close every entry instdio_pipes, which on POSIX includes the caller's fd — so after theSubprocessis GC'd, the parent's fd is closed and the number gets recycled. Callers that cache an fd and reuse it across many spawns (e.g. a self-exec handle passed asstdio[3]) start seeingEACCES/EBADFonce the first batch of subprocesses is collected.Fix: store
bun.invalid_fdin theextra_pipesslot for the.pipecase instead of the caller's fd.finalizeStreamsandgetStdioalready skip invalid entries (added in #29416 for the IPC slot), and this keeps index alignment for the IPC channel lookup. Windows already records.unavailablehere and was unaffected.How did you verify your code works?
test/js/bun/spawn/spawn.test.ts— opens an fd, passes it asstdio[3]across 8 spawns, GCs, then assertsfstatSync(fd)still succeeds and the fd is still usable asstdio[3]in a fresh spawnbun bd test test/js/bun/spawn/spawn.test.ts