Conversation
…h buffer The cache directory is joined from XDG_CACHE_HOME or HOME, which can be longer than a PathBuffer. The unchecked joiner then panics inside normalize_string_buf while the first file above MINIMUM_CACHE_SIZE is loaded. Use the checked joiner into a buffer that also reserves room for the cache file name, and treat a directory that does not fit, including an over-long BUN_RUNTIME_TRANSPILER_CACHE_PATH, as "cache disabled".
|
Status: ready for review. Reproduced on the 1.4 canary (4c68990) and on the debug build: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe transpiler cache now centralizes filename sizing, validates cache paths against fixed buffer limits, rejects overlong paths, and uses checked joins for default directories. Tests cover borderline and oversized paths without cache creation. ChangesTranspiler cache path handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the fix is correct and self-contained. One minor test-hygiene nit inline (Windows USERPROFILE), but it doesn't affect the platforms where the panic actually reproduces.
Extended reasoning...
Overview
This PR fixes a panic in RuntimeTranspilerCache::really_get_cache_dir when HOME / XDG_CACHE_HOME is close to MAX_PATH_BYTES — the unchecked join_abs_string_buf_z overran the fixed PathBuffer. The fix swaps to join_abs_string_buf_checked into a sub-slice that reserves room for the cache filename, and returns 0 (cache disabled) when the directory doesn't fit. It also lifts CACHE_FILE_SUFFIX / CACHE_FILE_NAME_LEN / CACHE_FILE_NAME_RESERVE to module constants so the reserve is derived from what write_cache_filename actually writes, and collapses the three per-platform join calls into one via cfg!. A regression test covers all three env-var routes at 4095 and 4200 bytes.
Security risks
None. This is defensive bounds handling on environment-variable input feeding a fixed path buffer; the failure mode changes from a panic to "cache silently disabled", which is the intended contract for every other failure in this module.
Level of scrutiny
Moderate — a small refactor of a single function plus constant extraction. I verified:
join_abs_string_buf_checkedreturnsSome(&buf[..len])starting at index 0 (both fast and slow paths), sobuf[len] = 0places the NUL correctly.- The
#[cfg(target_os = "macos")]→cfg!(...)rewrite preserves candidate order and the "no HOME → cache disabled" fallthrough exactly. - Boundary arithmetic: at the maximum
cache_dir_len == MAX_PATH_BYTES - CACHE_FILE_NAME_RESERVE,get_cache_file_pathwrites the NUL atbuf[MAX_PATH_BYTES - 1], which is in bounds. - The over-long
BUN_RUNTIME_TRANSPILER_CACHE_PATHbranch now returns 0 instead of silently truncating toMAX_PATH_BYTES - 1— visible behavior (no cache) is unchanged, just decided earlier.
Other factors
The one inline nit (HOME variant on Windows reads USERPROFILE, so it inherits the runner's real profile) is a test-hermeticity issue only; on Windows MAX_PATH_BYTES is ~98 KB so a 4 KB path never triggers the guard regardless, and the panic only reproduces on Linux/macOS where the test is correct. The fix itself is sound on all platforms.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the USERPROFILE fix from the earlier review is in, and the checked-join change looks correct.
What was reviewed:
join_abs_string_buf_checkedreturn semantics vs. the manual NUL write andCACHE_FILE_NAME_RESERVEarithmetic — the exact-fit case (len == dir_buf.len()) still leaves room for separator + name + NUL atbuf[MAX_PATH_BYTES-1].#[cfg(target_os = "macos")]→cfg!refactor preserves candidate order and the no-HOME → disabled result.- The three comment-cop flags are on ordinary 2–4 line doc comments (derived-constant descriptions and the function contract), not workaround justifications; the FileSystem comment at :712 predates this PR with one word changed.
Extended reasoning...
Overview
Two files: src/jsc/RuntimeTranspilerCache.rs (really_get_cache_dir and write_cache_filename) and a new test in test/cli/run/transpiler-cache.test.ts. The fix swaps the unchecked join_abs_string_buf_z for join_abs_string_buf_checked into a sub-slice sized to leave room for the cache file name, and returns 0 (cache disabled) when the environment-derived directory does not fit. The suffix and filename length are hoisted to constants so write_cache_filename and the reserve share one source of truth. The three duplicate join calls collapse into one via cfg! instead of #[cfg].
Security risks
None. The input is an environment variable the user already controls; the failure mode being fixed was a bounds-checked panic, not memory corruption. The change tightens bounds handling and disables an optional cache on overflow.
Level of scrutiny
Medium. Runtime path-buffer arithmetic in Rust, so a mistake would panic rather than corrupt, and the transpiler cache is designed to be silently skippable on any error. I traced the reserve arithmetic end to end: get_cache_dir now returns at most MAX_PATH_BYTES - CACHE_FILE_NAME_RESERVE, and get_cache_file_path then writes at most through buf[MAX_PATH_BYTES - 1]. The join_abs_string_buf_checked slow path allows joined.len() == buf.len(), which still fits because the NUL goes into the reserved tail of the full PathBuffer, not dir_buf. NLL releases the dir_buf reborrow before each buf[...] = 0 write, so this compiles.
Other factors
- The
#[cfg]→cfg!change is behavior-preserving: on macOS with HOME set the old code always returned from the cfg block, and both call the sameenv_var::HOME.get(), so the fallback.bun/install/cachebranch was already unreachable there. - My earlier finding (Windows reads
USERPROFILE, notHOME) was addressed in 25edd97 and the thread is resolved. - The three open
github-actionscomment-cop flags fire on the new constant doc comments, the extendedreally_get_cache_dirdoc comment, and the pre-existingFileSystemcomment (one word changed). None of them are workaround justifications; the file already carries far longer comments. I'm treating them as heuristic noise. - Test covers all three env-var routes at two lengths, runs the six spawns concurrently, and the sibling tests in the file establish that
toSpawnfails on non-zero exit (so a panic would be caught). The PR body confirms the HOME/XDG variants panic on the current build.
|
Updated 3:00 AM PT - Aug 20th, 2026
✅ @robobun, your commit da43b73a5e95ba904fa3e9d812f40115c100a4f6 passed in 🧪 To try this PR locally: bunx bun-pr 39705That installs a local version of the PR into your bun-39705 --bun |
There was a problem hiding this comment.
LGTM — the checked joiner and reserved-tail arithmetic look correct, and the earlier USERPROFILE feedback is addressed. The remaining inline nit is only about the wording of a test comment.
The unresolved comment-cop flag on RuntimeTranspilerCache.rs:680 is on the two-line doc comment for really_get_cache_dir; that's a normal function doc, not a workaround justification, so I'd treat it as a false positive.
Extended reasoning...
Overview
Two files: src/jsc/RuntimeTranspilerCache.rs (hoists CACHE_FILE_SUFFIX/CACHE_FILE_NAME_LEN/CACHE_FILE_NAME_RESERVE constants, rewrites write_cache_filename to use them, and rewrites really_get_cache_dir to join into buf[..MAX_PATH_BYTES - CACHE_FILE_NAME_RESERVE] via join_abs_string_buf_checked, returning 0 on overflow) and test/cli/run/transpiler-cache.test.ts (adds a 6-variant test over HOME/USERPROFILE, XDG_CACHE_HOME, and BUN_RUNTIME_TRANSPILER_CACHE_PATH at 4095 and 4200 bytes).
Correctness of the refactor
- Candidate order is unchanged:
BUN_RUNTIME_TRANSPILER_CACHE_PATH→XDG_CACHE_HOME→HOME→ 0. The macOS#[cfg]→cfg!fold is behavior-preserving: on macOS the old code returned from theLibrary/Cachesarm before ever reaching the.bun/install/cachearm, so collapsing them into anif cfg!(macos)inside a singleHOMEbranch yields the same result on every platform. - Buffer arithmetic checks out:
join_abs_string_buf_checked's slow path (resolve_path.rs:1698) bounds the result at<= dir_buf.len(), solen <= MAX_PATH_BYTES - CACHE_FILE_NAME_RESERVEandbuf[len] = 0is in range; then inget_cache_file_path,total = len + 1 + CACHE_FILE_NAME_LEN <= MAX_PATH_BYTES - 1, sobuf[total] = 0is in range andwrite_cache_filename'sENOSPCbranch is now unreachable in practice (kept as a defensive guard, which is fine). The explicit-path branch'sdir.len() > dir_buf.len()guard makes the subsequentcopy_from_sliceand NUL write safe. join_abs_string_buf_checkeddoes not NUL-terminate (unlike the_zvariant it replaces); the new code writesbuf[len] = 0explicitly, soget_cache_dir's contract is preserved.
Security risks
None. The input is process environment (already trusted at this layer), and the change strictly narrows what reaches the fixed buffer. The old truncation of BUN_RUNTIME_TRANSPILER_CACHE_PATH to 4095 bytes is replaced with a clean disable, which is the safer direction.
Level of scrutiny
Medium. Native path-buffer arithmetic on user-controllable input, but the fix is a straight swap to an existing size-checked helper plus derived constants, and the failure mode on any mistake is a Rust bounds-check panic (the very thing being fixed), not memory corruption. The one refactor beyond the minimal fix — folding three join calls into one via cfg! — is small and traced above.
Other factors
- My previous review (Windows
USERPROFILEleaking throughbunEnv) was addressed in 25edd97; the HOME variant now sets both. - Three comment-cop flags were resolved in 32ac850; the fourth, still open on line 680, targets the two-line
really_get_cache_dirdoc comment and reads as a bot false positive. - The one finding this run is a nit on the trailing
existsSync(cache_dir)comment wording — the assertion itself is not vacuous (it guards theundefinedoverride), only the comment overstates its scope. Not merge-blocking. - The test panics on the pre-fix build per the PR description, and the other 13 tests in the file still pass, so the
USE_SYSTEM_BUN=1criterion is met for the HOME/XDG variants.
There was a problem hiding this comment.
LGTM — small, well-scoped panic fix with a regression test.
Checked: join_abs_string_buf_checked returns Option and does not NUL-terminate, so the manual buf[len] = 0 is required and always in bounds (dir length is capped at MAX_PATH_BYTES - CACHE_FILE_NAME_RESERVE, leaving room for SEP + filename + NUL in get_cache_file_path). The #[cfg] → cfg! refactor preserves the macOS candidate order (XDG → ~/Library/Caches → disabled). Both earlier review notes (USERPROFILE on Windows, the trailing-assertion comment) were addressed in 25edd97 and da43b73.
Extended reasoning...
Overview
Two files: src/jsc/RuntimeTranspilerCache.rs swaps join_abs_string_buf_z for the size-checked join_abs_string_buf_checked when resolving the transpiler cache directory from BUN_RUNTIME_TRANSPILER_CACHE_PATH / XDG_CACHE_HOME / HOME, and lifts the cache-file suffix and name length into module constants so really_get_cache_dir can reserve exactly enough room for get_cache_file_path to append the separator, hex hash, suffix and NUL. test/cli/run/transpiler-cache.test.ts gains one test spawning six variants (three env vars × two lengths) that used to panic on Linux/macOS.
Security risks
None. The change only decides whether the on-disk transpiler cache is disabled when an environment variable is pathologically long; the disabled state was already reachable via BUN_RUNTIME_TRANSPILER_CACHE_PATH=0. No new filesystem paths are derived, no untrusted data is parsed, and the fallback (transpile without caching) is the safe default.
Level of scrutiny
Low-to-medium. This is a targeted fix for a reachable panic in a non-critical subsystem (the cache is a performance optimization; every failure in it is already designed to be silent). The diff is ~60 lines net, mostly consolidating three duplicated join calls into one and hoisting literals into named constants. I verified the arithmetic: with cache_dir_len ≤ MAX_PATH_BYTES - (1 + CACHE_FILE_NAME_LEN + 1), get_cache_file_path writes SEP at cache_dir_len, the filename at cache_dir_len+1, and NUL at cache_dir_len+1+CACHE_FILE_NAME_LEN ≤ MAX_PATH_BYTES-1, so no index is out of range. The #[cfg(target_os = "macos")] → cfg! change is behavior-preserving: on macOS, HOME set → Library/Caches, HOME unset → disabled, exactly as before (the old second HOME branch was dead on macOS because the first one returned).
Other factors
The two comments I left on earlier revisions (Windows USERPROFILE leak making the HOME variant non-hermetic; the trailing assertion's comment overstating what it checks) were both addressed in follow-up commits and the threads are resolved. The comment-cop bot's long-comment complaints were shortened in 32ac850. The new test follows the file's existing conventions (bunRun, toSpawn, Buffer.alloc over .repeat, concurrent spawns via Promise.all), and the PR description confirms the other 13 tests in the file still pass. No design decisions here that need a human — this is a mechanical bounds fix at the layer that owns the invariant.
|
Closing in favor of #43067. It fixes this trigger with the shared checked path helpers and carries the tests from this pull request. |
2 participants
Problem
HOMEorXDG_CACHE_HOMEof about 4 KB, running any file of 4 KiB or more dies withpanic: index out of bounds: the len is 4095 but the index is 4095(orrange end index 4099 out of range for slice of length 4095),Crashed while parsing <file>. Bun 1.3 ran the file.RuntimeTranspilerCache::really_get_cache_dir(src/jsc/RuntimeTranspilerCache.rs:705and:730before this change) joins the variable into a fixedPathBufferwithjoin_abs_string_buf_z, which does not check the size. The panic is innormalize_string_bufduring the cache lookup for the first file large enough to be cached.Fix
join_abs_string_buf_checkedinto the firstMAX_PATH_BYTES - CACHE_FILE_NAME_RESERVEbytes of the buffer. A directory that does not fit returns 0, which the caller already treats as "cache disabled" (the stateBUN_RUNTIME_TRANSPILER_CACHE_PATH=0produces). An over-longBUN_RUNTIME_TRANSPILER_CACHE_PATHtakes the same exit instead of being cut to 4095 bytes.write_cache_filenamewrites (CACHE_FILE_SUFFIX,CACHE_FILE_NAME_LEN, now used there too), soget_cache_file_pathalways has room for the separator, the name and the NUL.test/cli/run/transpiler-cache.test.ts, "disables the cache when the cache directory does not fit in a path buffer" (HOME,XDG_CACHE_HOME, explicit path, at 4095 and 4200 bytes). It panics on the current build and passes with the fix. The other 13 tests still pass.Background
<hash>.pileunder$XDG_CACHE_HOME/bun/@t@,~/.bun/install/cache/@t@orBUN_RUNTIME_TRANSPILER_CACHE_PATH. Every failure in it is meant to be silent: Bun transpiles instead.PathBufferis a fixed[u8; MAX_PATH_BYTES](4096 on Linux, 1024 on macOS). An environment variable has no such limit.join_abs_string_buf_checkedis the existing size checked joiner. It returnsNonewhen the normalized result does not fit.Notes
HOMEfrom about 4073 bytes up (HOMEplus/.bun/install/cache/@t@no longer fits). A 4061 byteHOMEalready worked: only the file name failed to fit, whichwrite_cache_filenamereports asENOSPCand the callers swallow. The cut downBUN_RUNTIME_TRANSPILER_CACHE_PATHended in thatENOSPCpath on every lookup too, so its visible behavior (no cache) is unchanged, it is just decided in one place now. That variant of the test passes before and after, theHOMEandXDG_CACHE_HOMEvariants are the ones that panic.CACHE_FILE_NAME_RESERVEis 29 bytes in debug builds (separator, 16 hex digits,.debug.pile, NUL) and 23 in release.#[cfg]tocfg!so the three candidate part lists share one join call. Candidate order and the "no HOME means no cache" result (test "disables the cache instead of falling back to the shared temp directory") are unchanged..pile2). Both are independent of this fix. Whichever lands second needs a small rebase.HOME,XDG_CACHE_HOMEandBUN_RUNTIME_TRANSPILER_CACHE_PATHof 4096 bytes run an 84 KB file, and normal values still write a.debug.pileentry.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file