Skip to content

node:fs: pin the directory fd at opendir time - #35928

Open
robobun wants to merge 11 commits into
mainfrom
farm/3d50f98a/opendir-pin-fd
Open

node:fs: pin the directory fd at opendir time#35928
robobun wants to merge 11 commits into
mainfrom
farm/3d50f98a/opendir-pin-fd

Conversation

@robobun

@robobun robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

What

fs.opendir() / fs.opendirSync() now open a real directory descriptor and iterate entries through it, matching Node's uv_fs_opendir / uv_fs_readdir model.

Why

Previously the Dir handle was path-bound: opendir only did a stat check, and the first read() re-opened the path with readdirSync. Consequences verified against Node v26.3.0:

const dh = fs.opendirSync(R + "/d");        // node: openat(O_DIRECTORY); bun: statx only
fs.renameSync(R + "/d", R + "/old");
fs.mkdirSync(R + "/d");
fs.writeFileSync(R + "/d/REPLACEMENT", "");
while ((e = dh.readSync())) names.push(e.name);
// node: ["ORIGINAL"]   (reads the pinned inode)
// bun:  ["REPLACEMENT"] (re-opened the path)
  • TOCTOU: rename-swapping the path (or retargeting a symlink) between opendir and read() made Bun iterate the new object; Node iterates the original.
  • rm after opendir: Bun's read() threw ENOENT with syscall: 'scandir'; Node returns end-of-stream.
  • Open-time errors deferred: opendirSync on an unreadable directory or at RLIMIT_NOFILE succeeded, deferring EACCES/EMFILE to the first read() as a scandir error instead of the opendir error Node throws.
  • fd census: 40 held Dir handles pinned 0 fds in Bun, +40 in Node.

How

  • New native opendir op (openat(O_DIRECTORY|O_RDONLY) on POSIX, NtCreateFile iterable on Windows) returns the fd with syscall: "opendir" on failure.
  • New internal fdreaddir op reads entries from an already-open fd without closing it.
  • Dir stores the real fd, reads through fdreaddir, and closes it on close()/closeSync(). Recursive iteration still goes by path (subdirectories must be opened individually), but the root is opened eagerly so open-time errors surface.
  • A FinalizationRegistry closes the fd and emits Node's Closing directory handle on garbage collection warning when a Dir is collected without close(), so the newly-held descriptor can't leak (mirrors FileHandle).
  • Linux/macOS directory iterators now treat ENOENT from getdents64 / __getdirentries64 on an unlinked but still-open directory as EOF. POSIX requires this and glibc's readdir() does the same, which is where Node gets its behavior. The FreeBSD iterator already handled it.

Verification

# before (system bun)
{"fdHeldByHandle":0,"entriesAfterSwap":["REPLACEMENT"]}
after-rm read threw ENOENT scandir
# after
{"fdHeldByHandle":1,"entriesAfterSwap":["ORIGINAL"]}
after-rm read -> null(end)

test/js/node/fs/dir.test.ts (28 pass), test-fs-opendir.js, and test/js/node/fs/fs.test.ts pass on Linux.


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/dir.test.ts

fs.opendir/opendirSync previously returned a path-bound Dir: no file
descriptor was opened until the first read(), which re-opened the path
each time. Node opens the directory eagerly (uv_fs_opendir) and reads
entries through that fd, so the handle pins the inode.

The observable differences:
- renaming or swapping the target between opendir and read made Bun
  iterate the new object at that path; Node iterates the original
- removing the directory after opendir made Bun's read() throw ENOENT
  (syscall 'scandir'); Node returns end-of-stream
- open-time errors (EACCES, EMFILE) surfaced at the first read() as
  scandir errors instead of at opendir time

Add native opendir (openat O_DIRECTORY / NtCreateFile iterable) and an
fd-based readdir so Dir holds a real descriptor and iterates through it.
Also treat ENOENT from getdents64/__getdirentries64 on an unlinked but
still-open directory as EOF (POSIX requirement; glibc readdir does the
same, which is what node inherits).
@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a7e02c70-4d4b-45ad-86bb-61e74317bcc8

📥 Commits

Reviewing files that changed from the base of the PR and between 44f6469 and b0657d2.

📒 Files selected for processing (10)
  • src/event_loop/ConcurrentTask.rs
  • src/js/node/fs.promises.ts
  • src/js/node/fs.ts
  • src/runtime/dispatch.rs
  • src/runtime/node/dir_iterator.rs
  • src/runtime/node/node.classes.ts
  • src/runtime/node/node_fs.rs
  • src/runtime/node/node_fs_binding.rs
  • src/sys/lib.rs
  • test/js/node/fs/dir.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 11:27 AM PT - Jul 26th, 2026

@robobun, your commit b0657d2 has 2 failures in Build #82484 (All Failures):

  • 📦 Binary size — 12 over 0.50 MB
  • targetthis build canary: main #79916
    sizeΔ
    bun-darwin-aarch6458.13 MB57.58 MB+564.9 KB
    bun-darwin-x6463.48 MB62.95 MB+544.5 KB
    bun-linux-aarch6470.98 MB70.42 MB+576.0 KB
    bun-linux-x6472.48 MB71.95 MB+544.0 KB
    bun-linux-aarch64-musl64.88 MB64.32 MB+576.0 KB
    bun-linux-x64-musl66.98 MB66.45 MB+544.0 KB
    bun-linux-aarch64-android78.47 MB77.97 MB+512.0 KB
    bun-linux-x64-android80.64 MB80.10 MB+545.2 KB
    bun-freebsd-x6483.09 MB82.56 MB+544.0 KB
    bun-freebsd-aarch6484.84 MB84.31 MB+544.0 KB
    bun-windows-x6480.27 MB79.70 MB+582.0 KB
    bun-windows-aarch6470.87 MB70.34 MB+542.5 KB

    Add [skip size check] to the commit message if this increase is intentional.

  • test/bake/deinitialization.test.ts - pid 1993 error: addresssanitizer on 🐧 13 x64-asan

