webcore/Body: don't alias heap-allocated mime_type into shared Store#30260
webcore/Body: don't alias heap-allocated mime_type into shared Store#30260robobun wants to merge 2 commits into
Conversation
MimeType.init() heap-allocates its value string when the content type isn't one of the recognized static entries (e.g. "application/javascript"). Response#blob() / Request#blob() stored that heap pointer in both blob.content_type (owned, freed by Blob.deinit) and store.mime_type (borrowed, never freed by Store.deinit). When another Blob shares the store via slice() and the original is GC'd, reading slice.type follows the dangling store.mime_type.value. Only copy the mime into the store when it points at static memory.
|
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)
WalkthroughFixes blob MIME-type propagation: store MIME assignment now happens only when ChangesBlob MIME Type Assignment Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29947, which takes a more complete approach (gives Store its own copy so |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (blob.store != null and !allocated) { | ||
| blob.store.?.mime_type = mimeType; | ||
| } |
There was a problem hiding this comment.
🟡 This makes slice().type inconsistent: it now inherits the parent's type only when the Content-Type happens to match one of the ~7 hardcoded static entries in MimeType.init (e.g. text/plain), and returns "" for everything else (e.g. application/javascript). Per the W3C File API spec slice() without a contentType argument should return type === "", so the empty case is actually correct — but consider dropping the store.mime_type assignment entirely (and at line 767) so behavior is consistent rather than depending on an internal MIME-type list. Not a blocker for the UAF fix.
Extended reasoning...
What changed
The fix changes if (blob.store != null) to if (blob.store != null and !allocated), so store.mime_type is now only populated when MimeType.init() returns a static (non-heap-allocated) entry. This is the right call for safety — the heap-allocated string is owned by blob.content_type and freed by Blob.deinit, so aliasing it into the shared Store was the UAF being fixed. But it leaves store.mime_type in an inconsistent state that leaks an internal implementation detail to user code.
How it manifests
MimeType.init (src/http_types/MimeType.zig:260-360) returns a static constant with allocated=false only for a small hardcoded set: text/plain, text/html, text/css, text/javascript, application/json, application/wasm, application/octet-stream. Anything else — application/javascript, image/png, text/csv, custom types — falls through to the heap-allocating path with allocated=true.
When you call Blob#slice() with no contentType argument, getSliceFrom (Blob.zig:3009) only copies the parent's content_type into the slice when !this.content_type_allocated, so for allocated types the slice's own content_type is "". Blob.getType (Blob.zig:3137-3148) then falls through to store.mime_type.value. After this PR that fallback is populated for static types but stays .none (value "") for allocated ones.
Step-by-step proof
Static type:
const r = new Response('hi', { headers: { 'content-type': 'text/plain' } });
const b = await r.blob();
b.slice().type; // 'text/plain;charset=utf-8'MimeType.init('text/plain') → returns static MimeType.text, allocated=false → !allocated is true → store.mime_type = MimeType.text → getType returns it.
Allocated type:
const r = new Response('hi', { headers: { 'content-type': 'application/javascript' } });
const b = await r.blob();
b.slice().type; // ''MimeType.init('application/javascript') → heap-allocates, allocated=true → !allocated is false → store.mime_type stays .none (value "") → getType returns "".
So whether a slice inherits its parent's type depends purely on whether the Content-Type string is in an internal hardcoded list — which is surprising and not something users can predict.
Why this isn't a blocker
- Per the W3C File API spec,
slice()with nocontentTypeargument should return a Blob withtype === "". So the allocated case is now spec-correct, and the static case is the (pre-existing) deviation. - The "consistent" pre-PR behavior only existed because of the UAF being fixed — it wasn't a designed guarantee, it was UB that happened to look right while the parent blob was alive.
getSliceFromalready only inheritscontent_typewhen!content_type_allocated, so this PR makesstore.mime_typeconsistent with the existing slice-level split rather than inventing a new one.- The new test in this PR already accepts both
""and"application/javascript"— only""will ever be produced after this fix, which the assertion handles fine.
Suggested follow-up
For consistency, either:
- (a) Drop the
store.mime_typeassignment entirely at both Body.zig:767 and Body.zig:1443. This makesslice().typeuniformly""(spec-compliant) regardless of Content-Type. - (b) Deep-copy the allocated string into the Store with proper ownership and free it in
Store.deinit.
Option (a) is simpler and more correct per spec. Either way, this is a minor follow-up — the UAF fix itself is good.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stderr).toBe(""); | ||
| expect(["\"\"", "\"application/javascript\""]).toContain(stdout); |
There was a problem hiding this comment.
🟡 The assertion expect(["\"\"", "\"application/javascript\""]).toContain(stdout) accepts the buggy UAF output as valid. After this fix, slice.type is deterministically "" (since store.mime_type stays .none); the only way to get "application/javascript" here is the freed content_type allocation being read back before it's overwritten — exactly the regression this test guards against. Tighten to expect(stdout).toBe('""') so non-ASAN builds also fail if the UAF returns.
Extended reasoning...
What the issue is
The new regression test asserts:
expect(["\"\"", "\"application/javascript\""]).toContain(stdout);This accepts two outputs as passing. But after the fix in this PR, only one of them — "" — is actually reachable. The other accepted value, "application/javascript", is precisely the symptom of the use-after-free this test is meant to catch on builds that don't poison freed memory.
Why "" is the only correct post-fix output
Walking the code path for new Response("hi", { headers: { "content-type": "application/javascript" } }) → await response.blob() → blob.slice() → slice.type:
MimeType.init("application/javascript", alloc, &allocated)hits the"application".lenbranch insrc/http_types/MimeType.zig:276-295. It checks forjson,geo+json,octet-stream, andwasm— none matchjavascript— so it falls through and heap-allocates the lowercased copy, settingallocated = true.- After this PR's fix,
if (blob.store != null and !allocated)is false (becauseallocated == true), sostore.mime_typeis not assigned. It stays at theStorestruct defaultmime_type: MimeType = .none(Store.zig:5), whose.valueis""(MimeType.zig:239). blob.slice()with nocontentTypeargument goes throughgetSliceFrom(Blob.zig:3009), which only inherits the parent'scontent_typewhen!this.content_type_allocated. Here it is allocated, so the slice'scontent_typeis left empty.slice.typecallsBlob.getType(Blob.zig:3137-3146). Withcontent_type.len == 0, it falls through tostore.mime_type.value, which is"".
So the spawned subprocess must write JSON.stringify("") → '""'. There is no code path in the fixed build that produces "application/javascript" for slice.type.
Why accepting "application/javascript" is harmful
The only way slice.type becomes "application/javascript" here is the pre-fix bug: store.mime_type.value aliases the heap-allocated blob.content_type, the original blob is GC'd and frees it, and getType reads the freed-but-not-yet-overwritten 22 bytes back out. On ASAN builds this aborts (caught by exitCode/stderr), but on release/non-ASAN builds the freed slot frequently still contains the original bytes, so the subprocess prints "application/javascript" and exits 0 — and the test passes even though the UAF has regressed.
By including "\"application/javascript\"" in the accepted set, the assertion explicitly green-lights the buggy output. The test only functions as a regression guard on ASAN CI; on every other build configuration it would silently pass with the bug present.
Step-by-step proof
| Step | Fixed build | Regressed build (no ASAN) |
|---|---|---|
MimeType.init |
allocated=true, value heap-alloc'd |
same |
store.mime_type |
left at .none (value "") |
aliased to heap pointer |
| original blob GC'd | frees content_type |
frees content_type (store still points at it) |
slice.type reads |
store.mime_type.value = "" |
freed bytes ≈ "application/javascript" |
| stdout | '""' |
'"application/javascript"' |
| Current assertion | ✅ passes | ✅ passes (bug undetected) |
toBe('""') |
✅ passes | ❌ fails (bug caught) |
Fix
expect(stdout).toBe('""');This is a test-quality nit rather than a runtime bug — ASAN CI would still catch a regression via the crash — but tightening it costs nothing and makes the test meaningful on release builds too.
1 participant
What does this PR do?
Fixes a use-after-free in
Response#blob()/Request#blob()found by fuzzing.MimeType.init()heap-allocates its.valuestring when the Content-Type isn't one of the recognized static entries (e.g."application/javascript"). The body.blob()path stored that heap pointer in bothblob.content_type(owned — freed byBlob.deinit) andstore.mime_type(borrowed — never freed byStore.deinit).When another Blob shares the same store (via
.slice()) and the original Blob is GC'd, readingslice.typefalls through tostore.mime_type.valueand reads freed memory.ASAN stack:
How did you verify your code works?
New regression test in
test/js/web/fetch/blob.test.tsreads garbage / ASAN-aborts without the fix, returns a valid string with it.