node:fs: Dir.close must not close an fd it did not open - #35931
Conversation
Dir in the path-bound implementation never opens a descriptor of its own (opendir/opendirSync pass the sentinel 1 as the handle), so the `if (handle > 2) fs.closeSync(handle)` in close()/closeSync() is dead for legitimate use and only ever fires on a userland `new fs.Dir(fd, path)`, closing an unrelated fd by number. Node treats the handle as an opaque DirHandle and never touches an integer passed there. Drop the closeSync calls; #handle is now purely the open/closed marker.
|
Updated 10:08 AM PT - Jul 26th, 2026
❌ @robobun, your commit d699196 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35931That installs a local version of the PR into your bun-35931 --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 (2)
WalkthroughChangesDir handle lifecycle
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct removal of dead-for-internal / wrong-for-userland closeSync(handle) branch.
What was reviewed:
- Verified all three internal
Dirconstruction sites (opendir,opendirSync,promises.opendir) pass the literal1sentinel, so the removedhandle > 2branch was reachable only via userlandnew fs.Dir(int, ...). - Checked that
#handleis otherwise used purely as an open/closed marker — no other consumer reads it as an fd. - Tests cover all three close paths (sync, async,
Symbol.dispose), usetempDir/usingfor hermetic cleanup, and the probe's dispose swallows EBADF so the failing case surfaces the real assertion.
Extended reasoning...
Overview
Two-file change: src/js/node/fs.ts drops the if (handle > 2) fs.closeSync(handle) line from both Dir.#closeOp() and Dir.closeSync(), and updates the #handle doc comment to state that this path-bound implementation never owns a real fd. test/js/node/fs/dir.test.ts adds three regression tests asserting that a Dir constructed directly with a foreign integer handle does not close that fd on closeSync(), close(), or Symbol.dispose.
Correctness
I confirmed the PR's central claim by grepping src/js for new Dir(: the only constructions are in opendir (fs.ts:613) and opendirSync (fs.ts:1028), both passing the literal 1; fs.promises.opendir delegates to the callback form. So no Bun-created Dir ever had #handle > 2, and the removed branch was dead for the intended path — it fired only when userland called new fs.Dir(someFd, path) directly, in which case closing that fd diverges from Node (Node's DirHandle.close throws TypeError on an integer and touches no descriptor). The remaining uses of #handle are all < 0 / >= 0 state checks plus the = -1 write, so nothing else depended on it holding a real fd.
Security risks
None. The change removes an fd close on a value the class never opened, which is strictly safer (no more accidental close of an unrelated descriptor). No new syscalls, no new user-controlled input handling.
Level of scrutiny
Low-to-moderate. Small, self-contained JS-side Node compat fix with a clear mechanism, verified against all internal call sites, and covered by tests that the description shows fail on system Bun and pass on the debug build. The PR notes the interaction with #35928's fd-bound rewrite, which is a separate concern.
Other factors
Tests follow harness conventions (tempDir, using, per-case isolation), open a real probe fd rather than assuming a fixed number, and wrap the probe's cleanup closeSync in try/catch so the USE_SYSTEM_BUN failing case reports the fstat assertion instead of a cleanup EBADF. The test-fs-opendir.js parallel test still passes per the description. No prior review comments to address.
2 participants
Repro
Node's
Dirholds an opaqueDirHandleand calls.close()on it; passing an integer there throwsTypeError: this[#handle].close is not a functionand leaves the descriptor untouched.Cause
Dirin the current path-bound implementation never opens a descriptor of its own:opendir/opendirSyncpass the literal1as the handle sentinel, and reads go throughfs.readdir(path). Theif (handle > 2) fs.closeSync(handle)in#closeOp/closeSyncis therefore unreachable for anyDirBun creates, and only fires when userland constructsnew fs.Dir(<integer>, path)directly, closing an unrelated fd by number.Fix
Drop the
fs.closeSync(handle)from both close paths.#handleis now purely the open/closed state marker its doc comment describes.This is orthogonal to the fd-bound rewrite in #35928; that PR stores a real fd via a private
dirSetHandlesetter after the native open, so a userland integer passed to the constructor would still need to be kept out of the close path there too.Verification
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/fs/dir.test.ts