shell(windows): report ENAMETOOLONG for operands that do not fit the path buffer - #38205
shell(windows): report ENAMETOOLONG for operands that do not fit the path buffer#38205robobun wants to merge 1 commit into
Conversation
…path buffer shell_get_path rewrites every builtin, redirect and [[ -f/-d ]] operand against the shell's cwd inside one PathBuffer before the open or stat. Both of its writes were unchecked: a POSIX-absolute operand was spliced after the drive root with a plain slice copy, and a relative operand was joined with join_z_buf, which indexes into the buffer without a bounds check. An operand of roughly MAX_PATH_BYTES (98302 bytes on Windows) or more therefore aborted the process with a slice index panic. Check the length before each write and return ENAMETOOLONG naming the operand, tagged with the syscall the caller is about to make. The relative join is now normalized on the heap first so that the check is against the normalized length: operands that are over-long as written but collapse through `..` segments keep working as before.
|
Warning Review limit reached
Next review available in: 33 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 (2)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted Windows-only crash fix that turns an unchecked buffer write into a proper ENAMETOOLONG.
What was reviewed:
- Both write paths in
shell_get_pathnow bounds-check againstbuf.len()with the correct>=(leaves room for the trailing NUL); the relative branch's switch tojoin_spillwrites intoJOIN_BUF/spill, neverbuf, so the borrow ofdirpath(a slice ofbuf) is released before the copy-back and there is no aliasing. - All three call sites of
shell_get_path(the only ones in the tree) pass the newTagargument; every changed line is inside#[cfg(windows)]. - Verified
join_spillsemantics inresolve_path.rs— it sizes off the raw parts and normalizes into a separate buffer, so the collapsing-..case the test covers keeps resolving;spillis a localVecthat drops at scope end. - Test runs in a child process, drains pipes concurrently, covers both branches × several builtins/redirects/
[[ ]], asserts exact messages, and checks the mv-source is untouched and the collapsing redirect actually wrote its file.
Extended reasoning...
Overview
The PR fixes a Windows-only panic in shell_get_path (src/runtime/shell/interpreter.rs), the helper that rewrites shell operands against the tracked cwd before open/stat. Two unchecked writes into a fixed-size PathBuffer (~98 KB) could overflow: a slice copy_from_slice in the POSIX-absolute branch and join_z_buf in the relative branch. Both now return ENAMETOOLONG (tagged with the caller's syscall and the original operand) when the rewritten path would not fit. The relative branch was reworked to join via join_spill (thread-local buffer or a heap Vec) so the normalized length can be checked before copying into buf; this preserves the existing behavior for over-long-as-written operands that collapse via .. to something that fits. The function grew a bun_sys::Tag parameter and all three (Windows-only) call sites were updated. A Windows-only subprocess test in bunshell.test.ts exercises cat/ls/mv/redirects/[[ -f/-d ]] on both branches plus the collapsing case.
Security risks
None. This replaces a user-reachable panic (a DoS on user input) with a recoverable error identical to what shorter over-long operands already produce, and identical to what the downstream syscalls would have returned. No new inputs are accepted; the collapsing-.. path already worked and continues to. All changes are gated to Windows.
Level of scrutiny
Moderate. The change is small (~35 lines of Rust), local to one #[cfg(windows)] helper and its three private call sites, and turns a crash into an error that matches existing behavior one length-bracket down. I checked the off-by-one on both >= guards (one byte reserved for the NUL), confirmed join_spill writes into storage disjoint from buf so dropping the old .to_vec() copy of dirpath is sound, confirmed spill is dropped, and grepped for other callers of shell_get_path (none). I read join_spill/join_z_buf/join_needed in resolve_path.rs to confirm the "normalizes outside the buffer" claim.
Other factors
The test follows the repo's harness conventions: tempDir, Buffer.alloc(n, fill) instead of .repeat, concurrent pipe drain, exact-message assertions, exit-code asserted last, subprocess isolation so a regression fails the test rather than the runner, and side-effect checks (in.txt intact, out.txt written). The PR description documents USE_SYSTEM_BUN=1 failing and bun bd passing on Windows, plus the length probe table showing the boundary. Neither non-Windows platforms nor any code path outside the shell's Windows operand-rewrite is touched.
|
Updated 2:04 PM PT - Aug 13th, 2026
✅ @robobun, your commit 7d445dc3bc79073651e1f443793d2e6a6a743edf passed in 🧪 To try this PR locally: bunx bun-pr 38205That installs a local version of the PR into your bun-38205 --bun |
1 participant
Problem
[[ -f / -d ]]operand of roughly 98 KB or more aborts the process instead of failing:cat /aaa…,ls /aaa…,mv x /aaa…,echo hi > /aaa…,[[ -d /aaa… ]](100000 a's):panic: range end index 100003 out of range for slice of length 98302cat aaa…,echo hi > aaa…,[[ -f aaa… ]]:panic: range end index 100013 out of range for slice of length 98301File name too long; only the ones that do not fit the buffer crash.shell_get_path(src/runtime/shell/interpreter.rs:2173on main) rewrites each operand against the shell's cwd inside onePathBuffer(MAX_PATH_BYTES= 98302 on Windows) and never checks the length. The POSIX-absolute branch splicesto[1..]after the drive root with a plain slice copy; the relative branch callsjoin_z_buf, which indexes into the buffer unchecked. The Zig version (ShellSyscall.getPath) had the same two writes, so this is not a regression.shell_openat(cat, ls, rm, mv, builtin and external-command redirects) andshell_statat([[ -f ]],[[ -d ]]). POSIX hands operands toopenat/fstatatuntouched and is unaffected;cdis unaffected on both (it has its own 4 KiB check).rm /aaa…currently panics earlier in rm.rs's root check and will reach this path once shell(rm): stop panicking on operands longer than the path scratch buffers #37521 lands.Fix
ENAMETOOLONGnaming the operand, tagged with the syscall the caller is about to make (shell_get_pathtakes aTag;shell_openatpassesopen,shell_statatpassesfstatat, matching what the POSIX branches' errors carry).join_spill(normalizes outside the buffer, on the heap when needed) and copies the result in once its normalized length is known to fit. Operands that are over-long as written but collapse through..segments resolve today and keep resolving; a check on the raw length would have broken them.ENAMETOOLONGis the right answer:PathBufferis sized to hold the UTF-8 form of any path Windows can open (see Background), so a rewritten path that does not fit is longer than any openable path, and the consumers of this function (bun_sys::openvia libuv,open_dir_at_windows_a,bun_sys::stat) already returnENAMETOOLONGfor it, which is what the 40 KB to 98 KB operands show today. The check returns the same error one layer earlier instead of overflowing. Everything that resolved before still resolves: the POSIX-absolute check is exact, and the relative branch now checks the final normalized length, which is never more than whatjoin_z_bufneeded.cat: <operand>: File name too long,ls: …,mv: …,bun: File name too long: <operand>for redirects, exit 1;[[ -f ]]/[[ -d ]]evaluate false.test/js/bun/shell/bunshell.test.tsdrives both branches through cat, ls, mv, builtin redirects and[[ -f ]]/[[ -d ]]in a child process, plus the collapsing operands. On Windows x64 (Server 2019):USE_SYSTEM_BUN=1 bun test test/js/bun/shell/bunshell.test.ts -t "operands longer"with the canary at b7a0431: fails, the child aborts withpanic: range end index 100064 out of range for slice of length 98301.bun bd test test/js/bun/shell/bunshell.test.ts -t "operands longer": passes.bun bd teston the rest of bunshell.test.ts, commands/{ls,mv,rm}.test.ts, shell-seq-condexpr, file-io, bunshell-file, bunshell-default: pass (the five tilde_expansion cases in bunshell.test.ts fail identically with the unmodified canary on that machine; they read an env var it does not set).cfg(windows), so the Linux binary is unchanged.Background
shell_get_path: Windows has no*at()syscalls, so the shell emulatesopenat(cwd_fd, operand)by asking for the cwd handle's real path (GetFinalPathNameByHandle) and building an absolute path from it. Relative operands are joined under that path, a/xoperand is placed on that path's drive root so POSIX-style absolute paths mean something on Windows, and/dev/nullbecomesNUL. Its output goes straight tobun_sys::open,open_dir_at_windows_aorbun_sys::stat.PathBuffer/MAX_PATH_BYTES: bun's fixed-size UTF-8 scratch buffer for one path. On Windows it is 32767 (the NT limit on a path, in UTF-16 units) times 3 (the most UTF-8 bytes one UTF-16 unit can take) plus one byte for the NUL, which is why "does not fit the buffer" implies "longer than any path the OS accepts".join_z_bufvsjoin_spill(src/paths/resolve_path.rs): both concatenate path parts and normalize the result (.,.., repeated separators).join_z_bufwrites into a caller-supplied buffer with no bounds check;join_spillwrites into a thread-local buffer, or into a caller-suppliedVecthat it grows, so the result can be measured before it is copied into anything fixed-size.bun_sys::Tag: thesyscallfield ofbun_sys::Error, naming the operation the error belongs to. Shell error messages do not print it; it is diagnostic metadata.Operand length probes on the canary (b7a0431) and on this branch, Windows x64
Relative operand (
echo hi > a…, cwdC:\workspace, 12 bytes) and rooted operand (echo hi > /a…, rootC:\, 3 bytes):No such file or directoryNo such file or directoryFile name too longFile name too longFile name too long98303vs98301)File name too longFile name too longFile name too longFile name too longcat,ls,mv(rooted target) and[[ -f ]]/[[ -d ]]behave the same way as the redirect in each row.a/../repeated to 100000 bytes followed by an existing file name works on both the canary and this branch forcat, a redirect and[[ -f ]].