install: do not abort when $XDG_CONFIG_HOME or $HOME is too long for the .npmrc path buffer - #38372
install: do not abort when $XDG_CONFIG_HOME or $HOME is too long for the .npmrc path buffer#38372robobun wants to merge 1 commit into
Conversation
…E does not fit the path buffer PackageManager::init joined $XDG_CONFIG_HOME and $HOME with .npmrc into a stack PathBuffer using the unchecked join, so a value of MAX_PATH_BYTES - 7 bytes or longer aborted every install-family command with an index out of bounds panic. Build the candidate with join_abs_string_buf_checked and treat a path that does not fit like a missing file, which is what opening it would have reported (ENAMETOOLONG) anyway.
|
Warning Review limit reached
Next review available in: 14 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 |
|
Updated 3:07 AM PT - Aug 14th, 2026
✅ @robobun, your commit d76ead521fb4fa4ac4dc0ea417b3a7a553d0ac18 passed in 🧪 To try this PR locally: bunx bun-pr 38372That installs a local version of the PR into your bun-38372 --bun |
|
Status: fix and tests pushed, waiting on CI. Reproduced on the release build of main (any directory with a package.json): mkdir -p /tmp/xdg # no .npmrc in it
XDG_CONFIG_HOME=/tmp/xdg HOME="/$(head -c 5000 /dev/zero | tr '\0' a)" bun pm cache
# panic: range end index 5000 out of range for slice of length 4095 (exit 134)With this branch the same command prints the cache directory and exits 0. An oversized |
There was a problem hiding this comment.
LGTM — targeted fix that swaps the unchecked path join for the existing checked variant, converting a panic on oversized $XDG_CONFIG_HOME/$HOME into the same "no user .npmrc" path that an unopenable file already takes.
What was reviewed:
- Verified
join_abs_string_buf_checkedsemantics againstsrc/paths/resolve_path.rs: the slow path leavesbufuntouched onNone, so a failed XDG candidate does not corrupt the buffer before the HOME candidate runs. - Checked NUL-terminator arithmetic:
buf[..MAX_PATH_BYTES-1]reserves the last byte, andbuf[len] = 0withlen ≤ MAX_PATH_BYTES-1is always in bounds; the downstreamZStr::from_buf(&buf[..], global_len)at line 1916 still sees the NUL it expects. - Confirmed the
./.npmrc→.npmrcchange is behavior-preserving (both normalize identically). - Tests cover the exact-fit boundary, one-past, and far-past cases; the boundary test builds a real 4088-byte directory to prove no readable
.npmrcis newly skipped.
Extended reasoning...
Overview
This PR fixes a panic in PackageManager::init (src/install/PackageManager.rs) when $XDG_CONFIG_HOME or $HOME is long enough that joining /.npmrc overflows the fixed PathBuffer. The fix extracts a small helper user_npmrc_path that uses the pre-existing resolve_path::join_abs_string_buf_checked instead of the unchecked _z variant, reserving one byte for the NUL terminator and returning None when the result doesn't fit. Both call sites (XDG and HOME) now go through the shared helper. Three new test cases in test/cli/install/npmrc.test.ts cover the boundary, one-byte-over, and far-over cases.
Security risks
None. The change converts a crash on attacker-controllable environment input into a graceful "file not found" — strictly a DoS-hardening improvement. No new codepaths read or write user data, no auth/crypto/permissions surface is touched, and the outcome for an oversized path is identical to what open() would return anyway (ENAMETOOLONG), which load_npmrc_config already handles by skipping.
Level of scrutiny
Low-to-medium. This is a small (~25 net lines of Rust), mechanical substitution of one join helper for its checked sibling — a pattern that already exists in-tree for exactly this purpose. I traced through join_abs_string_buf_checked in src/paths/resolve_path.rs to confirm it doesn't scribble on buf when returning None (it uses a separate scratch vec), so the XDG-then-HOME sequence can't leave stale data that gets misread. The NUL-terminator bookkeeping is correct: max len is MAX_PATH_BYTES - 1, so buf[len] = 0 never indexes past the buffer, and ZStr::from_buf's debug assertions (len < buf.len(), buf[len] == 0) hold.
Other factors
- The PR description is unusually thorough: it names both panic messages, cites exact line numbers, explains why treating oversized as absent is semantically correct (matches
openat_a's own>= MAX_PATH_BYTEScheck), and documents why the tests drive$HOMErather than$XDG_CONFIG_HOME(a sibling bug inarguments.rsfires earlier on that path — tracked separately in #38370). - Tests follow harness conventions:
tempDirwithusing,it.concurrent,Buffer.allocinstead of.repeat(), combined{stdout, stderr, exitCode}assertions via the existingpublishDryRun/usesRegistryhelpers, and askipIf(isWindows)with a stated reason. - The boundary test ("longest $HOME that fits") is a real regression guard — it proves the fix didn't shrink the set of readable
.npmrcfiles by one byte. - No CODEOWNERS entry covers
src/install/. No prior human reviews or outstanding comments on the PR.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35863, though the two are related.
|
Problem
bun install,bun pm ...,bun publishand the other commands that go throughPackageManager::initabort at startup when$XDG_CONFIG_HOME(or, when that directory has no.npmrc,$HOME) isMAX_PATH_BYTES - 7bytes or longer (4089 on Linux, 1017 on macOS):MAX_PATH_BYTES - 7bytes:panic: index out of bounds: the len is 4096 but the index is 4096panic: range end index 5000 out of range for slice of length 4095, top framebun_paths::resolve_path::normalize_string_generic_tz(src/paths/resolve_path.rs:1071), called frombun_install::package_manager_real::initsrc/install/PackageManager.rs:1887and:1894join the environment value and.npmrcinto a stackPathBufferwith the uncheckedjoin_abs_string_buf_z. It normalizes into the buffer without a size check and then writes the NUL terminator atresult.len(), so a result of exactlyMAX_PATH_BYTES - 1bytes fails on the NUL write and anything longer overflows inside the normalization.XDG_CONFIG_HOME=<short dir without .npmrc> HOME=/aaaa...<5000 bytes> bun pm cachein any directory with a package.json, exit 134). The values are environment input, so this should never be a crash.Fix
user_npmrc_path(dir, buf)buildsdir/.npmrcwithjoin_abs_string_buf_checkedintobuf[..MAX_PATH_BYTES - 1], writes the NUL itself, and returnsNonewhen the result does not fit. Both candidates ($XDG_CONFIG_HOME, then$HOME) use it;Noneleavesglobal_lenat 0, which is the existing "no user-level .npmrc" path.MAX_PATH_BYTESbytes or longer, whichopen()rejects withENAMETOOLONG(bun_sys::openat_achecks the same>= MAX_PATH_BYTESbound itself), andload_npmrc_configalready skips user-level candidates that fail to open (src/ini/lib.rs:1232). So the outcome is exactly what a larger buffer would have produced, and no.npmrcthat could be read before is skipped now: the longest directory that fits,MAX_PATH_BYTES - 8bytes, still loads, and its.npmrcpath is the longest oneopen()accepts.partsliteral changes from./.npmrcto.npmrc; the join normalizes both to the same path.test/cli/install/npmrc.test.ts("user .npmrc lookup > $HOME longer than the path buffer"): the longest$HOMEthat fits (built as a real directory of exactlyMAX_PATH_BYTES - 8bytes) still has its.npmrchonored,$HOMEone byte longer is skipped,$HOMElonger than the whole buffer is skipped. On the unfixed build the last two fail with the two panics above; with the fix all 34 tests in the file pass. Skipped on Windows, where the buffer (~96 KiB) is longer than any environment value.$HOMEcandidate (with$XDG_CONFIG_HOMEpointing at a short directory) because the global.bunfig.tomllookup insrc/bunfig/arguments.rsreads$XDG_CONFIG_HOME(or$HOMEwhen it is unset) before this code with the same unchecked join and a longer file name, so an oversized value there still aborts earlier; bunfig: stop panicking when the config path does not fit in a path buffer #38370 fixes that site. Both candidates here shareuser_npmrc_path, and the existing lookup tests in the samedescribecover the$XDG_CONFIG_HOMEcandidate when the path fits.cargo clippy -p bun_installandrustfmt --checkare clean.Background
PathBufferis bun's fixed stack buffer for path syscalls,MAX_PATH_BYTESlong (the platformPATH_MAX: 4096 on Linux, 1024 on macOS). Paths in it are NUL-terminated, so the longest path it can hold isMAX_PATH_BYTES - 1bytes, which is also the longest pathopen()accepts.resolve_path::join_abs_string_buf_z(cwd, buf, parts)ispath.resolveinto a caller buffer plus a NUL terminator; it assumes the result fits.join_abs_string_buf_checkedis the variant for input of unbounded length: it normalizes first and returnsNoneinstead of writing when the result is longer thanbuf, so it has to be given a buffer one byte short of the NUL slot the caller fills in..npmrclookup:PackageManager::inituses$XDG_CONFIG_HOME/.npmrcwhen that file exists, otherwise$HOME/.npmrc(install: fall back to $HOME/.npmrc when $XDG_CONFIG_HOME is set #36289), and passes the result together with the project.npmrctoload_npmrc_config, which ignores candidates it cannot read.