FileSystemRouter: throw when a relative dir does not fit in the path buffer instead of aborting - #39307
FileSystemRouter: throw when a relative dir does not fit in the path buffer instead of aborting#39307robobun wants to merge 3 commits into
Conversation
…buffer The constructor joined a relative dir onto the cwd with join_abs_string_buf into a fixed stack array, which indexes past the array when the joined path does not fit and aborts the process. Join with join_abs_string_buf_checked into a pooled PathBuffer instead and throw when the result does not fit.
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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 (2)
Comment |
|
Status: ready for review. CI is green on d52de00 (Buildkite build 99183, 179/179 jobs).
|
There was a problem hiding this comment.
LGTM — swaps the unchecked stack-buffer join for the checked, pooled variant already used by Bun.mmap/fs.watch, turning a user-reachable panic into a TypeError.
What was reviewed:
join_abs_string_buf_checkedsignature and thepath_buffer_pool::get()guard lifetime —dir_bufoutlives every read ofroot_dir_path.slice(), and the never-freeZigStringSlicedoes not dereference on drop.- Boundary math in the test's
resolvingTohelper against the&mut dir_buf[..MAX_PATH_BYTES - 1]slice —MAX_PATH_BYTES - 1fits and falls through toread_dir_info,MAX_PATH_BYTESthrows. - Test shape: subprocess-isolated crash repro, concurrent pipe drain,
signalCode: nullasserted, and a trailing valid relativedirto confirm the happy path still resolves against cwd.
Extended reasoning...
Overview
Replaces one call site in FileSystemRouter::constructor (src/runtime/api/filesystem_router.rs): the relative-dir branch now joins onto the cwd with join_abs_string_buf_checked into a pooled PathBuffer instead of join_abs_string_buf into a [0u8; MAX_PATH_BYTES * 2] stack array, and throws a TypeError naming the option and the byte limit when the joined path does not fit. Adds one subprocess-based test to test/js/bun/util/filesystem_router.test.ts covering the crash input, the absolute variant, the exact boundary (MAX_PATH_BYTES - 1 vs MAX_PATH_BYTES), and a real relative pages dir.
Security risks
None. This tightens validation of user input on a JS-visible constructor and removes a user-reachable abort; no auth, crypto, network, or filesystem-write paths are touched.
Level of scrutiny
Low-to-medium. The Rust change is ~15 lines and mirrors the identical pattern in BunObject.rs (Bun.mmap) and node_fs_watcher.rs line-for-line: pooled buffer, let Some(joined) = ... else { throw_invalid_arguments }. The pooled guard is declared at the same scope the old stack array was, so the ZigStringSlice::from_utf8_never_free(joined) lifetime story is unchanged — the buffer lives past the last read of root_dir_path.slice() (the Unable to find directory error), and is returned to the pool on drop at function exit. Passing MAX_PATH_BYTES - 1 as the buffer length lines up the new error with the length Resolver::read_dir_info already refuses, which the test's boundary cases pin down.
Other factors
The test follows the repo's conventions: tempDir + bunEnv/bunExe, Buffer.alloc(n, fill) instead of repeat, drains stdout/stderr/exited concurrently, asserts signalCode: null so an abort surfaces as a diff rather than a hang, and the per-platform maxPathBytes constant matches src/bun_core/util.rs. The PR description confirms the test aborts on the unfixed build and passes with the fix, and the rest of filesystem_router.test.ts still passes. No prior reviewer comments to address.
…sed on Windows too
|
Updated 11:01 PM PT - Aug 15th, 2026
✅ @robobun, your commit d52de004661102d21418d22cb4c89cd34701f9d2 passed in 🧪 To try this PR locally: bunx bun-pr 39307That installs a local version of the PR into your bun-39307 --bun |
There was a problem hiding this comment.
LGTM — converts a user-reachable panic into a catchable TypeError, following the same join_abs_string_buf_checked + pooled-buffer pattern already used in BunObject.rs and node_fs_watcher.rs.
What was reviewed:
dir_buf(RAIIPoolGuard) is declared at constructor scope, so the borrowedjoinedslice insideroot_dir_pathstays valid throughclone_normalizing_separatorsand theUnable to find directoryerror path; the guard returns to the pool on every exit.- No NUL-sentinel byte is reserved (
&mut dir_buf[..]), unlike the other callers — checked that the result is wrapped as a plainZigStringSliceand cloned, never passed as aZStr. join_abs_string_buf_checkedreturnsSomeat exactlybuf.len()andNoneabove it, matching the test'satLimit/oneByteOverLimitboundary and theMAX_PATH_BYTESvalue in the error message.- Test spawns a subprocess (so an abort would surface as a nonzero exit / signal), covers relative-over-limit, absolute, exact boundary, one-over, and a real relative dir afterward; the hardcoded
maxPathBytesper platform matchessrc/bun_core/util.rs.
Extended reasoning...
Overview
Two-file change: src/runtime/api/filesystem_router.rs swaps a fixed [0u8; MAX_PATH_BYTES * 2] stack buffer + unchecked join_abs_string_buf for a pooled PathBuffer guard + join_abs_string_buf_checked, throwing a TypeError when the joined cwd + dir overflows. test/js/bun/util/filesystem_router.test.ts gains one subprocess-spawning test covering the crash case, the boundary, the absolute-path branch (unchanged), and a positive relative-dir case.
Security risks
None introduced. The change tightens validation of user input (dir) that previously reached an out-of-bounds slice index and aborted the process. The error message names the option and byte limit rather than echoing the arbitrarily-long input. No new syscalls, no auth/crypto, no network.
Level of scrutiny
Low-to-medium. This is a small, mechanical substitution to the checked variant of an existing helper, matching the exact shape of two other JS entry points (Bun.mmap, fs.watch). The pooled-buffer guard is RAII (PoolGuard drops back to the pool), so there is no acquisition/release pairing to audit. The only lifetime subtlety — root_dir_path borrowing into dir_buf via from_utf8_never_free — is handled by declaring dir_buf at constructor scope (annotated with a one-line comment), and its last read is well before the guard drops.
Other factors
The comment-cop bot flagged an earlier revision's paragraph-long comment; the author addressed it in d52de00 by sizing the join to the whole buffer and dropping the comment (thread resolved). The test was verified to abort on an unfixed build and pass on the fixed one on both Linux and Windows. The bug-hunting system found nothing. Side benefit: removes a ~196 KB stack array on Windows.
Problem
new Bun.FileSystemRouter({ dir, style: "nextjs" })aborts the process whendiris relative andcwd + diris longer than the join buffer:Reproduces on 1.4.0 with
dir: Buffer.alloc(10000, "a").toString()(exit 134, SIGABRT). Found while fixing the same pattern in bake (bake: report router roots that do not fit the path buffer and skip such route entries instead of panicking #39196); there is no user report. The joined path has to be longer than2 * MAX_PATH_BYTES(8192 bytes on Linux, 2048 on macOS, 196604 on Windows, where the panic readsrange end index 200017 out of range for slice of length 196604), so only adirthat could never name a directory reaches the abort. It is still an abort on a documented constructor's string option rather than a thrown error.Cause: the constructor (
src/runtime/api/filesystem_router.rs,FileSystemRouter::constructor) resolves a relativedirwithjoin_abs_string_bufinto a fixed[u8; MAX_PATH_BYTES * 2]stack array. That join normalizes straight into the buffer and indexes past its end when the result does not fit; nothing checks the length first.The absolute form of the same input does not crash: it skips the join,
Resolver::read_dir_inforefuses any path ofMAX_PATH_BYTESor more, and the constructor throwsUnable to find directory: .... Relative dirs that joined to betweenMAX_PATH_BYTESand2 * MAX_PATH_BYTESbytes got the sameUnable to find directoryerror.Fix
join_abs_string_buf_checkedinto a pooledPathBuffer(path_buffer_pool::get()), and throwTypeError: Expected dir to resolve to a path of at most <MAX_PATH_BYTES> byteswhen the joined path does not fit in it.MAX_PATH_BYTESbytes still fits and takes the existing route (read_dir_inforefuses it,Unable to find directory), one byte more throws the new error. The message names the option and the limit rather than echoing the path, which can be arbitrarily long.dirjoining to more thanMAX_PATH_BYTESbytes now gets thisTypeErrorinstead ofError: Unable to find directory. No such path can name a directory, so nothing that constructed successfully before changes.Bun.mmapinBunObject.rs,fs.watchinnode_fs_watcher.rs), and the same per-site granularity as the merged fixes of this class inBun.Glob(glob: fix abort when joined absolute paths exceed the join buffer #33145), the resolver (resolver: don't abort on a package.json browser map key longer than 1024 bytes #37526) andbun install(install: stop panicking on workspaces entries longer than the path buffer #37531). Dropping the stack array also removes ~196 KB of stack usage per construction on Windows, whereMAX_PATH_BYTESis 98302.root_dir_pathis a borrowed slice into it that is read up to theUnable to find directoryerror.Unable to find directory.test/js/bun/util/filesystem_router.test.ts, "throws instead of aborting when a relative dir no longer fits in a path buffer once joined with the cwd". One spawned fixture constructs with a 250 KB relative dir (the abort on every platform), the same dir made absolute (stillUnable to find directory), relative dirs resolving to exactlyMAX_PATH_BYTESandMAX_PATH_BYTES + 1bytes (missing directory vs. the new error), and finally a real relativepagesdir, which still resolves against the cwd (no existing test in the file used a relativedir).src/change and passes with it, on Linux and on Windows x64; the rest offilesystem_router.test.tspasses; the fixture also passes underBUN_JSC_validateExceptionChecks=1.Background
MAX_PATH_BYTES(src/bun_core/util.rs) is Bun's fixed path buffer size: 4096 on Linux, 1024 on macOS, 98302 on Windows (the UTF-8 worst case of a 32767-unit wide path).PathBufferis a[u8; MAX_PATH_BYTES], andbun_paths::path_buffer_poolhands out heap-allocated ones so large path buffers do not live on the stack.join_abs_string_buf(cwd, buf, parts)resolvespartsagainstcwdand normalizes the result intobuf; it assumes the result fits.join_abs_string_buf_checkedis the variant for user-controlled input: it returnsNonewhen the normalized result is longer thanbuf.Resolver::read_dir_info(path)is what the constructor uses next to load the directory; it returns "not found" for any path ofMAX_PATH_BYTESbytes or more, which is how the absolute form (and the exactly-MAX_PATH_BYTESrelative form) end up asUnable to find directory.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/util/filesystem_router.test.ts