🧪   To try this PR locally:

bunx bun-pr 35928

That installs a local version of the PR into your bun-35928 executable, so you can run:

bun-35928 --bun

Comment thread src/js/node/fs.ts Outdated
Comment thread src/runtime/node/dir_iterator.rs Outdated
Comment thread src/runtime/node/dir_iterator.rs Outdated
Comment thread src/runtime/node/node_fs.rs Outdated
Comment thread src/runtime/node/node_fs.rs
@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Check if path exists before returning from opendirSync #17846 - Also fixes opendirSync not validating the directory at call time; a smaller draft-quality attempt that node:fs: pin the directory fd at opendir time #35928 fully supersedes

🤖 Generated with Claude Code

@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Heads up from #35931: with the constructor now doing this.#handle = handle unguarded, a userland new fs.Dir(<integer>, path).closeSync() would close that unrelated fd here too (the dirSetHandle path is fine, it's the public constructor that's the hole). The tests added in test/js/node/fs/dir.test.ts (new fs.Dir with a foreign integer handle) cover this and should keep passing after rebasing; easiest guard is probably to only close when the handle arrived via dirSetHandle.

Comment thread src/js/node/fs.ts
Comment thread src/js/node/fs.ts
Comment thread src/js/node/fs.ts Outdated
Comment thread src/runtime/node/node_fs.rs Outdated
robobun and others added 3 commits July 26, 2026 11:28
The binary-size baseline is build #79916 (ae4b17d, 2026-07-25). Since
then 12 commits landed on main including node:quic (#32602), node:repl
(#31827), node:inspector (#31823) and the tls overhaul (#34598); other
PRs branched from current main see the same ~550KB delta (e.g. build
82225). This PR adds two small node:fs ops.
Comment thread src/runtime/dispatch.rs Outdated
@robobun

robobun commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

CI status on b0657d2 (build 82484):

The diff is green; ready for review.

Comment thread src/js/node/fs.ts Outdated
Comment thread src/js/node/fs.ts
Comment thread src/js/node/fs.ts
Comment thread test/js/node/fs/dir.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All six rounds of prior feedback have been addressed and this pass found nothing new, but I'd still like a human to sign off — this changes Dir from path-bound to owning a real fd, adds two native ops wired through the task-tag dispatch table, and touches the shared dir_iterator.rs ENOENT handling on Linux/macOS. The Windows open_dir_at_windows_a path is unverified locally per the PR description.

Checked this pass: readdir_with_entries does not close the caller-supplied fd (so fdreaddir correctly leaves ownership with Dir); the Windows iterator's RestartScan=TRUE on each fdreaddir is fine because the JS side caches #entries after one call; the ENOENT→EOF mapping in dir_iterator.rs applies to all iterator callers but matches glibc readdir() and the pre-existing FreeBSD branch.

Extended reasoning...

Overview

Reworks fs.opendir/fs.opendirSync from a path-bound stat check to Node's fd-bound model: a new native opendir op (openat(O_DIRECTORY|O_RDONLY|O_CLOEXEC) on POSIX, NtCreateFile iterable on Windows) returns a real fd stored on Dir, and a new internal fdreaddir op reads entries through it. Touches ten files across the event-loop task table (ConcurrentTask.rs, dispatch.rs), the fs binding surface (node_fs.rs, node_fs_binding.rs, node.classes.ts), the shared directory iterator (dir_iterator.rs), the syscall tag table (sys/lib.rs), the built-in JS Dir class (fs.ts, fs.promises.ts), and adds five new tests in dir.test.ts.

Security risks

None identified. The change narrows a TOCTOU window (path swap between opendir and read()) rather than opening one. O_CLOEXEC was added in an earlier round; the new fd is read-only and O_DIRECTORY-gated. No user-controlled data reaches size/index arithmetic.

Level of scrutiny

High. This is the commit where Dir gains ownership of a real OS resource, so every lifecycle exit (close/closeSync, dispose/asyncDispose, async-iterator finally, GC via FinalizationRegistry) had to be audited — and was, across six prior review rounds that landed the registry, the #owned guard, null-before-close ordering, and the > 2 guard removal. The dir_iterator.rs ENOENT→EOF change is a shared-helper edit that affects every directory iteration in the runtime, not just fs.Dir. The Windows branch calls open_dir_at_windows_a and relies on fs.closeSync accepting the resulting HANDLE; the author notes this was deferred to CI.

Other factors

The PR has been iterated extensively in response to review; all prior inline threads are resolved with fix commits, CI on 816de4e was green on every dir.test.ts lane, and the new tests cover the rename-swap TOCTOU, rm-after-open EOF, fd census, and GC-warning paths (the GC test was moved out of the !isPosix gate in b0657d2 so it now runs on Windows too). The remaining reason to defer is scope, not open concerns: cross-platform fd lifecycle changes in node:fs warrant a maintainer's eye even when clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants