sys(windows): dup() of a descriptor that is not open fails with EBADF instead of returning a process handle - #37516
sys(windows): dup() of a descriptor that is not open fails with EBADF instead of returning a process handle#37516robobun wants to merge 1 commit into
Conversation
On Windows, an fd whose CRT index is not open (and Fd::INVALID) decodes to INVALID_HANDLE_VALUE, which is the same value as the GetCurrentProcess() pseudo handle. DuplicateHandle accepts that as a source, so dup() returned a handle to the current process instead of failing, and callers such as fetch() with a Bun.file(fd) body or Bun.file(fd).stream() only failed later with a misleading EMFILE. Reject it up front with EBADF, as POSIX does.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling 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 (3)
Comment |
|
Updated 7:20 AM PT - Aug 11th, 2026
✅ @robobun, your commit af1a583b1b19b8f346d0ea37db4079eda8742a66 passed in 🧪 To try this PR locally: bunx bun-pr 37516That installs a local version of the PR into your bun-37516 --bun |
|
Status: reproduced on Windows x64 with the release canary (da3851e): |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a three-line guard in Windows dup() that rejects INVALID_HANDLE_VALUE with EBADF before DuplicateHandle can misinterpret it as the current-process pseudo handle.
What was reviewed
- Verified
fd.native()returns*mut c_void(==w::HANDLE) on Windows, so dropping theas w::HANDLEcast and comparing directly tow::INVALID_HANDLE_VALUEis type-correct. - Error construction (
Error::new(E::EBADF, Tag::dup).with_fd(fd)) matches the existing pattern atsrc/sys/lib.rs:3791and the failure arm just below. - Tests follow harness conventions (tempDir, bunEnv, concurrent pipe drain, subprocess isolation for the closed-fd case) and cover never-opened / closed / still-open on both entry points.
Extended reasoning...
Overview
The PR adds a guard to the Windows implementation of bun_sys::dup() in src/sys/lib.rs: if fd.native() decodes to INVALID_HANDLE_VALUE, return EBADF (tagged Tag::dup, with the fd attached) instead of passing it to DuplicateHandle. It also hoists fd.native() into a local (dropping a redundant as w::HANDLE cast — FdNative on Windows is already *mut c_void) and adds the required SAFETY: comment above the unsafe FFI block. Two test files gain coverage for fetch() with a Bun.file(fd) body and Bun.file(fd).stream() on a never-opened fd, a closed fd (in a fresh subprocess so the number can't be reused), and — for the stream path — a still-open fd.
Security risks
None. The change tightens an error path: an invalid fd that previously produced a duplicated process handle (which then failed downstream at _open_osfhandle with a misleading EMFILE) now fails immediately with the correct errno. No new surface, no untrusted-input parsing, no auth/crypto.
Level of scrutiny
Low-to-moderate. The Rust change is a 3-line early-return guard in a Windows-only arm, using the same error-construction shape as the neighboring fstat guard (line 3791) and the existing DuplicateHandle failure branch (line 3983). The root-cause analysis is sound and well-documented: INVALID_HANDLE_VALUE == (HANDLE)-1 == GetCurrentProcess(), and DuplicateHandle explicitly accepts pseudo handles as source. The POSIX arm already returns EBADF for this input via fcntl, so this brings Windows to parity.
Other factors
- Tests are well-constructed per repo conventions:
tempDir/bunEnv/bunExe,Promise.allon stdout/stderr/exited, stderr asserted before exitCode,describe.concurrentfor independent subprocess tests, and the closed-fd case isolated in a child process to avoid fd-number reuse races. The127.0.0.1:1target is fine — the body dup fails synchronously before any connection attempt (asserted viaBun.peek.status). - The PR description confirms the negative tests fail on the unfixed canary and pass with the fix on Windows, and pass either way on Linux (parity guard only).
- No CODEOWNERS on
src/sys/. No prior human review comments to address. The PR notes it composes with #37501 and #37509 without overlap.
…nheriting them (#37523) ## Problem On Windows, every handle returned by `bun_sys::dup()` was created inheritable (`DuplicateHandle(..., bInheritHandle = TRUE, ...)`). libuv's `uv_spawn` calls `CreateProcessW` with `bInheritHandles = TRUE` and no handle list (`vendor/libuv/src/win/process.c`), so any process Bun spawned while such a duplicate was open received a copy of it, and kept the underlying file / pipe / directory open after Bun itself had closed the duplicate, for as long as the child lived. `dup()` backs `Bun.file(fd).stream()` (`FileReader.rs`), a `fetch()` body made from `Bun.file(fd)`, `FileSink` started on an fd (`io/openForWriting.rs`), and the shell: its duplicates of stdin/stdout/stderr and of the cwd directory handle for every subshell / pipeline (`shell/interpreter.rs`, `shell_dup` / `dupe_for_subshell`). The POSIX arm of the same function uses `fcntl(F_DUPFD_CLOEXEC)` precisely so this does not happen. Measured on Windows x64 with the released `1.4.0-canary` (da3851e): a `bun -e` child that prints its own `GetProcessHandleCount()`, spawned five times each way while the parent held 64 open fds: ``` parent holds 64 plain fs.openSync() fds: 164 164 164 164 164 same, plus Bun.file(fd).stream().getReader() on each one: 228 228 228 228 228 (+64, one per dup()) ``` ## Fix `src/sys/lib.rs`, Windows `dup()`: pass `FALSE` for `bInheritHandle`. Nothing relies on these duplicates being inheritable: - When an fd is handed to a child as stdio, libuv duplicates it again itself with `bInheritHandle = TRUE` (`uv__duplicate_handle` in `vendor/libuv/src/win/process-stdio.c`), regardless of the source handle's flag. This covers `Bun.spawn` / `child_process` (`UV_INHERIT_FD` in `spawn/process.rs`) and every shell command, since the shell spawns through the same path. - Bun's other process-creation paths (`--watch` manager, the bin shim, the crash reporter, `std::process::Command` in `bun_core::util`) do not involve `dup()` at all; the first two hand the child the process's real std handles and set those inheritable themselves. - Every other handle Bun opens on Windows (`NtCreateFile` paths, libuv's `uv_fs_open`) is already non-inheritable, which is what the 164 / 164 control above shows. `dup()` was the only `DuplicateHandle` call in the tree. The POSIX arm is unchanged; it already has the CLOEXEC behaviour this gives Windows. ## Verification New test in `test/js/bun/spawn/spawn.test.ts` (Windows only, since the POSIX `dup()` is already close-on-exec): opens 64 fds, spawns a control child, starts `Bun.file(fd).stream()` on each fd, checks with `GetProcessHandleCount` that this process now holds at least 64 more handles, spawns a second child, and asserts that the second child did not start with (about) 64 more handles than the first. - Windows x64, released canary (unfixed): fails with `Expected: < 32, Received: 64`. - Windows x64, debug build of this branch: passes; the same probe as above reads 171 / 171. The rest of `spawn.test.ts` passes there as well (125 pass, 19 platform skips, 3 todo). - Also checked by hand on the debug build that each Windows `dup()` caller still works: `Bun.file(fd).stream().text()`, `fetch()` with a `Bun.file(fd)` body, `Bun.spawn` with an fd as `stdout`, and shell commands with inherited stdio, pipelines and subshells. - On POSIX the test is skipped and the compiled code is unchanged. The same function is touched by #37516 (EBADF for a descriptor that is not open); the two changes are on different lines and `git merge-tree` reports a clean merge either way.
1 participant
Problem
On Windows, using a descriptor number that is not open with an API that duplicates it reports
EMFILEinstead ofEBADF:Same result for a descriptor that was opened with
fs.openSync()and then closed. Reproduced on Windows x64 withbun 1.4.0-canary(da3851e);Bun.file(fd).text(), which does not dup, already reportsEBADFon both platforms.Cause
The Windows
bun_sys::dup(src/sys/lib.rs) passesfd.native()straight toDuplicateHandle. For a libuv-kind fd (which is what every JS-visible fd is on Windows)native()goes throughuv_get_osfhandle, which returnsINVALID_HANDLE_VALUEfor an index that is not open;Fd::INVALIDdecodes to the same value.INVALID_HANDLE_VALUEis(HANDLE)-1, which is also the pseudo handleGetCurrentProcess()returns, andDuplicateHandleexplicitly accepts a pseudo handle as its source: it returns a real handle to the current process. Sodup()succeeded and returned a process handle. The failure only surfaced when the caller tried to turn that handle into a CRT fd (make_lib_uv_owned, innode_fsread_file_with_optionsfor fetch and inFileReader::open_file_blobforstream()); both of those sites report any_open_osfhandlefailure as a hard-codedEMFILE.Every other kernel32/ntdll call we make on
INVALID_HANDLE_VALUEfails withERROR_INVALID_HANDLE(which maps toEBADF);DuplicateHandleis the one call where the value is meaningful, so the check belongs indup()itself rather than at its callers. The posixdup(fcntl(F_DUPFD_CLOEXEC)) fails withEBADFfor the same input, and the Windows arm already tags its errors withTag::dupand the fd, so the result is the same error shape on both platforms apart from the syscall name (dupvsfcntl):Fix
dup()on Windows returnsEBADF(with the fd attached) when the fd decodes toINVALID_HANDLE_VALUE, before callingDuplicateHandle. Valid fds are unaffected; the other Windows callers ofdup()(the shell's dup of stdio and of the cwd handle) only ever pass open handles and keep working (checkedecho, subshells and pipelines with stdio inherited and ignored).Tests
test/js/bun/http/fetch-file-upload.test.ts:fetch()with aBun.file(fd)body rejects withEBADFfrom the dup for a never-opened descriptor and for one that was closed (the latter in a child process so the number cannot be reused in between).test/js/bun/util/bun-file-fd-read.test.ts: the same two cases forBun.file(fd).stream(), plus a check that an open descriptor still streams.The assertions are the same on every platform (
code: "EBADF",fd, and the syscall the error comes from), so on POSIX, where the behaviour was already right, they only guard parity.Windows x64, release canary without the fix: the four negative tests fail (
EMFILEreceived,EBADFexpected); the positive one passes. Windows x64, debug build with the fix: both files pass (16 pass, 3 pre-existing Windows skips). Linux debug (ASAN): both files pass; since the bug is Windows-only, the new tests pass on Linux with and without the change.Related, not overlapping: #37501 changes the class of the fetch body rejection (and leaves the Windows errno alone), #37509 fixes the CRT fd slot that the same fetch path leaks for a valid fd on Windows. Both compose with this change.