Conversation
… $BUN_INSTALL, $XDG_CACHE_HOME or $HOME do not fit the path buffer open_global_dir and open_global_bin_dir joined the environment value with the unchecked join_abs_string_buf, which indexes past the PathBuffer when the joined path is longer than MAX_PATH_BYTES, so every -g command aborted with "panic: range end index N out of range for slice of length 4095". Join through join_abs_string_buf_checked and fail with Error::Sys(ENAMETOOLONG), the error an oversized $BUN_INSTALL_GLOBAL_DIR or $BUN_INSTALL_BIN already produces.
|
Warning Review limit reached
Next review available in: 41 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: fix and tests pushed (244a51e), waiting on CI. Reproduced on the release build of main with The new cases in |
There was a problem hiding this comment.
LGTM — swaps the unchecked path join for the checked variant and folds the four identical blocks into one helper, matching the established pattern.
What was reviewed:
make_open_dir_undermirrors the existingjoin_abs_string_buf_checkedusage inlockfile/Package.rsandWorkspaceMap.rs; theError::Sys(ENAMETOOLONG)return matches whatmake_open_pathalready produces for the direct-path arms.- Tests: all six env-var arms covered, boundary at exactly/one-above buffer size, and a positive control (100 KB value that normalizes short) pinning that the checked join measures the normalized result — hermetic, concurrent, POSIX-gated with reason.
- Checked that the 100 KB
x/../fill divides evenly so it normalizes back to${dir}, and that returning the disposed tempDir path fromrunGlobalis only used for string comparison.
Extended reasoning...
Overview
Two files. In src/install/PackageManager/PackageManagerOptions.rs, the four call sites in open_global_dir and open_global_bin_dir that joined an environment value ($BUN_INSTALL / $XDG_CACHE_HOME / $HOME) with a fixed suffix using the unchecked join_abs_string_buf are replaced by a single private helper make_open_dir_under that uses join_abs_string_buf_checked and returns crate::Error::Sys(ENAMETOOLONG) when the normalized result does not fit the PathBuffer. The open call (Dir::cwd().make_open_path(...).map(|d| d.into_raw()).map_err(Into::into)) is byte-identical to what each site did before. In test/cli/install/bun-pm.test.ts, a new describe.concurrent.skipIf(isWindows) block adds ten cases exercising every arm plus the exact boundary and a normalization control.
Security risks
None. The change strictly reduces attack surface: an environment-variable-driven out-of-bounds slice write (panic in release, but still a bounds violation) is replaced by an early ENAMETOOLONG error. No new inputs are read, no new syscalls are made, and the error value is the same one the adjacent $BUN_INSTALL_GLOBAL_DIR / $BUN_INSTALL_BIN arms already produce via openat_a.
Level of scrutiny
Low-to-medium. The Rust change is a mechanical refactor plus a one-function swap (join_abs_string_buf → join_abs_string_buf_checked), and that swap follows precedent already in the crate at lockfile/Package.rs:1735 and lockfile/Package/WorkspaceMap.rs:218,446 (referenced in the PR as #37531). The helper is private, the callers are unchanged, and crate::Error::Sys(bun_errno::SystemErrno::ENAMETOOLONG) is used verbatim elsewhere in src/install/bin.rs. The success path is provably identical because join_abs_string_buf_checked returns the same slice join_abs_string_buf would have when it fits.
Other factors
The tests are unusually thorough for this class of fix: they set every variable both lookups read (including $XDG_CONFIG_HOME with an .npmrc to keep the separate bunfig/npmrc joins from interfering), so each case reaches exactly the arm it names; they cover both entry points (bun pm bin -g and bun install -g); they pin the boundary at buffer-size and buffer-size+1; and the normalization control proves no regression for long-but-normalizable values. They follow harness conventions (tempDir, Buffer.alloc(n, fill), concurrent stdout/stderr/exited drain, describe.concurrent, exit-code asserted last). The PR description documents that 8/10 fail on main and all pass with the fix. No prior reviews or outstanding comments on the timeline.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 244a51e has some failures in 🧪 To try this PR locally: bunx bun-pr 38413That installs a local version of the PR into your bun-38413 --bun |
Problem
-gcommand (bun install -g,bun add -g,bun pm ls -g,bun pm bin -g,bun link, ...) aborts when$BUN_INSTALL, or when that is unset$XDG_CACHE_HOMEor$HOME, is longer than the path buffer (4096 bytes on Linux, 1024 on macOS):bun_paths::resolve_path::normalize_string_generic_tz,src/paths/resolve_path.rs:1071.) Repro on main:BUN_INSTALL=/$(head -c 5000 /dev/zero | tr '\0' a) bun pm ls -g.$BUN_INSTALL_GLOBAL_DIRor$BUN_INSTALL_BINalready fails cleanly witherror: An internal error occurred (ENAMETOOLONG), exit 1: those values are opened as given andopenat_a(src/sys/lib.rs:6334) rejects any path ofMAX_PATH_BYTESbytes or more.open_global_dirandopen_global_bin_dirinsrc/install/PackageManager/PackageManagerOptions.rs(lines 319, 332, 367 and 380 on main) join the environment value withinstall/global,.bun/install/global,binor.bun/binusing the uncheckedjoin_abs_string_bufinto a stackPathBuffer. It normalizes straight into the buffer, so a result that does not fit is an out of bounds slice write instead of an error.Fix
make_open_dir_under, which usesjoin_abs_string_buf_checkedand returnsError::Sys(ENAMETOOLONG)when the joined path does not fit; otherwise it opens the path exactly as before.MAX_PATH_BYTESbytes, whichopenat_arejects withENAMETOOLONGanyway, so the helper reports one layer earlier what a larger buffer would have reported. The error is the sameError::Sys(ENAMETOOLONG)value thatmake_open_pathproduces for an oversized$BUN_INSTALL_GLOBAL_DIR/$BUN_INSTALL_BIN, so every caller (PackageManager::init,setup_global_dir, the link/unlink commands) prints the message above and exits 1 without any caller changes.ENAMETOOLONGfromopenat_aas it does today. Same pattern as install: stop panicking on workspaces entries longer than the path buffer #37531 (workspaces entries) andlockfile/Package.rs(folder dependencies).test/cli/install/bun-pm.test.ts,describe("global directories longer than the path buffer"), POSIX only (on Windows the buffer holds more than an environment variable can carry). Each case sets every variable the two lookups read, so it reaches exactly the arm it names;$XDG_CONFIG_HOMEpoints at a directory with an.npmrcso the$HOMEcases reach this code regardless of the separate.npmrc(install: do not abort when $XDG_CONFIG_HOME or $HOME is too long for the .npmrc path buffer #38372) and bunfig (bunfig: stop panicking when the config path does not fit in a path buffer #38370) joins:bun pm bin -gwith an 8 KiB value in each of the six arms (global directory and global bin directory, from$BUN_INSTALL,$XDG_CACHE_HOME,$HOME):ENAMETOOLONG, exit 1bun install -gwith an 8 KiB$BUN_INSTALL: sameENAMETOOLONG, exit 1$BUN_INSTALLthat normalizes to a short directory:bun pm bin -gprints that directory'sbin, exit 0USE_SYSTEM_BUN=1, and a debug build withsrc/stashed) 8 of the 10 cases fail with the panics above (range end index 8192, and4096for the one byte above case); the exactly-the-buffer-size and normalized cases pass both ways and pin down the boundary. With the fix the whole file passes (28 tests), as do the-gcases ofbun-install-registry.test.ts;cargo clippy -p bun_installandrustfmtare clean.bun-pm.test.ts, so whichever lands second needs a trivial rebase. install: skip non-absolute $BUN_INSTALL when locating global dirs #32515, install: load bunfig before resolving globalDir for -g installs #35454 and install: stop resolving global install dirs from $XDG_CACHE_HOME #36492 touch these two functions for other reasons and keep the unchecked join, so this is independent of them.Background
PathBufferis bun's fixed stack buffer for building syscall paths,MAX_PATH_BYTESlong (the platformPATH_MAX: 4096 on Linux, 1024 on macOS, about 96 KiB on Windows).openat_acopies a path into one and adds the NUL, so the longest path it accepts isMAX_PATH_BYTES - 1bytes.resolve_path::join_abs_string_buf(base, buf, parts)ispath.resolveinto a caller buffer. It assumes the normalized result fits and indexesbufaccordingly.join_abs_string_buf_checkedis the variant for input of unbounded length: it normalizes first (into heap scratch when the input is large) and returnsNoneinstead of writing when the result is longer thanbuf.-gcommandsPackageManager::initopens the global directory (where the globalpackage.jsonandnode_moduleslive) andsetup_global_diropens the global bin directory (where bin links are created). The first choice for each is a directory named outright ($BUN_INSTALL_GLOBAL_DIR/ bunfigglobalDir,$BUN_INSTALL_BIN/ bunfigglobalBinDir), opened as given; the remaining choices are derived by joining$BUN_INSTALL, else$XDG_CACHE_HOME, else$HOMEwith a fixed suffix, which is the code changed here.Probes on the release build of main