blob: stop aliasing store.mime_type.value with blob.content_type in response.blob()#29947
blob: stop aliasing store.mime_type.value with blob.content_type in response.blob()#29947robobun wants to merge 5 commits into
Conversation
|
Updated 7:06 PM PT - May 4th, 2026
❌ @robobun, your commit 6713c82 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29947That installs a local version of the PR into your bun-29947 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
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)
WalkthroughTracks and fixes ownership of heap-allocated Blob MIME strings so stores do not alias blob-owned MIME data. Adds Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 18 minutes and 55 seconds. 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 `@src/bun.js/webcore/Body.zig`:
- Around line 751-763: The store's ownership flag isn't reset when falling back
to the static MimeType.text value, so Store.deinit may free a static pointer;
update the fallback branch that assigns store.mime_type = MimeType.text (and the
analogous assignment at the other location) to also set
store.mime_type_allocated = false (and ensure any previous allocated value is
freed beforehand, mirroring the header-present branch behavior); locate the
logic around blob.store / store.mime_type / store.mime_type_allocated and the
fallback assignment to MimeType.text and make the flag change there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c8b9cbc-b244-4933-848e-133036388679
📒 Files selected for processing (3)
src/bun.js/webcore/Body.zigsrc/bun.js/webcore/blob/Store.zigtest/js/web/fetch/blob.test.ts
|
On the suggested issues:
Leaving both out of the PR description. |
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 `@src/bun.js/webcore/Body.zig`:
- Around line 751-763: Extract the duplicated MIME propagation logic into a
single helper (e.g., a function like propagateMimeTypeToStore or
setStoreMimeType) that accepts the Store reference, the mimeType, and the
allocated flag and performs the current behavior (allocating via
bun.default_allocator.dupe, calling bun.handleOom on failure, and preserving
mimeType.category), then replace the repeated blocks in Blob (the
store.setMimeType calls around Blob.deinit ownership handling) and the other
occurrences (lines around 766-771, 1437-1449, 1452-1457) to call this helper;
ensure the helper mirrors the exact allocation/oom semantics and keeps ownership
semantics identical to the original store.setMimeType invocations so behavior
and refcounting remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7ec0e19-7c8b-408f-8762-cdcfeb3e06b4
📒 Files selected for processing (2)
src/bun.js/webcore/Body.zigsrc/bun.js/webcore/blob/Store.zig
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Build 49201: Remaining failures are pre-existing flakes on main (6c21a7e), none touch the changed code:
|
There was a problem hiding this comment.
Thanks — 653364d addresses the test-coverage note (the fetch case now streams so resolve()'s .getBlob arm is actually exercised), and the .text fallback going through setMimeType() covers the earlier ownership-flag concern. I didn't find anything else, but since this adds a new ownership invariant to the shared/refcounted Blob.Store and changes Store.deinit, it's worth a maintainer's pass.
Extended reasoning...
Overview
This PR fixes a use-after-free in response.blob() where blob.content_type and blob.store.mime_type.value aliased the same heap allocation for non-static MIME types (image/, audio/, etc.). When the original Blob was GC'd while a slice still held the Store, Blob.deinit freed the string and the Store's pointer dangled. The fix:
src/bun.js/webcore/blob/Store.zig: addsmime_type_allocated: boolfield, asetMimeType()helper that frees any prior heap value before assigning, and frees indeinit().src/bun.js/webcore/Body.zig: at bothresponse.blob()materialization sites (Body.Value.resolve's.getBlobarm andgetBlobWithThisValue), the Store now gets its owndupeof the mime string when heap-allocated, viasetMimeType(). TheMimeType.textfallback also routes throughsetMimeType()so a pre-allocated store can't end up with the flag set on a static string.test/js/web/fetch/blob.test.ts: subprocess regression test covering both code paths under ASAN.
Security risks
None. This is a memory-safety hardening (UAF fix); no auth, crypto, or untrusted-input parsing changes. The only new behavior is an extra small allocation per response.blob() call when the Content-Type is a non-static category.
Level of scrutiny
Moderate-to-high. The change is small and well-scoped, mirrors the pattern from #29910, and has a verified-failing-on-main ASAN regression test. However, it introduces a new ownership invariant on Blob.Store — a refcounted, shared structure on the hot path of every fetch().blob() — and adds a free in Store.deinit(). I grepped for other direct store.mime_type = writes: the only one outside setMimeType() is Blob.zig:570 on a freshly-created store with a static value, which is safe. Still, ownership changes to shared refcounted state in unmanaged Zig are exactly where subtle lifetime bugs hide, so a maintainer who knows the Blob/Store model should confirm the invariant is sound across all paths (including any future Store reuse/pooling).
Other factors
- Both of my prior inline comments have been addressed: c4c1af4 routed the
.textfallback throughsetMimeType()(closing the static-free risk), and 653364d switched the fetch test to a delayedReadableStreamso theresolve()arm is actually covered. - CodeRabbit's concerns are resolved.
- CI failures (
fetch-http2-client,bun-create,bake/dev/stress) are pre-existing flakes on main per robobun's analysis;blob.test.tsitself passes on all lanes. - No bugs found by the bug-hunting system on the current revision.
|
Build 49219: Remaining failures are pre-existing flakes also seen on other unrelated branches off the same main commit (6c21a7e):
The S3 init paths use static extension-derived mime strings, so All other test noise is in the "flaky (passed on retry)" warning bucket — |
|
Independently arrived at the same fix on |
…esponse.blob() Body.Value.resolve and Body.Mixin.getBlobWithThisValue both assigned the heap-allocated MimeType.value to both blob.content_type (owned, freed in Blob.deinit) and store.mime_type.value (not owned). The Store is refcounted and shared with slices, so it can outlive the Blob. After the original is collected, reading sliced.type falls through to store.mime_type.value -> use-after-free. Give the Store its own copy when the mime string is heap-allocated, tracked by Store.mime_type_allocated and freed in Store.deinit.
Ensures mime_type_allocated is reset when overwriting with a static value.
symbolize=0 breaks LSan's symbol-name suppression matching, so CI's normally-suppressed leaks (uws_create_app, etc.) surfaced on the fetch variant. We only care about the UAF here.
A fully-buffered 11-byte body lands before fetch() resolves, so both test.each cases were taking getBlobWithThisValue. Serve a delayed ReadableStream (same pattern as the #10072 test) so the fetch case hits Body.Value.resolve's .getBlob arm. Emit the server at module scope rather than inside a block-scoped await using — the extra async-dispose resume point left a stack slot that conservatively rooted the original blob across Bun.gc(), so the finalizer never ran and the pre-fix UAF didn't reproduce.
Jarred-Sumner
force-pushed
the
farm/e47d6495/blob-store-mime-uaf
branch
from
653364d to
f8e1193
Compare
There was a problem hiding this comment.
I didn't find any further issues — both of my earlier inline comments were addressed and CI is green. Deferring to a human reviewer since this adds a new ownership invariant (mime_type_allocated) to the shared refcounted Store struct, and memory-lifetime changes in this hot path are worth a second pair of eyes.
Extended reasoning...
Overview
This PR fixes a use-after-free in response.blob() where the Blob's heap-allocated content_type and the shared Store.mime_type.value aliased the same allocation. It adds Store.mime_type_allocated: bool, a Store.setMimeType() helper that frees any prior owned value, frees the owned value in Store.deinit(), and updates four call sites in Body.zig (two header-present blocks + two MimeType.text fallbacks in Body.Value.resolve and getBlobWithThisValue) to dupe the mime string into the Store rather than alias the Blob's. A parameterized subprocess regression test in test/js/web/fetch/blob.test.ts covers both code paths under ASAN.
Security risks
None. This is internal memory-ownership bookkeeping for a small mime-type string; no auth, crypto, permissions, or untrusted-input parsing is touched.
Level of scrutiny
Moderate-to-high. The diff is small (~50 LOC of Zig + tests) and the fix is conservative (give the Store its own copy), but it introduces a new ownership invariant on a refcounted struct shared across many Blob instances. Getting this class of change wrong produces UAF/double-free in a hot path. The PR already went through two rounds of review feedback (the MimeType.text fallback flag-reset issue, and the test not actually exercising the resolve() arm), both now addressed via Store.setMimeType() and a streaming-body test variant. I verified the remaining direct store.mime_type = ... assignments outside this PR (Blob.zig:709, standalone_graph_jsc.zig:14) are on freshly-created stores with static registry mimes, so the new invariant holds there. The author also noted an alternative design (transfer ownership to Store, have Blob borrow) on a separate branch — that's a judgment call a human maintainer may want to weigh.
Other factors
- All prior inline comments (CodeRabbit's and mine) are resolved.
- CI is green on the changed test; remaining failures are documented pre-existing flakes (
s3-storage-class,fetch-http2-client, etc.) unrelated to Blob/Store. - The new test is well-constructed: subprocess isolation, ASAN-aware (
symbolize=0:detect_leaks=0), and the streaming variant was verified to actually reachBody.Value.resolve. - This is the second PR in a related series (#29910 fixed the same aliasing in
dupeWithContentType), so a maintainer familiar with that one is well-placed to review.
CI status for this PR`blob.test.ts` passes on every lane it has run on across builds 49201, 49219, 51028, and 51418. The rebased code (post src/-restructure) verified locally with `zig:check`. All bot reviews approve; 0 unresolved threads. All CI "failures" since 889194d are infrastructure or pre-existing flakes, none in code touched by this PR:
This PR needs a Buildkite retry once the agent fleet (darwin-x64/freebsd-aarch64/win-2019) recovers, or merge based on the passing lanes. |
There was a problem hiding this comment.
Both of my earlier concerns (the MimeType.text fallback and the resolve() test coverage) have been addressed and the change looks correct, but since this introduces a new ownership invariant on Store.mime_type in core Blob/Body memory management, I'd like a human to sign off.
Extended reasoning...
Overview
Fixes a use-after-free where response.blob() aliased the same heap-allocated mime string between blob.content_type (owned, freed in Blob.deinit) and the refcounted Store.mime_type.value. Adds Store.mime_type_allocated: bool, a Store.setMimeType() helper that frees any prior allocation, and a free in Store.deinit. Two call sites in Body.zig (resolve's .getBlob arm and getBlobWithThisValue) now dupe the mime string into the Store when it's heap-allocated and route the MimeType.text fallback through setMimeType so the flag is always reset. ASAN-verified regression tests cover both code paths via subprocess.
Security risks
None introduced. The change strictly tightens memory ownership — it eliminates a UAF rather than adding one. No new untrusted-input parsing, no auth/crypto/permission surface.
Level of scrutiny
High. This is manual memory management in a hot, refcounted core-runtime path (Blob/Store). The PR introduces a new invariant (mime_type_allocated ⇒ Store.deinit frees mime_type.value) that all future writers to store.mime_type must respect. I verified the only remaining direct assignment outside setMimeType (Blob.fromURLSearchParams at Blob.zig:709) is on a freshly-created Store with a static registry value, so it's safe — but a maintainer who owns this area should confirm the invariant is acceptable going forward.
Other factors
- Both of my prior inline comments (fallback flag reset; test not actually exercising
resolve()) were addressed in c4c1af4 and 653364d respectively, and verified by the author against an unfixed build under ASAN. - CodeRabbit's concerns are resolved; the nitpick about further deduplication was reasonably declined to keep the change scoped.
- The bug-hunting system found no issues on the current revision.
- CI on the relevant test file is green across lanes; remaining failures are documented pre-existing flakes / agent-capacity outage unrelated to this change.
- An alternative branch exists that transfers ownership to the Store instead of duping; either approach is sound and the author chose the more conservative dup-per-owner model.
|
Closing as obsolete after the Zig→Rust port of webcore on main. While rebasing onto current main I found that the implementation this PR patches has moved:
Thanks to the reviewers — the analysis here (aliasing between |
1 participant
What
Body.Value.resolve(the.getBlobarm) andBody.Mixin.getBlobWithThisValueset up the Blob returned fromresponse.blob(). When the response'sContent-Typeis a category thatMimeType.initheap-allocates (image/, audio/, video/, font/, most application/*, …), both places did:The Store is refcounted and shared —
blob.slice()keeps it alive while the slice's owncontent_typeis"". When the original Blob is collected first,Blob.deinitfreescontent_type; the Store survives withmime_type.valuedangling.sliced.typethen reads it viaBlob.getType's store fallback → use-after-free.Related to #29910, which fixed the same aliasing inside
dupeWithContentType; this is an independent occurrence at the pointresponse.blob()materializes the mime type.Repro
ASAN trace on main:
Fix
Give the Store its own copy of the mime string when it's heap-allocated, and free it in
Store.deinit:Blob.Store: addmime_type_allocated: bool = false;deinitfreesmime_type.valuewhen set.Body.Value.resolve/getBlobWithThisValue: whenMimeType.initallocated, dupe the string for the Store and setmime_type_allocated = true. Static mime strings continue to be stored by reference with the flagfalse(no behaviour change, no free).Blob and Store now each own their allocation with independent lifetimes;
sliced.typereads the Store's copy, which lives until the last ref drops.Verified
test/js/web/fetch/blob.test.tscover both code paths (fetch →resolve, and syncnew Response(...).blob()→getBlobWithThisValue).AddressSanitizer: use-after-poisoninBlob.getType.bun bd test test/js/web/fetch/blob.test.ts— 18/18 pass.