bundler: do not abort on a Bun.build files specifier longer than a path buffer - #40345
Conversation
…th buffer FileMap::resolve joined every non-absolute specifier onto the importer's directory with join_abs_string_buf, which writes into a fixed PathBuffer with no bounds check. A CSS url() or an import specifier longer than the buffer (4096 bytes on Linux, 1024 on macOS) panicked with "range end index N out of range for slice of length 4095" and aborted the process. Use join_abs_string_buf_checked and treat a result that does not fit as "not a virtual file", so the specifier falls through to the resolver (which marks data: URLs external and reports too-long paths as not found). Guard the importer path the same way, and bound the Windows separator normalization in get, contains and resolve, which copied the raw specifier into a PathBuffer. Fixes #39252
|
Warning Review limit reached
On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file. Or wait 16 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 8:05 AM PT - Aug 24th, 2026
✅ @robobun, your commit de882e34722db119f136041cb208d6c81cf09d4e passed in 🧪 To try this PR locally: bunx bun-pr 40345That installs a local version of the PR into your bun-40345 --bun |
|
Status: CI is green on de882e3 (Build #104926), ready for review. Reproduced on bun 1.4.0 and on a debug build of main with the script from #39252: Verification is in the PR description ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix replacing unchecked path-buffer writes with checked variants.
What was reviewed:
join_abs_string_buf_checked/abs_buf_checkedreturnOption, so?correctly falls through to the resolver;path_to_posix_bufwrites exactlypath.len()bytes, so the> buf.len()guards are the right bound.- Windows
get_key_value: returned key/value borrow fromself.map, not the local buffer, so no lifetime issue;contains/getsemantics unchanged. - Tests use 128 KiB specifiers (exceeds all platform limits), run in child processes since the bug is a crash, and follow harness conventions (
Buffer.alloc, concurrent pipe drain,describe.concurrent).
Extended reasoning...
Overview
The PR fixes a process abort in Bun.build({ files }) when an in-memory file contains an import specifier or CSS url() longer than a PathBuffer (4096 bytes on Linux, 1024 on macOS, 98302 on Windows). FileMap::resolve in src/bundler/bundle_v2.rs joined every non-absolute specifier onto the importer's directory with join_abs_string_buf, which indexes into a fixed-size buffer without bounds checking. The fix swaps in join_abs_string_buf_checked and abs_buf_checked (both already in-tree, returning Option), adds a length guard before each path_to_posix_buf call, and consolidates three copies of the Windows separator-normalization lookup into one get_key_value helper. Three new subprocess tests in test/bundler/bundler_files.test.ts cover the CSS url() case, a too-long relative import, and a too-long relative entry point.
Security risks
None. This turns a panic on adversarial-length user input into a graceful None that hands the specifier to the resolver, which already handles data: URLs and reports Could not resolve for oversized paths. No new trust boundaries, no allocation sized from untrusted input.
Level of scrutiny
Medium — bundler resolution is a hot, correctness-sensitive path, but the change is narrow and mechanical: three unchecked buffer writes become checked, plus a dedup refactor. I verified that path_to_posix_buf writes exactly path.len() bytes (so > buf.len() is the correct comparison, not >=), that abs_buf_checked and join_abs_string_buf_checked both return Option so the ? operator propagates correctly to the enclosing Option<bun_resolver::Result>, and that on Windows the new get_key_value returns references into self.map (not the local pooled buffer), so there is no dangling borrow. contains and get are now thin wrappers over get_key_value with identical semantics to before.
Other factors
The tests are well-constructed: they run in child processes because the bug aborts, use 131072-byte specifiers to exceed every platform's path-buffer size, use Buffer.alloc(n, fill).toString() per repo convention, drain stdout/stderr/exited concurrently, assert stderr and exitCode in the right order, and use describe.concurrent for the independent subprocess spawns. The PR description documents that all three tests abort on 1.4.0 and pass with the fix, and that the surrounding bundler test files still pass. The dedup of the three Windows normalization blocks into one helper is a net simplification with no behavior change on the existing paths.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted crash fix that swaps unchecked path-buffer writes for their checked variants and falls through to the resolver on overflow.
What was reviewed:
- Verified
path_to_posix_bufindexes intobuf[..path.len()]unchecked, so the newspecifier.len() > buf.len()guard inget_key_valueand theabs_source_filelength check are both required and correctly bounded. - Confirmed
join_abs_string_buf_checkedandabs_buf_checkedreturnNoneon overflow, so?propagation makesresolvefall through to the real resolver — same behavior as any non-matching specifier. - Checked the
get_key_valuerefactor preserves the priorget/contains/direct-match semantics on both cfg branches; only the Windows overflow guard is new. - Tests spawn a child (the bug is an abort), use 131072-byte specifiers to exceed every platform's path buffer, and assert exact
{success, logs}shapes.
Extended reasoning...
Overview
The PR fixes a process abort in Bun.build({ files }) when an in-memory file contains an import specifier or CSS url() longer than MAX_PATH_BYTES. FileMap::resolve in src/bundler/bundle_v2.rs joined every non-absolute specifier onto the importer's directory with join_abs_string_buf, which writes into a fixed-size PathBuffer with plain indexing. On Windows, get, contains and resolve also copied the raw specifier into a path buffer via path_to_posix_buf, likewise unchecked. The fix swaps in the _checked variants and adds explicit length guards before path_to_posix_buf, and consolidates three duplicated Windows-normalization blocks into one get_key_value helper. Three subprocess tests are added to test/bundler/bundler_files.test.ts.
Security risks
None. The change turns a user-reachable panic into a graceful None return, which is exactly what the review guide asks for ("user-reachable failures are recoverable errors, never panics"). No new trust boundary is introduced; the specifier simply falls through to the normal resolver, which already handles data: URLs and reports Could not resolve for oversize relative paths.
Level of scrutiny
Medium-low. This is a mechanical unchecked→checked substitution in a well-scoped helper. I read path_to_posix_buf (resolve_path.rs:2453) to confirm it panics when path.len() > buf.len(), so both new length guards are necessary and correctly use > rather than >=. I read join_abs_string_buf_checked (resolve_path.rs:1680) and abs_buf_checked (resolver/lib.rs:348) to confirm they return Option and that ? propagation is sound here — resolve already returns Option<bun_resolver::Result>, and None is the existing not-in-map signal, so callers need no change. The get_key_value refactor is a pure deduplication: get and contains on non-Windows still hit map.get_key_value directly, and the direct-match block in resolve is behavior-identical to the removed cfg-split code.
Other factors
The tests follow harness conventions: child process for the crash case, Buffer.alloc(n, fill) instead of .repeat(), concurrent subprocess spawns, drained stdout/stderr/exited together, exact-object assertions with the specifier normalized out of the log message. The 131072-byte specifier exceeds Linux (4096), macOS (1024), and Windows (98302) buffer sizes, so the tests exercise the guard on every platform. The comment-cop bot's earlier objections to long comments were addressed in ab3fc04 and 939574e and all threads are resolved. The PR description documents that the full bundler_files suite plus adjacent bundler suites and cargo check for Windows were run.
There was a problem hiding this comment.
LGTM — narrow crash fix that swaps unchecked path-buffer joins for their checked variants.
What was reviewed:
FileMap::resolve:join_abs_string_buf_checked/abs_buf_checkedreturnOption, and?correctly short-circuits toNoneso the specifier falls through to the resolver.- The
get_key_valuerefactor preserves the priorget/containssemantics on both cfg branches; the new Windows length guard is needed becausepath_to_posix_bufindexes intobufunchecked. - Tests use 128 KiB specifiers (exceeds every platform's
MAX_PATH_BYTES), run in a child process, drain both pipes concurrently, and assert exact structured output.
Extended reasoning...
Overview
This PR fixes a process abort in Bun.build({ files }) when an in-memory file contains an import specifier longer than MAX_PATH_BYTES. The fix touches FileMap in src/bundler/bundle_v2.rs (~40 lines net) and adds four regression tests to test/bundler/bundler_files.test.ts. Three unchecked path-buffer writes (join_abs_string_buf, abs_buf, path_to_posix_buf) become checked, and three duplicated Windows separator-normalization blocks are folded into one get_key_value helper.
Security risks
None. This turns a user-reachable panic (a DoS on the calling process) into a graceful None that lets the resolver report Could not resolve or treat a data: URL as external. No new inputs are accepted; the change only prevents overflowing a fixed-size buffer.
Level of scrutiny
Moderate. The bundler is a hot path, but the change is mechanical: each unchecked helper is swapped for its documented _checked sibling that returns Option, and ? propagates None out of a function that already returns Option<bun_resolver::Result>. The get/contains refactor is a straight de-duplication — contains_key → get_key_value(...).is_some() is semantically identical, and the non-Windows branch is unchanged aside from returning the key alongside the value. I confirmed path_to_posix_buf uses raw slice indexing (src/paths/resolve_path.rs:2458), so the added length guard before it is required.
Other factors
The tests are well-constructed: they spawn a child process (the bug is a crash), use a 128 KiB specifier that exceeds every platform's path-buffer size including Windows's 98302, drain stdout/stderr/exited concurrently, and assert an exact JSON shape rather than substring matching. describe.concurrent keeps the four subprocess spawns from serializing. The comment-cop feedback about long comments was addressed in ab3fc04/939574e and all threads are resolved. The PR description clearly scopes out the adjacent path-buffer panics tracked in #39626 and #38696, which live at different call sites.
2 participants
Problem
Bun.build({ files })aborts when an in-memory file holds a specifier longer than a path buffer:panic: range end index 4096 out of range for slice of length 4095(1024 on macOS, 98302 on Windows). Every loader reaches it: CSSurl()and@import, JSimport,import()andrequire(), HTML<script src>and<link href>. Any inlinedata:image over 4 KB in a virtual CSS file hits it. The same file from disk builds fine. Fixes Bun.build({ files }) panics on CSS data URLs at 1024 bytes #39252.FileMap::resolve(src/bundler/bundle_v2.rs:1022) treats every non-absolute specifier as relative and joins it onto the importer's directory withjoin_abs_string_buf, which writes into a fixedPathBufferwith no bounds check. On Windows,get,containsandresolvealso copy the raw specifier into aPathBuffer, unchecked.Fix
FileMap::resolvejoins withjoin_abs_string_buf_checkedand returnsNonewhen the result does not fit, the rule the resolver applies incheck_relative_path. The specifier then reaches the resolver, which marks adata:URL external and reportsCould not resolvefor a too-long path.abs_buf_checkedand a length check before its separator normalization.get_key_valuehelper replaces the three Windows separator normalizations. A specifier longer than a path buffer is never a key.test/bundler/bundler_files.test.ts, four new tests in a child process (CSSurl(), JS import, HTML references, entry point), all abort on 1.4.0. Alsobundler_plugin,bundler_defer,bundler_naming,html-import-manifest,css/doesnt_crash,cargo checkfor Windows.Background
files:is the in-memory file map ofBun.build. Before the resolver runs,FileMap::resolvechecks each import specifier against that map: by exact key, then joined onto the importer's directory.PathBufferis[u8; MAX_PATH_BYTES]: 4096 bytes on Linux, 1024 on macOS, 98302 on Windows.join_abs_string_bufnormalizes into it with plain indexing. The_checkedvariant returnsNoneinstead.data:URLs before any path join.Notes
Related PRs:
files:keys are stored (relative keys resolved against the cwd) and replaces the same join as part of that. It has been open since August 14. This PR is the minimal crash fix so that it can land on its own. Whichever lands second needs a small rebase inFileMap::resolve.load_as_file,load_extension) panics on an absolute import path or entry point longer than a path buffer, with or withoutfiles:. That is out of scope here.files:key whose relative form does not fit a path buffer (for example/+ 4090 bytes +.jswith no imports at all) panics ingeneric_path_with_pretty_initialized(relative_platform_buf) while the entry point's display path is computed. This PR does not reach that site.Repro on 1.4.0 and on main (
44411167):Stack on a debug build (the crash handler only prints the top frames in release):
Checked on the fixed build: quoted and unquoted
url(),@font-face src,https:and#fragmentURLs of 64 KiB all build. A 128 KiB relative, bare or dynamic import,require(), CSS@import, and HTML<script src>/<link href>reportCould not resolve, the same as from a disk file. A 128 KiBdata:URL in an HTML<img src>is kept. Each of these aborts on 1.4.0 (/usr/local/bin/bun,34cbb9a40). Relative imports between virtual files,..segments, and a relative CSSurl()to a virtual asset still resolve as before.With the fix, a too-long specifier skips the relative join and reaches the resolver. A virtual file whose key itself is longer than a path buffer cannot be found through a relative specifier. Such keys are not supported elsewhere in the bundler either (output path computation uses path buffers).
Sentry BUN-4S5H (1.4.1-canary
abe2ad4f0, Linux x64) is this crash, reached from a JSimportwhose specifier is./plus 2100a/segments. The same guards also cover the importer side: a virtual file whose key is longer than a path buffer, reached by an exact key match, used to abort inabs_buforpath_to_posix_bufon its first relative import.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 0
evidence per changed file