install(UpdateRequest): heap-allocate spec normalizer temp buffer#30064
Conversation
|
Updated 9:45 PM PT - May 1st, 2026
❌ @robobun, your commit fb59e85 has 3 failures in
DetailsDetails🧪 To try this PR locally: bunx bun-pr 30064That installs a local version of the PR into your bun-30064 --bun |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR addresses a potential buffer overflow in the package manager's input handling by replacing a fixed 2048-byte stack buffer with dynamic allocation. A regression test is added to ensure Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 37 seconds. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward fix replacing the fixed 2048-byte stack buffer with a heap allocation sized to the input, with a regression test.
Extended reasoning...
Overview
This PR fixes a stack buffer overflow in UpdateRequest.parseWithError (src/install/PackageManager/UpdateRequest.zig). The normalizer previously wrote the full positional into a fixed var temp: [2048]u8 via std.mem.replace, which has no capacity check; positionals >2048 bytes smashed the stack in ReleaseFast and panicked in debug. The fix heap-allocates temp with input.len bytes (the \\\\ → / replacement is 2→1 so output never exceeds input length), frees it via defer, and scopes platformToPosixInPlace to the initialized slice instead of the whole array. A regression test is added to bun-add.test.ts exercising an 8000-byte positional.
Security risks
The change removes a security risk (stack overflow reachable from CLI argv). The new code introduces no new attack surface: allocation is OOM-handled via bun.handleOom, the buffer is freed, and the slice math (temp[0 .. input.len - len]) is identical to before. The input.len == 0 edge case (all-whitespace positional) is safe — zero-length alloc and zero-length memcpy are well-defined.
Level of scrutiny
Low-to-moderate. This is a small, mechanical, self-contained change to a single block inside one function. The logic is unchanged except for buffer provenance and the (strictly more correct) narrowing of platformToPosixInPlace to initialized bytes. The code path is exercised by every bun add/remove/link/unlink invocation, so any regression would surface immediately in the existing test suite.
Other factors
The new test mirrors the adjacent "should reject invalid path without segfault" test in structure and assertions, and the PR description documents that it fails before the fix and passes after. No CODEOWNERS cover this path, no outstanding reviewer comments, and the bug-hunting system found no issues.
|
CI failures are unrelated to this change — Build #49952 (fb59e85, rebased on main):
None of these touch the |
std.mem.replace wrote the full positional argument into a fixed 2048-byte stack array with no capacity check. In ReleaseFast (slice safety off, stack_protector=false on x86) this smashed the stack and segfaulted for any 'bun add/remove/link' positional longer than 2048 bytes. The '\\' -> '/' replacement never grows the string, so a heap buffer of input.len bytes is always sufficient. Also scope platformToPosixInPlace to the initialized portion of the buffer.
robobun
force-pushed
the
farm/4d30c33c/fix-updaterequest-stack-overflow
branch
from
8c8dbc0 to
fb59e85
Compare
There was a problem hiding this comment.
LGTM — straightforward fix replacing the fixed 2048-byte stack buffer with a properly-sized heap allocation, with a regression test.
Extended reasoning...
Overview
This PR touches two files: src/install/PackageManager/UpdateRequest.zig (~10 lines changed in parseWithError) and test/cli/install/bun-add.test.ts (one new regression test). It replaces a fixed var temp: [2048]u8 stack buffer — into which std.mem.replace blindly copied the entire positional — with a heap allocation sized to input.len, and scopes platformToPosixInPlace to the initialized slice rather than the whole array.
Security risks
None introduced. This fixes a stack-buffer-overflow / segfault triggerable from CLI argv. The new allocation is bounded by the trimmed positional length (itself bounded by OS argv limits), uses bun.handleOom like the surrounding code, and is freed via defer. The 2→1 byte replacement can only shrink, so input.len is provably sufficient. Moving platformToPosixInPlace to operate on input2 instead of the full buffer also eliminates a scan over uninitialized stack memory on Windows.
Level of scrutiny
Low-to-moderate. The change is surgical, mechanically obvious once the root cause is understood, and follows the same allocation pattern (bun.handleOom(bun.default_allocator.…)) already used two lines above for input itself. std.mem.replace returns the replacement count, so temp[0 .. input.len - len] is the correct output slice — same arithmetic as before. No design decisions, no behavior change for inputs ≤ 2048 bytes.
Other factors
- Neither file is covered by CODEOWNERS.
- The new test mirrors the adjacent "should reject invalid path without segfault" test and passed on every CI platform that ran; the 3 build failures are unrelated (HTTP/2 flake, GitHub 403 rate limit, agent provisioning).
- The PR description includes a clear repro, root-cause analysis (debug bounds-check trace), and before/after verification.
- Bug-hunting system found no issues.
…en-sh#30064) ## Reproduction ```console $ bun add $(python3 -c 'print("a"*8000)') bun add v1.3.14 error: unrecognised dependency format: ============================================================ … panic(main thread): Segmentation fault at address 0x0 ``` Any positional > 2048 bytes passed to `bun add`/`remove`/`link`/`unlink` (and `bunx`, which goes through the same parser) segfaults release builds on Linux/macOS x86_64. ## Cause `UpdateRequest.parseWithError` normalizes each positional by running `std.mem.replace(u8, input, "\\\\", "/", &temp)` into a fixed `var temp: [2048]u8` on the stack. `std.mem.replace` copies the entire input into the output buffer with no capacity check — the output slice length is trusted. ReleaseFast disables slice safety and `stack_protector` is off on x86 (build.zig), so the overflow silently smashes the stack frame. The `positional` slice variable gets corrupted and the subsequent `Output.errGeneric("… {s}", .{positional})` dereferences garbage → SIGSEGV. In debug builds the same input hits Zig's bounds check: ``` panic(main thread): index out of bounds: index 2048, len 2048 mem.replace /vendor/zig/lib/std/mem.zig:3829 install.PackageManager.UpdateRequest.parseWithError UpdateRequest.zig:129 ``` ## Fix Heap-allocate `temp` sized to `input.len`. The `"\\\\"` → `"/"` replacement is 2→1 bytes and can only shrink, so `input.len` is always sufficient. Also scope `platformToPosixInPlace` to the initialized portion of the buffer instead of the whole array. ## Verification Added to `test/cli/install/bun-add.test.ts` (next to the existing "should reject invalid path without segfault" case): ``` USE_SYSTEM_BUN=1 bun test … -t 'longer than 2048' → FAIL (segfault) bun bd test … (without src fix) → FAIL (index out of bounds panic) bun bd test … (with fix) → PASS ``` After the fix the 8000-byte positional is gracefully rejected with `error: unrecognised dependency format: aaa…` and exit code 1. Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Reproduction
Any positional > 2048 bytes passed to
bun add/remove/link/unlink(andbunx, which goes through the same parser) segfaults release builds on Linux/macOS x86_64.Cause
UpdateRequest.parseWithErrornormalizes each positional by runningstd.mem.replace(u8, input, "\\\\", "/", &temp)into a fixedvar temp: [2048]u8on the stack.std.mem.replacecopies the entire input into the output buffer with no capacity check — the output slice length is trusted. ReleaseFast disables slice safety andstack_protectoris off on x86 (build.zig), so the overflow silently smashes the stack frame. Thepositionalslice variable gets corrupted and the subsequentOutput.errGeneric("… {s}", .{positional})dereferences garbage → SIGSEGV.In debug builds the same input hits Zig's bounds check:
Fix
Heap-allocate
tempsized toinput.len. The"\\\\"→"/"replacement is 2→1 bytes and can only shrink, soinput.lenis always sufficient. Also scopeplatformToPosixInPlaceto the initialized portion of the buffer instead of the whole array.Verification
Added to
test/cli/install/bun-add.test.ts(next to the existing "should reject invalid path without segfault" case):After the fix the 8000-byte positional is gracefully rejected with
error: unrecognised dependency format: aaa…and exit code 1.