fix OOB in pathToFileURL with long relative paths#28464
Conversation
|
Updated 4:17 AM PT - Mar 26th, 2026
❌ @robobun, your commit 1ae1141 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 28464That installs a local version of the PR into your bun-28464 --bun |
|
Verified: CI passing (lint, auto-label, pipeline ✅; Buildkite build in progress, no failures). Heap-allocated buffer size ( |
|
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 (1)
WalkthroughPath-joining now selects between a thread-local buffer and a heap allocation based on required size; a new test spawns a Bun subprocess to verify Bun.pathToFileURL with a very large relative path (200,000 bytes). Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/url/pathToFileURL.test.ts`:
- Line 15: The test currently checks url.href.endsWith(longPath) which can
falsely pass if the cwd and relative input are concatenated without a separator;
update the assertion in the Bun.pathToFileURL test to check
url.href.endsWith("/" + longPath) (i.e., include the "/" separator before
longPath) so the check also verifies correct join semantics between cwd and the
provided path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80a92358-d92a-4b69-a7ce-a07169d781a8
📒 Files selected for processing (2)
src/resolver/resolve_path.zigtest/js/node/url/pathToFileURL.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean fix for the OOB write; the two nits found are minor and do not affect correctness.
Extended reasoning...
Overview
This PR modifies ResolvePath__joinAbsStringBufCurrentPlatformBunString in src/resolver/resolve_path.zig to guard against an OOB write when pathToFileURL is called with a relative path whose combined length with CWD exceeds the 4096-byte threadlocal join_buf. It also adds a regression test in test/js/node/url/pathToFileURL.test.ts.
Security risks
None. The previous code was the security/stability risk (OOB write / panic). The fix eliminates it by heap-allocating when the input is large. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low. The change is a single-function fix with a clear before/after. The heap path is already used by many other callers in the same file (e.g., joinStringBufT and JoinScratch), and bun.handleOom + defer free is the established pattern. The fix is correct: total = cwd.len + input.len + 2 is a conservative upper bound for the output of joinAbsStringBuf, so the allocated buffer is always large enough.
Other factors
Two nits were flagged by the bug hunter: (1) the test does not consume proc.stderr, which could deadlock if stderr exceeds the OS pipe buffer, and (2) the buffer size check uses < instead of <=, causing an unnecessary heap allocation at the boundary. Neither affects correctness of the fix itself. The off-by-one is a minor performance nit, and the stderr issue is unlikely to trigger given the subprocess only calls console.log. The core fix is sound and the PR is small and self-contained.
| expect([path.join(import.meta.dir, "pathToFileURL-leak-fixture.js")]).toRun(); | ||
| }); | ||
|
|
||
| test("pathToFileURL handles relative paths longer than 4096 bytes", async () => { |
There was a problem hiding this comment.
🟡 The new test uses plain test() instead of test.concurrent(), violating the project convention in test/CLAUDE.md. Since this file now contains two process-spawning tests (the existing leak test using .toRun() and this new one using Bun.spawn), both should use test.concurrent to avoid unnecessarily serializing CI time.
Extended reasoning...
What the bug is and how it manifests
test/CLAUDE.md line 22 explicitly states: "Prefer concurrent tests over sequential tests: When multiple tests in the same file spawn processes or write files, make them concurrent with test.concurrent or describe.concurrent unless it is very difficult to make them concurrent." The PR adds a second process-spawning test to pathToFileURL.test.ts using test() instead of test.concurrent().
The specific code path that triggers it
The file now has two process-spawning tests: (1) test("pathToFileURL doesn't leak memory", ...) at line 5 using .toRun() which calls Bun.spawnSync internally, and (2) test("pathToFileURL handles relative paths longer than 4096 bytes", ...) at line 9 using Bun.spawn() directly. Neither test uses test.concurrent. With sequential execution, the second test cannot start until the first subprocess has fully completed.
Why existing code does not prevent it
The first test (leak test) was written before there was a second process-spawning test in the file, so the CLAUDE.md rule did not apply at that time. The PR introduced the second process-spawning test without updating either test to use test.concurrent.
What the impact would be
CI time is unnecessarily serialized. These two tests are completely independent -- there is no shared state, no shared files, and no ordering dependency between them. Running them concurrently would reduce wall-clock time for this test file by roughly the runtime of the slower of the two subprocess executions.
How to fix it
Change line 9 from test("pathToFileURL handles relative paths longer than 4096 bytes", async () => { to test.concurrent("pathToFileURL handles relative paths longer than 4096 bytes", async () => {. Optionally, also update the existing leak test to use test.concurrent.
Step-by-step proof
- Bun test runner executes tests in a file sequentially by default when
test()is used. "pathToFileURL doesn't leak memory"runs first, spawning a subprocess via.toRun()which callsBun.spawnSync.- The test runner waits for the first test to complete before starting the second.
"pathToFileURL handles relative paths longer than 4096 bytes"then runs, spawning its own subprocess viaBun.spawn.- With
test.concurrent, both tests would be launched in parallel, reducing total file execution time by the overlap between the two subprocess lifetimes.
There was a problem hiding this comment.
LGTM — the OOB fix is correct and all prior review comments have been addressed.
Extended reasoning...
Overview
This PR modifies ResolvePath__joinAbsStringBufCurrentPlatformBunString in src/resolver/resolve_path.zig to conditionally heap-allocate when cwd.len + input.len + 2 > join_buf.len (4096 bytes), preventing an out-of-bounds write in normalizeStringGenericTZ. A regression test in test/js/node/url/pathToFileURL.test.ts spawns a subprocess with a 5000-byte relative path and asserts exit 0 and a correctly-formed URL.
Security risks
The original bug was itself an OOB write, so this PR reduces risk. The heap-allocation path is straightforward: bun.handleOom wraps the alloc, the buffer is freed immediately via defer, and the result is cloned into a bun.String before the buffer is freed. No security concerns in the fix itself.
Level of scrutiny
Moderate — the Zig change is small and self-contained, touching only one exported function. The boundary condition (<= so total == join_buf.len uses the stack buffer, whose max output is total - 1 bytes, safely within bounds) was previously flagged and correctly fixed. The test validates the crash-free path.
Other factors
All three prior review comments (separator assertion, stderr pipe/deadlock, <= boundary) were addressed in commit 4f09afa. The one remaining nit — stderr: "ignore" deviating from the canonical CLAUDE.md subprocess pattern — is minor and already captured as an inline comment. It does not affect correctness of the regression test.
|
All 57 non-macOS checks pass consistently across both CI runs. The 4 darwin test failures are infrastructure issues (runners expired or killed with exit -1/exit 2) — macOS builds compile fine, only test runners can't acquire machines. This matches the pattern seen across other PRs merging today. |
ResolvePath__joinAbsStringBufCurrentPlatformBunString uses a fixed 4096-byte threadlocal join_buf as the output buffer for joinAbsStringBuf. When a relative path from JS exceeds this size (combined with the CWD), the normalizeStringGenericTZ memcpy writes past the buffer boundary. Heap-allocate a sufficiently sized buffer when the input is too large for the fixed join_buf.
…arator in assertion
robobun
force-pushed
the
farm/e9c12352/fix-pathtofileurl-oob
branch
from
67e6b74 to
9160f90
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/url/pathToFileURL.test.ts (1)
2-2:⚠️ Potential issue | 🟡 MinorAssert the long-path suffix instead of only checking that the child exits cleanly.
This currently proves the subprocess does not crash, but it never checks that
Bun.pathToFileURL()returns the right URL. A bad slow-path join that drops the separator or truncates the tail would still pass here.🧪 Suggested change
-import { bunExe } from "harness"; +import { bunEnv, bunExe } from "harness"; @@ -test("pathToFileURL handles relative paths longer than 4096 bytes", () => { - expect([bunExe(), "-e", 'Bun.pathToFileURL("a".repeat(200000))']).toRun(); +test("pathToFileURL handles relative paths longer than 4096 bytes", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + 'const longPath = Buffer.alloc(200000, "a").toString(); const url = Bun.pathToFileURL(longPath); console.log(url.href.endsWith("/" + longPath));', + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const stdout = await new Response(proc.stdout).text(); + const stderr = await new Response(proc.stderr).text(); + + expect(stdout).toBe("true\n"); + expect(stderr).toBe(""); + expect(await proc.exited).toBe(0); });As per coding guidelines, "Use
Buffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests" and "In tests, expect stdout before exit code: expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0) for more useful error messages."Also applies to: 9-11
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/js/node/url/pathToFileURL.test.ts` at line 2, Update the test in pathToFileURL.test.ts to assert the actual URL suffix returned by Bun.pathToFileURL rather than only checking the child process exit code: build the long tail using Buffer.alloc(count, "A").toString() (not 'A'.repeat), run the subprocess (using bunExe), capture stdout and assert expect(stdout).toEndWith(expectedSuffix) (or expect(stdout).toBe(expectedUrlSuffix) as appropriate) before asserting expect(exitCode).toBe(0), and ensure the expectedSuffix matches the correctly joined path separator and tail so a missing separator or truncated tail will fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/js/node/url/pathToFileURL.test.ts`:
- Line 2: Update the test in pathToFileURL.test.ts to assert the actual URL
suffix returned by Bun.pathToFileURL rather than only checking the child process
exit code: build the long tail using Buffer.alloc(count, "A").toString() (not
'A'.repeat), run the subprocess (using bunExe), capture stdout and assert
expect(stdout).toEndWith(expectedSuffix) (or
expect(stdout).toBe(expectedUrlSuffix) as appropriate) before asserting
expect(exitCode).toBe(0), and ensure the expectedSuffix matches the correctly
joined path separator and tail so a missing separator or truncated tail will
fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b53e669-6522-4944-ad2d-f81cb8ffcc16
📒 Files selected for processing (2)
src/resolver/resolve_path.zigtest/js/node/url/pathToFileURL.test.ts
There was a problem hiding this comment.
LGTM — the Zig fix is correct, the <= boundary is right, and the test has been cleaned up per prior feedback.
Extended reasoning...
Overview
This PR touches two files: src/resolver/resolve_path.zig (the core fix) and test/js/node/url/pathToFileURL.test.ts (regression test). The change adds a size check before using the 4096-byte thread-local join_buf, falling back to a short-lived heap allocation when cwd.len + input.len + 2 > join_buf.len.
Security risks
No security-sensitive code is touched. The OOB write being fixed was itself a potential safety issue (could be triggered by user-supplied paths), and the fix is conservative and correct.
Level of scrutiny
The change is small and mechanical. The only non-trivial question is whether the buffer-size math is correct. joinAbsStringBuf outputs at most cwd.len + 1 + input.len = total - 1 bytes into buf, so total <= join_buf.len is the correct boundary (the <= in the final diff is right; my prior comment about < vs <= was addressed). The heap allocation size total = cwd.len + input.len + 2 is sufficient.
Other factors
My previous inline comment (#2990492743) raised three issues: (1) double bunExe() in .toRun() — fixed, (2) .repeat() vs Buffer.alloc — fixed, (3) missing URL correctness assertion — not addressed, but this is genuinely minor since .toRun() catches the crash regression that motivated the PR. CI passes on non-macOS runners (macOS failures are infrastructure-only per robobun).
There was a problem hiding this comment.
LGTM — all prior review issues addressed.
Extended reasoning...
Overview
Two files changed: src/resolver/resolve_path.zig (the OOB fix) and test/js/node/url/pathToFileURL.test.ts (regression test). The fix is a small, self-contained change to ResolvePath__joinAbsStringBufCurrentPlatformBunString.
Security risks
None introduced. The fix actually removes a memory-safety issue (OOB write into a fixed 4096-byte threadlocal buffer). The heap fallback uses bun.handleOom and the allocation is freed via defer immediately after the result is cloned into a bun.String.
Level of scrutiny
Low. The logic is straightforward: compute total = cwd.len + input.len + 2, use the stack buffer when it fits (total <= join_buf.len), otherwise heap-allocate exactly total bytes. The <= boundary is correct because joinAbsStringBuf writes at most cwd.len + 1 + input.len = total - 1 bytes into the output buffer, which always fits inside a total-sized allocation.
Other factors
All issues from my previous reviews have been resolved in the final commits: (1) <= instead of < for the boundary check, (2) stderr handling in the test, (3) double bunExe() removed, (4) Buffer.alloc used instead of slow .repeat(), and (5) URL correctness assertion added (u.href.endsWith("/" + p)). The only outstanding nit is test.concurrent — minor CI-performance convention, not a correctness issue. CI is passing on non-macOS platforms (macOS failures are infrastructure timeouts unrelated to this PR).
There was a problem hiding this comment.
LGTM — all previously raised concerns have been addressed.
Extended reasoning...
Overview
This PR modifies ResolvePath__joinAbsStringBufCurrentPlatformBunString in src/resolver/resolve_path.zig to fix an OOB write when cwd + input exceeds the 4096-byte thread-local join_buf, and adds a regression test in test/js/node/url/pathToFileURL.test.ts.
Security risks
The original bug was an out-of-bounds write — a potential memory safety issue. The fix is conservative: compute total = cwd.len + input.len + 2, use the thread-local buffer when total <= join_buf.len, and heap-allocate otherwise. The heap-allocated buffer is freed immediately after the result is cloned into a bun.String. No security risks are introduced by this change.
Level of scrutiny
This is a narrow, well-scoped bug fix in path resolution. The code change is small (8 lines replaced with ~10 lines), the logic is straightforward, and the boundary math is correct: joinAbsStringBuf writes at most cwd.len + 1 + input.len = total - 1 bytes, so total <= join_buf.len is the right guard for the 4096-byte buffer.
Other factors
Multiple review cycles addressed earlier concerns: the <= boundary (vs <), Buffer.alloc instead of .repeat(), no double bunExe() prefix, and a URL correctness assertion (endsWith("/" + p)). The regression test now verifies both crash-freedom and correct join semantics. All inline comments are resolved. No bugs were found by the bug hunting system.
|
Build #42163: 53/58 checks pass. 5 expired runners (4 macOS test, 1 Windows baseline zig build). All tests that ran passed across Linux x64/aarch64, Linux musl, Alpine, ASAN, Windows x64/aarch64, and macOS (zig builds). Zero code-related failures across 7 consecutive CI runs. |
There was a problem hiding this comment.
LGTM — all prior review feedback has been addressed and the fix is correct.
Extended reasoning...
Overview
This PR modifies ResolvePath__joinAbsStringBufCurrentPlatformBunString in src/resolver/resolve_path.zig to avoid writing past the end of the 4096-byte threadlocal join_buf when the combined CWD + input length exceeds that limit. A test in test/js/node/url/pathToFileURL.test.ts exercises the long-path case.
Security risks
The original OOB write was a memory-safety bug; the fix eliminates it. No new security risks are introduced — the heap allocation is bounded by cwd.len + input.len + 2, used only to pass to joinAbsStringBuf, and immediately freed after the result is cloned into a bun.String.
Level of scrutiny
The change is narrow and mechanical: a size check selects between the existing stack buffer and a short-lived heap buffer. The logic is straightforward and the regression test directly exercises the previously-crashing code path with a 200,000-byte relative path and asserts both no-crash and correct URL semantics.
Other factors
All three issues I raised in prior reviews were resolved across successive commits: (1) < corrected to <= for the boundary check, (2) Buffer.alloc instead of .repeat() in the test, (3) double bunExe() removed, and (4) URL correctness assertion added. CI shows 53/58 checks passing with only expired macOS/Windows runner failures unrelated to this change.
There was a problem hiding this comment.
LGTM — all prior review comments have been addressed.
Extended reasoning...
Overview
This PR modifies ResolvePath__joinAbsStringBufCurrentPlatformBunString in src/resolver/resolve_path.zig (2 files changed, ~18 lines). The fix adds a size check before using the 4096-byte threadlocal join_buf, falling back to a short-lived heap allocation when cwd.len + input.len + 2 > join_buf.len. The regression test in pathToFileURL.test.ts uses .toRun() with a 200 000-byte path and validates both no-crash and correct URL form.
Security risks
None. The fix eliminates an OOB write; it introduces no new attack surface. The heap-allocated buffer is immediately freed after cloneUTF8 copies the result into a bun.String.
Level of scrutiny
Low-to-medium. This is a targeted, self-contained bug fix in a single exported C function. The logic (choose stack vs heap based on input size) is straightforward and the boundary <= is correct: joinAbsStringBuf writes at most cwd.len + 1 + input.len = total - 1 bytes, so total <= join_buf.len (i.e. max output ≤ 4095) safely fits the 4096-byte buffer.
Other factors
All issues I raised in previous rounds have been resolved: the <= boundary, Buffer.alloc instead of .repeat(), removal of double bunExe(), and the URL correctness assertion inside the subprocess. CI failures across 7 consecutive runs are purely infrastructure (expired macOS/Windows runners), with zero code-related failures.
|
Superseded by #29673 which applies a similar fix (using stackFallback allocator) that has already been merged to main. |
1 participant
What
Fix an out-of-bounds write in
Bun.pathToFileURL()when called with a relative path longer than ~4096 bytes.Root Cause
ResolvePath__joinAbsStringBufCurrentPlatformBunStringpasses the fixed 4096-byte threadlocaljoin_bufas the output buffer tojoinAbsStringBuf. When the input path (combined with CWD) exceeds the buffer size,normalizeStringGenericTZwrites past the end of the buffer:Fix
When the total length (CWD + input + separators) exceeds
join_buf, heap-allocate a buffer of the right size instead. The result is cloned into abun.Stringimmediately after, so the allocation is short-lived.Repro
Verified by robobun: CI Build #41383 in progress on commit 67e6b74 (retry of #41364 whose only failures were unrelated macOS webview timeouts). Lint JS ✅. Traced
_joinAbsStringBufto confirmbufoutput is bounded bycwd.len + 1 + input.len; heap-alloc sizecwd.len + input.len + 2is sufficient. Boundary<=is correct (max outputtotal - 1fitsjoin_buf.len). Regression test spawns subprocess with 5000-byte relative path, asserts exit 0 and URL ends with"/" + longPath— would OOB-panic on main. All 3 review comments (separator assertion, stderr, boundary) addressed in 4f09afa. No TODO/FIXME/HACK. No unrelated changes.