Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 2 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Updated 8:05 AM PT - Oct 16th, 2026
❌ @robobun, your commit 6b6fe87 has some failures in 🧪 To try this PR locally: bunx bun-pr 45536That installs a local version of the PR into your bun-45536 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the narrowed Error/Tag imports (now #[cfg(windows)]) in both files — the macOS arms never construct errors locally, so only the Windows NtQueryDirectoryFile paths still need them. The unbounded EINTR retry itself matches the existing macOS getdirentries64 wrapper and libuv, so I did not flag it.
Extended reasoning...
The change adds two EINTR-retrying getdents wrappers in src/sys/lib.rs and rewires the four Linux/FreeBSD directory-iterator arms to use them, plus a glibc-only LD_PRELOAD test; no security-sensitive surface. Two inline findings (FreeBSD offset advance before EINTR, unchecked kernel-reported length) already signal that a human look is needed; this note only records the import narrowing and retry-loop shape that were examined and ruled out.
| let e = last_errno(); | ||
| if e == libc::EINTR { | ||
| continue; |
There was a problem hiding this comment.
🔴 FreeBSD users on fusefs or NFS intr mounts can now get a silently short directory listing where the base branch raised EINTR. The wrapper at src/sys/lib.rs:183 re-issues getdents on the same fd after EINTR, but FreeBSD's kern_getdirentries stores auio.uio_offset into the file offset before it checks the VOP_READDIR error, so entries a filesystem had already copied out before the interrupt are skipped on the retry and never surfaced. Fix: on FreeBSD, only retry when the fd offset is unchanged (compare lseek(fd, 0, SEEK_CUR) before and after, or rewind to the saved offset before retrying); otherwise keep returning the error as the base does. Linux is unaffected because sys_getdents64 returns the byte count whenever any record was emitted.
Why this was flagged
Condition: FreeBSD, a directory on fusefs (daemon honoring FUSE_INTERRUPT) or an NFS mount with intr, and a signal arriving while getdents is in its second or later internal request for one call; reached through every caller of bun_sys::dir_iterator and node::dir_iterator via src/sys/lib.rs:572 and src/runtime/node/dir_iterator.rs:234. FreeBSD fuse_internal_readdir and nfs readdir loop over several requests per getdents call and advance the uio as each block is copied; when a later request fails with EINTR kern_getdirentries has already done foffset = auio.uio_offset before if (error != 0) goto fail, so the fd offset moves past the copied entries. src/sys/lib.rs:179-183 then calls libc_getdents again with the same fd, reading from the advanced offset; the entries copied by the failed attempt are discarded and the iterator reports fewer names with no error. On the base branch the same event returned Err(EINTR) from the four arms, so callers saw a loud failure rather than a short listing. No safeguard checks the offset: the wrapper has no lseek, and the callers reset index/end_index only on success.
Verification: The FreeBSD wrapper at src/sys/lib.rs:177-190 re-issues libc_getdents on the same fd after EINTR with no check or reset of the fd offset. kern_getdirentries does foffset = auio.uio_offset; before if (error != 0) goto fail, so a partially advanced offset is committed even when the call returns EINTR. The retry reads from the advanced offset, gets 0, and the iterator returns Ok(None) at src/sys/lib.rs:572 and src/runtime/node/dir_iterator.rs:234.
| } | ||
| return Err(Error::from_code_int(e, Tag::getdents64)); | ||
| } | ||
| return Ok(rc as usize); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Anyone running bun under an interposed or buggy syscall() (LD_PRELOAD shims, sandboxes, gVisor) can get an out-of-bounds read of the 8 KiB dirent buffer instead of an error. src/sys/lib.rs:165 returns rc as usize unchecked and the callers store it as end_index at src/sys/lib.rs:408 and src/runtime/node/dir_iterator.rs:322, which then drive raw pointer reads. The new shared wrapper is the one site where the kernel-reported length can be clamped. Fix: in both wrappers return an error (or clamp) when rc as usize > len, so every arm is protected at once; keep the same check in the FreeBSD getdents wrapper.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The trigger is a getdents64 return value larger than the len passed in, arriving through libc::syscall at src/sys/linux_syscall.rs:553 from any interposer: this PR's own test loads such a shim via LD_PRELOAD (test/js/node/fs/fs-eintr-linux.test.ts:90), and seccomp/user-mode kernels and sandboxes answer the same call. src/sys/lib.rs:165 converts rc to usize with no comparison against len. src/sys/lib.rs:408 stores it in end_index and the loop at src/sys/lib.rs:430 onward reads d_reclen through a raw pointer while index < end_index, past the end of buf. The base branch had the same unchecked cast in each arm, but this PR introduces the single shared wrapper that owns the kernel boundary and leaves the check out, while src/sys/lib.rs:124 and the repository's own review rules (clamp kernel-reported lengths to capacity) show where it belongs. The node arm's bounds-checked slice at src/runtime/node/dir_iterator.rs:346 panics rather than reads out of bounds, which is still a crash on untrusted input.
Verification: Triggers only when the underlying getdents64 returns a byte count larger than the len it was given. src/sys/lib.rs:165 return Ok(rc as usize); returns the count with no comparison against len; the only bound is debug_assert!(len <= BUF_SIZE) at lib.rs:355, compiled out in release. The base branch already had the identical unclamped self.end_index = rc as usize;, so merging makes nothing worse.
Problem
fs.readdirSync,fs.promises.readdir,fs.opendirSyncandBun.GlobthrowEINTR: interrupted system callwhen a signal interrupts a directory read. Callers that take an iterator error as the end (install prune,bunx) continue with a short listing.getdents64orgetdentscall and returns any error (src/sys/lib.rs,src/runtime/node/dir_iterator.rs). macOS retries since fs(macOS): retry EINTR in the Darwin syscall wrappers instead of surfacing it #41086.Fix
bun_sys::getdents64(Linux, Android) andbun_sys::getdents(FreeBSD) loop onEINTR, next togetdirentries64. Both iterators call them.ENOENTas the end of the directory.test/js/node/fs/fs-eintr-linux.test.ts(8 of 8 rows reportEINTRon 1.4.3). Alsocargo checkfor the 12 CI targets.Background
getdents64fails withEINTRwhen a signal arrives while it waits on a FUSE daemon that honors interrupts (NFS, CIFS: for a handler withoutSA_RESTART).EINTR(uv__fs_work).bun_sys::dir_iterator(Glob, install, resolver) and thenode:fsone.Downsides
textis 256 bytes smaller and thegetdents64call count is the same.EINTRon every attempt does not return, as in libuv. Under--cpu-prof, a modelled 20 ms read that signals abort took 4,753 attempts (9.2 s, debug build), where 1.4.3 throwsEINTR.Notes
Test. A signal cannot be aimed at one syscall of a directory read. The test compiles an
LD_PRELOADlibrary that interposes libcsyscall()(bun issuesgetdents64through it) and fails onegetdents64of a marked directory withEINTR: the first read (eintr-first-*) or the read that reports the end of the directory (eintr-second-*). The library creates<dir>.interruptedwhen it does, and the test checks that file, so a pass cannot come from a shim that did not interpose. Rows:fs.readdirSync,fs.promises.readdir,fs.opendirSyncwithreadSync(thenode:fsiterator) andBun.Glob(thebun_sysiterator). It is glibc-only, like the two shim tests infs.test.ts, and sits next tofs-eintr-darwin.test.ts. On 1.4.3 and on a debug build of main every row reportsEINTR.Measurements. Release builds of one commit (base 519963e), with and without the two changed source files:
text80,701,156 and 80,700,900 bytes, file size equal.bun_sys::dir_iterator::State::nextgrows from 599 to 608 bytes andnode::dir_iterator::NewIterator<false>::nextfrom 547 to 571. There is nogetdents64symbol in the changed build: the wrapper is inlined in both callers. After the syscall the success path is three instructions in both builds (test, one conditional jump,je). An interposing counter sees 2getdents64calls for a directory of 3 entries and 5 for one of 1000, before and after, forreaddirSyncandBun.Glob.When it happens. The generic
getdents64code does not returnEINTR: with a signal pending it stops after the entries it has and returns them (filldir64,fs/readdir.c). The error comes from the filesystem. FUSE waits for its daemon with an interruptible wait and sends the daemon an interrupt request for any signal. A daemon that honors the request answersEINTR, and the kernel returns that answer whatever theSA_RESTARTflag of the handler is (request_wait_answer,fs/fuse/dev.c). NFSv4 and CIFS returnERESTARTSYSfrom some waits, and the process seesEINTRonly when the handler has noSA_RESTART. A read that fails this way wrote no entry and did not move the directory offset, so the next attempt reads the same entries.Retry cost. A shim models a read that takes 20 ms and that a signal aborts with
EINTR(ananosleepbefore the real call). Without a profiler, 1.4.3 and the changed build read the directory with no retry. With--cpu-profthe sampler suspends the JS thread with a signal for every sample (1 ms apart by default): 1.4.3 throwsEINTRafter 3 ms, and the changed debug build returns the listing after 4,753 attempts and 9.2 s. The same model onrenameshows what a wrapper that already loops does today:fs.renameSyncon 1.4.3 returns after 1,908 attempts and 3.8 s.Not in this change. Other calls report
EINTRin the same way. A shim that fails one libc call shows it on main:fs.unlinkSync,fs.rmdirSync,fs.linkSync,fs.truncateSync,fs.fsyncSync,fs.fdatasyncSync,fs.promises.unlinkandfs.promises.rmdirthrow it,fs.existsSyncanswersfalsefor a file that exists, andfs.cpSyncandBun.write(file, file)throw it fromcopy_file_range. None of them reads a directory, so they are left for a separate change.Design. The retry could also live inside
linux_syscall::getdents64(FreeBSD would keep two arms), or in the OS-independentnext()above the arms (the arm would still returnEINTRfor another layer to hide). The choice was made directly from the reasons in Background. A review of #41480 had asked for this shape in its own change. #41480 carried the retry inside the four arms. That was removed there.Two open pull requests touch the same lines. #44195 rewrites the record parsing in the same four
nextfunctions and edits the buffer arguments of the calls this change replaces. #45347 adds its ownbun_sys::getdents64, which reports a removed directory and has noEINTRloop. The one that lands later needs a small rebase, and the loop stays in the wrapper that remains.Self-review. A review pass over this diff did not complete. Five concerns from its partial work were checked by hand and are covered above: the comment lint (the two wrapper comments are two lines each now), the open pull requests on the same lines, the calls with the same fault, the wording of when
EINTRhappens, and the cost of the retry under a signal storm.Other suites.
test/js/node/fs/fs.test.ts: 613 pass. Three recursivereaddirstress tests (should work x 100) time out in the debug build on the machine used, and they time out the same way with main'ssrc/.test/js/node/fs/dir.test.ts: 23 pass.test/js/bun/glob/scan.test.ts: the same six recursive scans time out with and without the change.