Fix Bun.pathToFileURL crash with relative paths longer than PATH_MAX#29673
Conversation
|
Updated 2:19 PM PT - Apr 24th, 2026
❌ @robobun, your commit 31a2c88 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29673That installs a local version of the PR into your bun-29673 --bun |
|
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: 📥 CommitsReviewing files that changed from the base of the PR and between c8a668f8fb08c8c660366b02c0ed5e8141feeda0 and 31a2c88. 📒 Files selected for processing (2)
WalkthroughReplaced a fixed 4096-byte thread-local join buffer with a stack-fallback allocation sized from cwd + input length in path resolution; updated tests to cover extremely long relative paths and normalization with Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
| // The input is user-controlled and may be arbitrarily long. The | ||
| // threadlocal `join_buf` is only 4096 bytes, so use a stack-fallback | ||
| // allocator that heap-allocates for oversized inputs. | ||
| var sfa = std.heap.stackFallback(bun.MAX_PATH_BYTES * 2, bun.default_allocator); |
There was a problem hiding this comment.
This will be 32 KB of stack on Windows. Use a smaller max.
There was a problem hiding this comment.
Dropped to a fixed 4096-byte inline buffer in c8a668f8fb — same footprint as the previous threadlocal join_buf on all platforms, heap-allocates only when cwd + input exceeds that.
(The earlier CI failure was infrastructure — Image not found: linux-*-v30 agent provisioning errors, also affecting main.)
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct crash fix with tests; the stack-sizing nit is non-blocking (this runs on JS threads with multi-MB stacks) but worth a one-line follow-up.
Extended reasoning...
Overview
This PR fixes an index-out-of-bounds panic in Bun.pathToFileURL() when given a relative path that, joined with cwd, exceeds 4096 bytes. The change replaces the fixed 4096-byte threadlocal join_buf in ResolvePath__joinAbsStringBufCurrentPlatformBunString with a stack-fallback allocator sized to cwd.len + input.len + 2, and adds two regression tests in test/js/bun/util/fileUrl.test.js. ~12 lines of Zig + ~12 lines of test code.
Security risks
None introduced. The PR actually removes a latent hazard: the previous code wrote past the end of a fixed-size threadlocal buffer for long inputs (a panic in debug, but potentially silent memory corruption in release if bounds checks were elided). The new code allocates exactly enough space, clones the result into a bun.String, then frees the buffer — no UAF or leak.
Level of scrutiny
Low-to-moderate. This is a small, focused fuzzer-found crash fix in a single exported helper, following the same stackFallback + alloc pattern already used by joinAbsStringBufChecked and JoinScratch in the same file. The buffer-size math (cwd.len + input.len + 2) is correct since the normalized output cannot exceed the unnormalized concatenation. No CODEOWNERS apply to this path.
Other factors
The one inline nit (outer stackFallback(MAX_PATH_BYTES * 2) doubling the inner JoinScratch stack footprint to ~384KB on Windows) is valid as a style/convention concern — joinAbsStringBufChecked uses 1× for exactly this reason — but it is not a correctness issue: Bun.pathToFileURL is a JS host function called from threads with 4–18MB stacks, and on POSIX the extra is only ~8KB. The author can trivially drop the * 2 in a follow-up without blocking this crash fix. Tests cover both the long-flat-segment case and the long-path-that-normalizes-down case, and the PR description confirms they crash the unfixed build.
There was a problem hiding this comment.
LGTM — the 4096-byte stackFallback addresses the stack-size feedback while keeping the common case zero-alloc.
Extended reasoning...
Overview
This PR fixes an OOB panic in Bun.pathToFileURL() when given a relative path that, joined with cwd, exceeds 4096 bytes. The fix replaces the fixed 4096-byte threadlocal join_buf output buffer in ResolvePath__joinAbsStringBufCurrentPlatformBunString with a stackFallback(4096)-backed allocation sized to cwd.len + input.len + 2, so short paths remain zero-alloc and long paths heap-allocate. Two regression tests were added.
Feedback addressed
The first revision used stackFallback(bun.MAX_PATH_BYTES * 2), which Jarred flagged for excessive stack on Windows and which I separately flagged for stacking on top of JoinScratch's own inline buffer. Commit c8a668f switched to stackFallback(4096, ...), which is the same size as the original threadlocal buffer — so there is no stack-usage regression vs. the pre-PR code, and both review comments are resolved.
Security risks
None. The change converts an OOB write/panic on user-controlled input into a bounded heap allocation. bun.handleOom guards the allocation. The output buffer bound cwd.len + str.len + 2 is safe: joinAbsStringBuf normalizes with allow_above_root=false and no NT prefix, so the result cannot exceed the concatenated input length.
Level of scrutiny
Low-to-moderate. ~10 LOC in a single exported helper used only by Bun.pathToFileURL, plus tests. The pattern (stackFallback + alloc + defer free) matches joinAbsStringBufChecked in the same file. The maintainer already reviewed the approach and the requested change was applied.
Other factors
Bug-hunter found no issues on the current revision. Tests cover both the flat long-segment case and the ..-normalization case. CI is building.
ResolvePath__joinAbsStringBufCurrentPlatformBunString used the 4096-byte threadlocal join_buf as the output buffer for normalization. When the input relative path (joined with cwd) exceeded 4096 bytes, normalizeStringGenericTZ would index past the end of the buffer. Use a stack-fallback allocator sized to cwd + input instead, matching Node.js which returns a file URL for arbitrarily long paths.
bun.MAX_PATH_BYTES * 2 is ~192KB on Windows. Use 4096 bytes of inline stack (same as the previous threadlocal join_buf) and heap-allocate when cwd + input exceeds that.
robobun
force-pushed
the
farm/630ecabe/pathToFileURL-long-path
branch
from
c8a668f to
31a2c88
Compare
There was a problem hiding this comment.
LGTM — the 4096-byte stackFallback addresses the earlier stack-size feedback and keeps the same footprint as the old threadlocal buffer.
Extended reasoning...
Overview
This PR fixes an out-of-bounds panic in Bun.pathToFileURL() when the relative input + cwd exceeds 4096 bytes. The change is confined to one exported function in src/resolver/resolve_path.zig (replacing the fixed threadlocal join_buf with a stackFallback(4096)-backed buffer sized to cwd.len + input.len + 2) plus two regression tests in test/js/bun/util/fileUrl.test.js.
Security risks
None. The change removes an unchecked write past a fixed buffer (which previously panicked in debug and was UB-adjacent in release). The new sizing is a strict upper bound: normalization never produces output longer than its concatenated input, so cwd.len + input.len + 2 is safe. OOM is handled via bun.handleOom.
Level of scrutiny
Low-to-moderate. The diff is ~10 lines of mechanical buffer-management code following the same pattern already used by joinAbsStringBufChecked in the same file. The output is immediately cloned into a bun.String before the buffer is freed, so there's no lifetime hazard.
Other factors
- Jarred's review comment ("use a smaller max") and my own earlier inline comment about doubled
MAX_PATH_BYTESstack usage were both addressed in c8a668f8fb by switching to a fixed 4096-byte inline buffer — identical stack footprint to the previous threadlocal on all platforms. - The two new tests directly exercise the crash (6000-char segment) and the
..-normalization path (20000-char input that collapses tocwd/final). - CI failures are unrelated:
socket.test.tsis a known Windows flake (just loosened on main in #29663), and the Linux build-cpp jobs failed on agent provisioning, not compilation. - The bug-hunting system found no issues on the current revision.
|
CI on build 47802 — 272/275 passed. The 3 failures are pre-existing on main and reproduce on unrelated PRs:
|
…ven-sh#29673) ## What does this PR do? `Bun.pathToFileURL()` crashed with an index-out-of-bounds panic when given a relative path that, when joined with cwd, exceeded 4096 bytes. `ResolvePath__joinAbsStringBufCurrentPlatformBunString` passed the fixed 4096-byte threadlocal `join_buf` as the output buffer to `joinAbsStringBuf`. The inner normalizer writes the result into that buffer and does not bound-check against it, so long inputs overflowed: ``` panic(main thread): index out of bounds: index 5738, len 4095 resolver.resolve_path.normalizeStringGenericTZ src/resolver/resolve_path.zig:915 ... Bun::functionPathToFileURL src/bun.js/bindings/BunObject.cpp:793 ``` The fix uses a stack-fallback allocator sized to `cwd.len + input.len + 2` for the output buffer, so short paths stay zero-alloc and long paths heap-allocate. This matches Node.js, which happily returns a file URL for arbitrarily long paths. ## How did you verify your code works? ```js Bun.pathToFileURL("a".repeat(6000)).href.length // before: panic // after: 6022 (same as Node.js) ``` Added tests in `test/js/bun/util/fileUrl.test.js` covering both a long flat segment and a long path that normalizes down via `..` segments. Verified the new tests crash the unfixed debug build and pass with the fix. `zig:check-all` passes on all targets. Found via Fuzzilli (fingerprint `d8774a764480e01d`). --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
…ven-sh#29673) ## What does this PR do? `Bun.pathToFileURL()` crashed with an index-out-of-bounds panic when given a relative path that, when joined with cwd, exceeded 4096 bytes. `ResolvePath__joinAbsStringBufCurrentPlatformBunString` passed the fixed 4096-byte threadlocal `join_buf` as the output buffer to `joinAbsStringBuf`. The inner normalizer writes the result into that buffer and does not bound-check against it, so long inputs overflowed: ``` panic(main thread): index out of bounds: index 5738, len 4095 resolver.resolve_path.normalizeStringGenericTZ src/resolver/resolve_path.zig:915 ... Bun::functionPathToFileURL src/bun.js/bindings/BunObject.cpp:793 ``` The fix uses a stack-fallback allocator sized to `cwd.len + input.len + 2` for the output buffer, so short paths stay zero-alloc and long paths heap-allocate. This matches Node.js, which happily returns a file URL for arbitrarily long paths. ## How did you verify your code works? ```js Bun.pathToFileURL("a".repeat(6000)).href.length // before: panic // after: 6022 (same as Node.js) ``` Added tests in `test/js/bun/util/fileUrl.test.js` covering both a long flat segment and a long path that normalizes down via `..` segments. Verified the new tests crash the unfixed debug build and pass with the fix. `zig:check-all` passes on all targets. Found via Fuzzilli (fingerprint `d8774a764480e01d`). --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
What does this PR do?
Bun.pathToFileURL()crashed with an index-out-of-bounds panic when given a relative path that, when joined with cwd, exceeded 4096 bytes.ResolvePath__joinAbsStringBufCurrentPlatformBunStringpassed the fixed 4096-byte threadlocaljoin_bufas the output buffer tojoinAbsStringBuf. The inner normalizer writes the result into that buffer and does not bound-check against it, so long inputs overflowed:The fix uses a stack-fallback allocator sized to
cwd.len + input.len + 2for the output buffer, so short paths stay zero-alloc and long paths heap-allocate. This matches Node.js, which happily returns a file URL for arbitrarily long paths.How did you verify your code works?
Added tests in
test/js/bun/util/fileUrl.test.jscovering both a long flat segment and a long path that normalizes down via..segments. Verified the new tests crash the unfixed debug build and pass with the fix.zig:check-allpasses on all targets.Found via Fuzzilli (fingerprint
d8774a764480e01d).