fix(Bun.serve): HEAD response Transfer-Encoding/Content-Length freed before write - #30155
Conversation
… value freed before use doRenderHeadResponse() read the Transfer-Encoding / Content-Length header values via fastGet(), which returns a ZigString borrowing the header map entry's StringImpl. For ASCII values toSlice() keeps borrowing. It then called renderMetadata(), which in doWriteHeaders() calls fastRemove(.TransferEncoding/.ContentLength) and derefs the FetchHeaders, dropping the only reference to that StringImpl when the map is the sole owner. The subsequent writeHeader() / parseInt() read freed memory. Clone the Transfer-Encoding value before renderMetadata(); parse Content-Length before renderMetadata() so the borrowed slice is never touched after the map entry is gone.
|
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)
Review rate limit: 0/5 reviews remaining, refill in 10 minutes and 49 seconds. Comment |
|
Updated 8:19 PM PT - May 2nd, 2026
❌ @robobun, your commit 1dfb0d9 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30155That installs a local version of the PR into your bun-30155 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the lifetime fix (clone for Transfer-Encoding, parse-before-free for Content-Length) looks correct and the ASAN regression test is solid — but this is a memory-safety change in the core Bun.serve request path, so it's worth a human confirming the renderMetadata() / fastRemove lifetime reasoning before merge.
Extended reasoning...
Overview
This PR fixes a heap-use-after-free in doRenderHeadResponse() (src/bun.js/api/server/RequestContext.zig). fastGet(.TransferEncoding/.ContentLength) returns a ZigString that borrows the header map's StringImpl bytes; the subsequent renderMetadata() call invokes doWriteHeaders(), which fastRemove()s those headers and derefs the FetchHeaders, freeing the borrowed bytes before they are written to the socket / parsed. The fix: (1) Transfer-Encoding now uses bun.handleOom(transfer_encoding.toSliceClone(...)) so the bytes are owned and outlive renderMetadata(); (2) Content-Length is parsed into a usize and the slice is deinit()'d before renderMetadata() runs, avoiding a clone since only the integer is needed afterwards. A new subprocess test in bun-server.test.ts reproduces the sole-owner-StringImpl condition (duplicate header entries → makeString() combine) under Malloc=1 so ASAN builds catch a regression.
Security risks
The bug being fixed is itself a memory-safety issue (UAF on the HTTP response write path). The fix strictly tightens lifetimes — cloning or consuming the borrowed bytes earlier — and introduces no new untrusted-input parsing, auth, or crypto surface. No new security risk introduced; if anything this removes one.
Level of scrutiny
High. RequestContext.zig is the per-request hot path for Bun.serve — every HTTP response flows through here. The diff is small (~10 LOC of logic), but correctness depends on subtle ownership reasoning across fastGet / renderMetadata / doWriteHeaders / fastRemove / swapInitHeaders. I verified ZigString.toSliceClone returns an owned Slice (via toOwnedSlice) and that bun.handleOom is the standard unwrap pattern, and the Content-Length reorder is straightforward. Still, memory-lifetime changes in the server core warrant a maintainer's eyes rather than bot-only approval.
Other factors
No CODEOWNERS match these files. No prior human reviews on the PR. CI build was still in progress at review time. The bug-hunting system found no issues. The added test is well-constructed (Windows-guards Malloc=1, disables LSan/symbolizer appropriately) and asserts both wire output and clean stderr/exit, so regressions should be caught in ASAN lanes.
Status✅ Reproduced both UAFs under ASAN with CIThe one red job ( |
…before write (oven-sh#30155) ## Repro ```js Bun.serve({ port: 0, fetch: () => new Response("hello", { headers: [ ["Transfer-Encoding", "gzip"], ["Transfer-Encoding", "chunked"], ], }), }); // HEAD / → ASAN heap-use-after-free in uWS::HttpResponse::writeHeader ``` The duplicate entries make `FetchHeaders` combine them via `makeString()`, producing a `StringImpl` held only by the header map — the minimal condition for the free to actually happen. StringImpl is allocated via bmalloc which ASAN doesn't instrument by default; with `Malloc=1` (bmalloc → system heap) the debug build reports: ``` AddressSanitizer: heap-use-after-free READ of size 13 oven-sh#2 uWS::HttpResponse<false>::writeHeader oven-sh#5 doRenderHeadResponse RequestContext.zig:1378 freed by: oven-sh#23 HTTPHeaderMap::remove oven-sh#28 doWriteHeaders RequestContext.zig:2303 oven-sh#29 renderMetadata RequestContext.zig:2209 oven-sh#30 doRenderHeadResponse RequestContext.zig:1377 ``` ## Cause `doRenderHeadResponse()` calls `headers.fastGet(.TransferEncoding)`, which returns a `ZigString` that **borrows** the header map entry's `StringImpl` bytes (no ref taken). For an ASCII value, `toSlice()` also borrows rather than copying. It then calls `this.renderMetadata()`, whose `doWriteHeaders()` does `headers.fastRemove(.TransferEncoding)` (and `renderMetadata` also `swapInitHeaders()` + `deref()`s the whole `FetchHeaders`). When the map held the only reference to the `StringImpl`, it's destroyed right there — and the very next line `resp.writeHeader("transfer-encoding", transfer_encoding_str.slice())` writes the freed bytes to the socket. The adjacent `Content-Length` branch has the same bug: `std.fmt.parseInt()` runs on the borrowed slice *after* `renderMetadata()` has already `fastRemove(.ContentLength)`'d it. ## Fix - **Transfer-Encoding**: use `toSliceClone()` instead of `toSlice()` so the value is owned and survives `renderMetadata()`. - **Content-Length**: parse the integer *before* `renderMetadata()` (and drop the slice immediately), so the borrowed bytes are never touched after the header entry is removed. No extra allocation needed since only the parsed `usize` is used afterwards. ## Verification New test in `test/js/bun/http/bun-server.test.ts` (inside the existing `HEAD requests oven-sh#15355` block) spawns a subprocess with `Malloc=1` (non-Windows), serves HEAD responses whose Transfer-Encoding / Content-Length values are `makeString()`-combined (sole-owner StringImpl), and asserts the raw wire output. ``` git stash push -- src/ → test fails with "AddressSanitizer: heap-use-after-free" in stderr git stash pop → test passes ``` All other tests in the `HEAD requests oven-sh#15355` describe block continue to pass. Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Repro
The duplicate entries make
FetchHeaderscombine them viamakeString(), producing aStringImplheld only by the header map — the minimal condition for the free to actually happen.StringImpl is allocated via bmalloc which ASAN doesn't instrument by default; with
Malloc=1(bmalloc → system heap) the debug build reports:Cause
doRenderHeadResponse()callsheaders.fastGet(.TransferEncoding), which returns aZigStringthat borrows the header map entry'sStringImplbytes (no ref taken). For an ASCII value,toSlice()also borrows rather than copying. It then callsthis.renderMetadata(), whosedoWriteHeaders()doesheaders.fastRemove(.TransferEncoding)(andrenderMetadataalsoswapInitHeaders()+deref()s the wholeFetchHeaders). When the map held the only reference to theStringImpl, it's destroyed right there — and the very next lineresp.writeHeader("transfer-encoding", transfer_encoding_str.slice())writes the freed bytes to the socket.The adjacent
Content-Lengthbranch has the same bug:std.fmt.parseInt()runs on the borrowed slice afterrenderMetadata()has alreadyfastRemove(.ContentLength)'d it.Fix
toSliceClone()instead oftoSlice()so the value is owned and survivesrenderMetadata().renderMetadata()(and drop the slice immediately), so the borrowed bytes are never touched after the header entry is removed. No extra allocation needed since only the parsedusizeis used afterwards.Verification
New test in
test/js/bun/http/bun-server.test.ts(inside the existingHEAD requests #15355block) spawns a subprocess withMalloc=1(non-Windows), serves HEAD responses whose Transfer-Encoding / Content-Length values aremakeString()-combined (sole-owner StringImpl), and asserts the raw wire output.All other tests in the
HEAD requests #15355describe block continue to pass.