spawn(windows): stop closing an exposed extra stdio pipe handle twice - #39966
Conversation
On Windows, Subprocess.stdio[i >= 3] exposed the HANDLE of the parent-side
uv_pipe_t. node:child_process hands that value to net.connect({ fd }), which
adopts it into a second uv_pipe_t and closes it with the socket. The
Subprocess still closed its own uv_pipe_t, and with it the same HANDLE
value, when it was finalized. By then the value usually belonged to another
object, which the second close destroyed.
The getter now hands out a duplicate of the handle, closes its own pipe at
once, and keeps the duplicate in the slot so repeated reads return the same
value. The duplicate is the caller's to close, like ExtraPipe::UnownedFd on
POSIX.
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 9 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes Windows HANDLE ownership in the spawn stdio path (dup + early uv_close, with EOF-visibility implications for the child), a human look would still be worthwhile.
What was reviewed:
- The new
UnownedFdarm is covered in every exhaustive match onWindowsStdioResult(spawn bindings sync-cleanup, shellPipeReader::create,StaticPipeWriter::create,finalize_streams,stop_for_vm_teardown). close_and_destroycallsopen_handles::removefirst, so the entryrecord_stdio_pipe_ownershipregistered is cleaned up andstop_for_vm_teardowncannot reach the freed pipe.core::mem::takeon the slot is sound (WindowsStdioResult: Default→Unavailable); repeat.stdioreads see the storedUnownedFdand return the same value.bun_sys::dupon Windows usesDuplicateHandle(bInheritHandle=FALSE), so the exposed duplicate is not inherited by later spawns.
Extended reasoning...
Overview
Adds WindowsStdioResult::UnownedFd(Fd) and, in Subprocess::get_stdio on Windows, converts each extra-stdio Buffer(Box<uv::Pipe>) slot into an UnownedFd holding a DuplicateHandle of the pipe's OS handle, then closes the original uv_pipe_t immediately via close_and_destroy. The duplicate's numeric value is what JS receives; finalize_streams, stop_for_vm_teardown, and the sync-spawn error-cleanup path all skip the new variant. Three other match sites gain the arm mechanically. Two Windows-only regression tests are added.
Security risks
None identified. This is a resource-ownership fix: it removes a stale CloseHandle on a value the kernel may have reissued to an unrelated object. bun_sys::dup creates the duplicate non-inheritable, so it does not leak into children spawned while it is open.
Level of scrutiny
High. This is Windows-specific handle-lifetime code in the spawn subsystem. The reasoning is subtle: closing Bun's uv_pipe_t immediately (rather than at finalize) is required so the child sees EOF once the caller closes the duplicate; the duplicate keeps the kernel object alive for net.connect({fd}). The exhaustive-match updates and the open_handles interaction check out, but the EOF/ordering argument and the intentional "caller owns the duplicate, we never close it" leak-on-abandon semantics (matching the POSIX UnownedFd from #33828) deserve a human confirmation.
Other factors
Maybe<Fd>iscore::result::Result, so theif let Ok(dup)pattern is correct; ondupfailure the slot staysUnavailableand reads asnull.close_and_destroyremoves the pipe from the thread's open-handles list beforeuv_close, so the ownership record set byrecord_stdio_pipe_ownershipdoes not go stale.- The two new tests are precise: the
spawn.test.tsone reoccupies the freed HANDLE value with a signaled event and asserts GC does not close it; thechild_process.test.tsone checks thenet.Socketwiring end-to-end and that files opened afterward survive GC. Both were verified failing on canary per the description. - No prior human reviews or outstanding comments on the PR.
…dle (#39969) ### Problem - On Windows `Fd::cwd()` returned the handle ntdll keeps in the PEB (`src/bun_core/util.rs:1047`). `process.chdir()` on any thread closes that handle and installs a new one, and Windows gives the closed value to the next object created. So an `Fd::cwd()` taken before a chdir is a stale snapshot afterwards: it no longer compares equal to `Fd::cwd()`, and closing it closes an unrelated object. - `Dir::drop` (`src/sys/dir.rs:17`) relies on that comparison to skip the cwd. `fs.rm` / `fs.rmdir` with `recursive` hold a `Dir::cwd()` for the whole walk on a thread pool thread (`src/runtime/node/node_fs.rs:7664`, `:7702`), and the transpiler cache holds one across every cache write on worker and pool threads (`src/jsc/RuntimeTranspilerCache.rs:871`). On the current canary, `fs.promises.rm` of 1500 files with a `process.chdir()` during the walk closes one of 1024 events created after the chdir, in 6 of 6 runs. - Found while investigating `failed to join on thread: ... (os error 6)` in `WebWorker::join` (Sentry BUN-4NEC, Windows, 1.4.0): a Worker's thread handle had been closed by other code in the process. A thread created after a chdir is one of the objects this close can hit. The report's tags (`transpiler_cache`, `workers_spawned`) fit, but one report cannot prove it was this path. #39966 fixed a second stale close found in the same investigation. ### Fix - `Fd::cwd()` on Windows is now a fixed value, the counterpart of `AT_FDCWD`. The value is bit 62 alone: handle values fit in 32 bits, bit 63 is the uv tag, and `INVALID_HANDLE_VALUE` masks to all of bits 0..63, so nothing else can produce it. `decode_windows()` maps it to the PEB handle at the time of each call, so every syscall still resolves against the live cwd, and `Display` prints it as `[cwd]`. - Because the value is constant, `Dir::drop`'s comparison is exact on every platform, and `Dir::cwd()` and its callers need no change. `Fd::close()` refuses the sentinel with EBADF, as `close(AT_FDCWD)` does on POSIX, instead of closing ntdll's handle. - Verified on a Windows x64 debug build: the new test in `test/js/node/fs/fs.test.ts` fails on the current canary (`rm closed 1 handle(s) it did not own`) and passes 4 of 4 runs here. On the same build: `fs.test.ts` (one disk-bound 4.9 GB write times out here, unrelated), `bun-write.test.js`, `transpiler-cache.test.ts`, `bun-link.test.ts`, `bun-pm.test.ts`, `bundler/cli.test.ts` pass, and relative fs operations after a `process.chdir()` resolve against the new cwd. ### Background - `bun_sys::Dir` (#30878) closes its fd on `Drop` unless the fd is `Fd::INVALID` or `Fd::cwd()`. On POSIX `Fd::cwd()` is the `AT_FDCWD` constant, so that check was exact there and only broke on Windows. - `Fd` on Windows packs a kind bit and a value. `decode_windows()` is the one place that turns a system-kind value into a HANDLE for `native()`, `close()` and friends, which is why the mapping lives there. The Windows `*at` wrappers already read the PEB handle per call for an invalid dirfd; this makes the cwd fd itself behave that way. - On Windows all kernel objects share one handle table per process and a freed value is reused at once, so a stale close destroys an unrelated object. `Fd::close` notices that only in debug builds. <details><summary>Notes</summary> - The first version of this PR made `Dir::cwd()` return `ManuallyDrop<Dir>` and rewrote two callers. Review asked for the cwd abstraction itself to be fixed so callers stay platform-uniform, which is what the sentinel does. - The test creates 1500 files, starts `fs.promises.rm` (thread pool), polls until the walk has started deleting, calls `process.chdir()`, creates 1024 signaled events (one of them receives the retired handle value), awaits the rm and checks that every event is still signaled. On the debug build the walk is detected with about 1480 files to go and takes about 140 ms, so the chdir lands inside it. About 0.85 s per run on the debug build. - The same comparison exists once more in `RuntimeTranspilerCache.rs:884-888` on a raw `Fd`; it is exact now as well. - `Fd::cwd()` is passed as a dirfd in about 230 places. All of them reach the handle through `native()`, so they see the live cwd as before. The only behavior changes are the stable comparison, the refused close and the `Display` output. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts <!-- robobun:evidence:end -->
3 participants
Problem
Subprocess.stdio[i]fori >= 3exposes the HANDLE of the parent-sideuv_pipe_t(src/runtime/api/bun/subprocess.rs,get_stdio).node:child_processpasses that value tonet.connect({ fd })(src/js/node/child_process.ts:1281), which adopts it into a seconduv_pipe_t(src/runtime/socket/Listener.rs:1195) and closes it with the socket. The Subprocess kept the firstuv_pipe_tand closed the same HANDLE value again infinalize_streams. Windows reuses a closed handle value at once, so the second close destroyed whatever owned the value by then.child_process.spawn(cmd, { stdio: ["pipe", "pipe", "pipe", "pipe"] })is enough to hit it (Playwright and ffmpeg launchers use such pipes).failed to join on thread: ... (os error 6)inWebWorker::join(Sentry BUN-4NEC, Windows, 1.4.0): the std thread handle of a Worker had been closed by someone else in the process. This stale close is one Bun-side way for that to happen. The tags of the report (spawn,workers_spawned) fit, but one report cannot prove it was this one.Fix
get_stdioon Windows duplicates eachBufferslot's handle withbun_sys::dup, closes its own pipe right away, and stores the duplicate as the newWindowsStdioResult::UnownedFd. Reads return the duplicate's value, andfinalize_streamsand the VM teardown path leave it alone. This is the Windows form of the POSIX downgrade toExtraPipe::UnownedFd.net.connectreads it as before.dupfails, the slot staysUnavailableand reads asnull. The three exhaustive matches onWindowsStdioResultgain the new arm.test/js/bun/spawn/spawn.test.tsandtest/js/node/child_process/child_process.test.tsfail 6 of 6 runs on the current canary and pass on this build.spawn.test.ts,child_process.test.tsand the spawn IPC tests pass on that build.Background
uv_pipe_towns its OS handle:uv_closeon it callsCloseHandle.Bun.connect({ fd })on Windows turns a HANDLE value into a CRT fd with_open_osfhandleand gives it to a newuv_pipe_t, so after that call twouv_pipe_towned one handle.CloseHandletherefore closes an unrelated object.Fd::closeasserts on this only in debug builds, and this path did not go through it at all.stdio_pipesholds the slots for indexes 3 and up.finalize_streams(GC) andstop_for_vm_teardown(a Worker exiting) close every slot that still holds a pipe.Notes
Listener.rsinterprets a{ fd }number as a CRT fd first and only then as a HANDLE, so a HANDLE value that happens to match an open CRT fd number is still adopted as the wrong object. Separate issue.Dir::cwd()(src/sys/dir.rs, Windows only, needs a concurrentprocess.chdir()) is handled separately.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/bun/spawn/spawn.test.ts test/js/node/child_process/child_process.test.ts