paths: heap-backed relative_alloc and join_abs_string_buf_spill; use them in _nodeModulePaths and the runtime linker - #38392
Conversation
…ver-long paths relative() and join_abs_string_buf() write into fixed PathBuffers without bounds checks, so callers that hand them unbounded input abort the process. relative_alloc was a Box around the thread-local relative() and inherited the bound; it now sizes heap buffers to its inputs when either input, or the ../ chain the result needs, might not fit, and join_abs_string_buf_spill is the path.resolve counterpart of join_z_buf_spill. Module._nodeModulePaths() joins whatever string JS passes (string manipulation in Node, with no length limit) and now spills; the runtime linker computes the pretty name of a path returned by an onResolve plugin with relative_alloc, so a plugin returning a path longer than a PathBuffer for an import inside a module is a resolve error instead of a crash.
|
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 (5)
WalkthroughPath resolution now supports oversized relative and absolute paths through dynamic allocation and spill buffers. Bundler and Node module callers use the new helpers. Native and JavaScript regression tests cover long-path errors, lookup chains, and buffer growth. ChangesOversized path handling
Possibly related PRs
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.
Beyond the inline findings, I traced the new relative_scratch_needed / relative_out_needed bounds against every write site inside relative_platform_in (the normalize-into-other-scratch step for a relative from, the normalize-into-buf step for a relative to, and the join_abs_string_buf output into each scratch buffer) — each is covered by the computed sizes, so the heap path can't under-allocate.
Extended reasoning...
The core correctness invariant of this PR is that the three heap buffers relative_alloc allocates are large enough for every intermediate write relative_platform_in performs, not just the final result. I checked: (1) relative from normalizes into to_buf (needs from.len()+1, covered since scratch >= top_level_dir.len()+from.len()+4); (2) relative to normalizes into out (needs to.len()+1, covered since out_needed >= scratch); (3) the join_abs_string_buf output into each scratch buffer is bounded by join_abs_needed(top_level_dir, [normalized]), which is exactly what relative_scratch_needed computes for the relative branch. The absolute branch's path.len()+2 covers the leading-separator prefix plus the +1-byte normalization growth. The inline findings (Windows test assertion, join_abs_needed not applied to _checked) stand on their own.
…n the long-result test
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Both earlier findings are addressed (the long-result test now uses a two-byte target so it holds on Windows, and join_abs_string_buf_checked goes through join_abs_needed). No new issues found on this pass. Because the fix hinges on hand-derived buffer bounds (relative_scratch_needed / relative_out_needed) that back unchecked slice writes in relative_to_common_path, a human look at those derivations would still be worthwhile.
What was reviewed:
- Traced
relative_out_neededagainstrelative_to_common_path's worst case (every 2-byte component offrom→ 3-byte/.., plus separator and fulltotail); the bound holds. - Checked
relative_scratch_neededagainst both the absolute (normalize_string_bufintobuf[1..], +1 growth) and relative (normalizeinto the sibling scratch, thenjoin_abs_string_buf) arms ofrelative_platform_in; both fit, including the relative-tonormalize into the output buffer. - Confirmed the three
linker.rssites already copied the result, sorelative_alloccosts nothing there beyond OOM propagation;resolver_jsc.rs's spill Vec is dropped at scope end. - The comment-cop flags on the current head land on 2–3-line bound doc comments, not workaround justifications.
Extended reasoning...
Overview
Fixes two user-reachable process aborts from over-long paths flowing into fixed-size PathBuffers: _nodeModulePaths(longString) and a runtime Bun.plugin onResolve returning a long file-namespace path. src/paths/resolve_path.rs gains join_abs_string_buf_spill (mirrors the existing join_z_buf_spill pattern) and reworks relative_alloc to size heap scratch/output when the thread-local PathBuffers can't hold the inputs or the ../ chain. relative_platform_buf is split into a thin wrapper plus relative_platform_in so the same body runs over caller-supplied buffers. Three call sites in src/bundler/linker.rs switch from relative to relative_alloc; src/jsc/resolver_jsc.rs switches to the spill variant. Seven new cargo test -p bun_paths unit tests plus two subprocess integration tests.
Security risks
The inputs are user-controlled (_nodeModulePaths argument, plugin onResolve result), and the pre-fix behavior is a process abort — a DoS class the PR removes. The new bounds (join_abs_needed, relative_scratch_needed, relative_out_needed) size buffers that normalize_string_generic_tz and relative_to_common_path write into with plain slice indexing, so an under-bound would still panic (not corrupt memory — safe Rust). I traced each writer against its bound and they hold, including the Windows +1 growth cases already accounted for in join_abs_needed.
Level of scrutiny
High. resolve_path is core infrastructure reached by every module the runtime links, and the fix's correctness rests on arithmetic bounds derived from the writer's control flow rather than a checked write. The _spill addition follows an established in-file pattern; the relative_alloc bound derivation is the novel part.
Other factors
My two earlier findings (Windows-failing unit test assertion; join_abs_string_buf_checked open-coding the shared bound) were both addressed in d2c0a7d and are marked resolved. The comment-cop bot fired again on the current head, but the flagged lines are short doc comments explaining why each bound is what it is — not workaround justifications — so I'd leave that to a maintainer to dismiss. The integration tests are subprocess-isolated, drain both pipes, and assert exact ResolveMessage shape / Node-derived expected lists; the PR description records USE_SYSTEM_BUN=1 failures for both. Given the bound-derivation subtlety and ~60 untouched relative() callers the PR explicitly leaves for a separate change, deferring to a human reviewer rather than approving.
|
Updated 5:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit ed7f0aa has some failures in 🧪 To try this PR locally: bunx bun-pr 38392That installs a local version of the PR into your bun-38392 --bun |
|
Status after the review rounds: d2c0a7d switches the long-result unit test to a two-byte target (the one-byte case trips a pre-existing Windows bug in relative(), reported separately) and routes join_abs_string_buf_checked through join_abs_needed; 620cfbe trims the comments and derives the relative scratch bound from join_abs_needed; ed7f0aa names that helper's parameter the way #37457 and #38368 do, since all three PRs add the identical helper (overlap note added to the description: whichever lands last drops its copy on rebase). Every review thread is resolved; the remaining bound docs are intentional, as the latest review summary also concluded. Unit tests pass on the final state; the branch is otherwise unchanged since the integration tests were last run. Waiting on CI. |
There was a problem hiding this comment.
LGTM — both earlier findings addressed (d2c0a7d: join_abs_needed shared with _checked, and b"/tt" in the long-result test so it holds on Windows).
Reviewed: the relative_platform_buf → relative_platform_in split is a pure body-move (thread-local callers unchanged), and relative_alloc's fast path calls the same relative_platform as before, so every existing caller with real filesystem paths is byte-identical. The heap path is only reached for inputs that previously panicked; traced relative_out_needed's 3/2·scratch bound against relative_to_common_path's .. emission loop and it holds (first .. is 2 bytes, each subsequent /.. is 3, matching ≥2-byte components). join_abs_string_buf_spill mirrors join_z_buf_spill exactly.
Extended reasoning...
Overview
Two user-reachable panics from over-long paths flowing into fixed-size PathBuffer writes: _nodeModulePaths(longString) and a runtime Bun.plugin onResolve returning a long path. Fix adds heap-spilling variants (join_abs_string_buf_spill, a real relative_alloc) that follow the existing *_spill pattern in this file, plus a mechanical split of relative_platform_buf into a wrapper (supplies thread-local buffers) and relative_platform_in (takes buffers as parameters, body unchanged). Three call sites in linker.rs and one in resolver_jsc.rs switch to the new variants. Seven new cargo unit tests pin the bounds; two new integration tests reproduce both crashes end-to-end.
Prior review resolved
I left two findings on the first revision; both are addressed in d2c0a7d and visible in the current diff:
join_abs_string_buf_checkednow callsjoin_abs_needed(cwd.len(), parts)instead of open-coding the loop.relative_alloc_handles_a_result_longer_than_a_path_buffernow usesb"/tt"(two-byte target) so the exact-value assertion holds on Windows too, with a one-line comment noting the pre-existing one-byte-target quirk.
The comment-cop noise was resolved in 620cfbe (comments trimmed; the remaining doc comments on the bound helpers are one-to-two lines stating where the bytes come from, matching the sibling join_z_buf_spill / normalize_string_spill docs).
Security risks
None material. This is pure byte-slice string manipulation with no filesystem, network, or privilege boundary. Rust bounds-checks every slice write in normalize_string_generic_tz and relative_to_common_path, so an under-sized bound would panic (the same failure class as the bug being fixed), not corrupt memory. The unsafe added is two #[no_mangle] extern "C" test stubs for highway kernels, matching the two already present in the same mod tests.
Level of scrutiny
Moderate. Path handling is core infrastructure, but the change is structured to minimize risk: (1) the relative_platform_buf split moves the body verbatim — every existing caller of relative() / relative_platform() runs identical code; (2) relative_alloc's fast path (scratch ≤ MAX_PATH_BYTES && out_needed ≤ MAX_PATH_BYTES) calls the same relative_platform as before, so its two pre-existing callers (chunk naming, sourcemap sources) are unchanged for real paths; (3) the heap path only handles inputs that previously crashed. I traced the relative_out_needed bound: relative_to_common_path emits .. (2 bytes) then /.. (3 bytes) per component, and each component in the normalized from is ≥2 bytes (leading separator + name), so 3/2·scratch covers the .. chain; + 1 + scratch covers the separator and to tail. relative_scratch_needed covers the leading-separator byte plus the one byte Windows normalization can add (share root / C: → C:.), and for relative inputs reuses join_abs_needed with +1 for the same reason. join_abs_string_buf_spill is a straight copy of join_z_buf_spill's shape.
Other factors
Tests are well-constructed per REVIEW.md: the unit tests pin the fast-path/spill boundary exactly (join_abs_string_buf_spill_uses_the_buffer_up_to_its_bound), the Windows growth case, and the ../-chain overflow with two inputs that individually fit; the integration tests spawn a child (crash isolation), assert the specific ResolveMessage fields including the long path in the message, cover both CJS and ESM parents, and build expected _nodeModulePaths output with path.join rather than string literals so they hold cross-platform. The PR description confirms USE_SYSTEM_BUN=1 fails both. Windows was cargo checked but not cargo tested — CI will cover it, and the one Windows-specific test issue I found earlier is now fixed.
|
Ready for a maintainer at head ed7f0aa. Both new tests (_nodeModulePaths past PATH_MAX; onResolve returning a 150 KB path from a CJS and an ESM parent) passed on every lane of build 95718, and the 25 bun_paths unit tests pass; the lanes that build reports as failed are retry-passed flakes in unrelated files (retry-flag, inspect-error-leak, child_process IPC handle, HTTP/3 on Windows aarch64, malformed-integrity-base64, cluster tests), so no retrigger from my side. Review threads are all resolved; the description carries the note about #37457 / #38368 adding the same join_abs_needed helper, so whichever of the three lands last just drops its copy on rebase. |
2 participants
Problem
require("module")._nodeModulePaths(p)with aplonger than a path buffer aborts the process:panic: range end index 5000 out of range for slice of length 4095(Node returns the lookup list for any string; this is string manipulation). Site:node_module_paths_js_valueinsrc/jsc/resolver_jsc.rsjoins the argument onto the cwd withjoin_abs_string_bufinto a pooledPathBuffer.Bun.pluginonResolvecallback that returns afile-namespace path longer than a path buffer for an import orrequireinside a module aborts the process:panic: range end index 150000 out of range for slice of length 4095. Site:Linker::generate_import_path(src/bundler/linker.rs) computes the import's display name withresolve_path::relative(source_dir, plugin_path). The 1.5 xMAX_PATH_BYTESspecifier cap inVirtualMachine::resolvedoes not apply here because the path never goes through it before the linker; a 70 KB or 150 KB path still aborts. Both CJS and ESM parents hit it.normalize_string_generic_tz(src/paths/resolve_path.rs), which writes into whatever buffer it is given with plain slice indexing.relative()normalizes both inputs into thread-localPathBuffers and writes the result into a third;relative_allocwas just aBoxaround that result, so it had the same bound. The result buffer is also too small for valid inputs:relativeof two paths that each fit aPathBufferaborts once the../chain plus the tail does not (pinned by a unit test in this PR).Fix
relative_platform_bufis split into a wrapper that supplies the thread-local buffers andrelative_platform_in, which takes all three buffers as slices. Its body is unchanged.relative_alloccomputes how much scratch each input needs (normalized, plus the cwd for a relative input, using the boundJoinScratch::initalready uses) and how long the result can get (every component offromis at least two bytes and becomes at most a three-byte/.., then a separator and the tail ofto); when all three fit inMAX_PATH_BYTESit calls the thread-localrelativeexactly as before, otherwise it runs the same code in heap buffers of those sizes. Its two existing callers (chunk directory templates, source mapsources) get this for free.join_abs_string_buf_spillis added next tojoin_abs_string_buf_checked: same shape asjoin_z_buf_spill, joining into the caller's buffer when the bound (join_abs_needed, now shared withJoinScratch::initandjoin_abs_string_buf_checked) says the result fits and into a caller-ownedVecotherwise. Unlike the_checkedvariant it returns the result however long, which is what_nodeModulePathsneeds.join_abs_needed(same name, signature and formula, wired into the same two existing sites) plus a thread-localjoin_abs_string_spill, thejoin_spillcounterpart of the caller-buffer variant added here; the two variants are complementary, likejoin_spillandjoin_z_buf_spill. Whichever of the three lands last drops its copy ofjoin_abs_neededon rebase; nothing else overlaps._nodeModulePathsjoins through the spill variant; the rest of the function is slicing and formatting and needed no change. All three arms ofgenerate_import_pathuserelative_alloc(theAbsolutePatharm is the runtime one and the only one reproduced; the other two take the samesource_pathand already copied the result, so the change costs nothing there). The load that follows then fails with the resolver's normalENAMETOOLONGResolveMessage.cargo test -p bun_paths: 25 pass, 7 new (spill fast path, spill, the exact bound, a Windows share root that grows by a byte,relative_allocmatchingrelative, a target longer than aPathBuffer, and a result longer than aPathBufferfrom two inputs that fit; the last two abort on the thread-localrelative, checked with a temporary#[should_panic]copy). The last test uses a two-byte target because the Windows arm ofrelativedrops a one-byte root-level target (C:\d\dtoC:\tgives..\..), a pre-existing bug found while writing it and reported separately.test/js/node/module/node-module-module.test.js, "_nodeModulePaths() accepts paths longer than PATH_MAX": a child bun runs a 100000-byte component (longer than the buffer on Windows too), 100 components of 6 KB total, and a 5000-byte relative input; the output is compared against the lists Node produces (built withpathin the test).test/js/bun/plugin/plugins.test.ts, "an onResolve result longer than a path buffer is a resolve error, not a crash": a preload plugin returns a 150001-byte path (past the buffers and the specifier cap on every platform) for./child.js, required by one parent and re-exported by an ESM parent; both must surface aResolveMessagenaming the path and the parent.USE_SYSTEM_BUN=1: both tests fail with the panics above.bun bd teston this branch: both pass; the fullplugins.test.ts(39) andnode-module-module.test.js(40) files pass, as dobundler_splitting,bundler_naming,bundler_plugin,bun-build-apiand thetest/js/bun/sourcemapfiles (therelative_alloccallers).cargo checkof the touched crates for the Windows and macOS targets andcargo clippyare clean.relative()itself (about 60 callers, most with paths that came from the filesystem) and the other unchecked writers are left as they are: making them grow in place touches the install crate's direct use of the relative buffers and is the writer-level change being discussed separately; bun test: stop panicking on a path argument or tree entry longer than the path buffer #35863 boundsjoin_abs_string_buffrom the other direction and does not overlap with these lines. The resolver'sload_as_filewindow reached by the plugin path at 4096 to 6144 bytes is resolver: bound load_as_file path before writing into its PathBuffer #35857's site.Background
bun_paths::resolve_pathworks on byte slices and writes results into caller-provided or thread-local fixed buffers (PathBuffer=[u8; MAX_PATH_BYTES]: 4096 on Linux, 1024 on macOS, 98302 on Windows). The*_spillfamily (join_spill,join_z_buf_spill,normalize_string_spill) is the existing pattern for inputs that may be longer: use the fixed buffer when a cheap bound says the result fits, otherwise aVecthe caller owns.join_abs_string_bufispath.resolve: it concatenates the cwd and the parts into a scratch buffer and normalizes that into the output buffer, so the output is never longer than the concatenation (plus one byte Windows normalization can add, e.g.C:becomingC:.).relativeispath.relative: it normalizesfromandto(resolving relative ones against the cwd), finds their common prefix, and emits one..per remaining component offromfollowed by the rest ofto.onResolveplugins for specifiers that look like they could be plugin-handled (an extension or ans:prefix), and rewrites each record to the resolved path plus a shorter "pretty" name used for display._nodeModulePathsis thenode:modulehelper that returns thenode_moduleslookup chain for a directory;module.pathsis computed with it lazily.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/module/node-module-module.test.js