Fix use-after-free in Bun.Transpiler async transform() errors#30263
Fix use-after-free in Bun.Transpiler async transform() errors#30263robobun wants to merge 4 commits into
Bun.Transpiler async transform() errors#30263Conversation
TransformTask.run() uses a per-task MimallocArena for parsing. When the parser emits diagnostics, the message text and notes slices are allocated from that arena. The arena is destroyed at the end of run(), but the log messages are read later on the main thread in then() via log.toJS(), leading to a use-after-poison / use-after-free. Deep-clone any accumulated log messages into bun.default_allocator before the arena is torn down so then() can safely surface them.
|
Updated 1:05 AM PT - May 5th, 2026
❌ @robobun, your commit e2618c9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30263That installs a local version of the PR into your bun-30263 --bun |
|
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)
WalkthroughTransformTask now transfers log-message ownership to ChangesTranspiler allocator/cleanup + tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/api/JSTranspiler.zig`:
- Around line 506-508: The defer loop currently swallows OOMs by falling back to
the arena-backed msg on clone failure; change the error handler to crash on OOM
using bun.handleOom() instead of returning msg.*. Replace the current catch
(e.g., "catch msg.*") on the msg.clone(bun.default_allocator) call with a catch
that forwards the error to bun.handleOom(err) (e.g., "catch |err|
bun.handleOom(err)"), and then assign the successful clone to msg.*; this
ensures OOMs abort rather than reintroducing an arena-backed message that then()
and log.toJS() might read.
🪄 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: de54cc6a-ae50-444e-bac6-d82393d8cb55
📒 Files selected for processing (2)
src/runtime/api/JSTranspiler.zigtest/js/bun/transpiler/transpiler-async-error-uaf.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| defer for (this.log.msgs.items) |*msg| { | ||
| msg.* = msg.clone(bun.default_allocator) catch msg.*; | ||
| }; |
There was a problem hiding this comment.
Done — added msg.deinit(bun.default_allocator) for each message in TransformTask.deinit() before log.deinit().
| this.transpiler.setLog(&this.log); | ||
| this.log.msgs.allocator = bun.default_allocator; | ||
| defer for (this.log.msgs.items) |*msg| { | ||
| msg.* = msg.clone(bun.default_allocator) catch msg.*; |
| defer for (this.log.msgs.items) |*msg| { | ||
| msg.* = bun.handleOom(msg.clone(bun.default_allocator)); | ||
| }; |
There was a problem hiding this comment.
🟡 Minor: the msg.deinit(bun.default_allocator) cleanup added in TransformTask.deinit() won't actually free everything msg.clone() allocated — Location.deinit is a no-op (logger.zig:149), so the duped location.file and location.line_text strings (for the message and each note) leak on the global heap per failing async transform(). Tiny error-path-only leak and the root asymmetry is pre-existing in Location, so not a blocker for this UAF fix — but if you want full cleanup here you'd need to free those two fields manually (or fix Location.deinit).
Extended reasoning...
What the bug is
The new defer in TransformTask.run() deep-clones each log message into bun.default_allocator:
defer for (this.log.msgs.items) |*msg| {
msg.* = bun.handleOom(msg.clone(bun.default_allocator));
};Msg.clone → Data.clone → Location.clone (src/logger/logger.zig:113-123), which does allocator.dupe(u8, this.file) and allocator.dupe(u8, this.line_text.?). And Msg.clone also calls bun.clone(this.notes, allocator), which (per src/bun.zig:551-557) detects Data.clone and deep-clones every note, so each note's location.file / location.line_text is also duped onto bun.default_allocator.
The matching cleanup added in TransformTask.deinit():
for (this.log.msgs.items) |*msg| msg.deinit(bun.default_allocator);calls Msg.deinit → Data.deinit → Location.deinit. But Location.deinit is intentionally stubbed:
// don't really know what's safe to deinit here!
pub fn deinit(_: *Location, _: std.mem.Allocator) void {}So Data.deinit frees .text and the notes slice is freed, but every Location.file and Location.line_text that was just duped onto the global heap is never freed.
Code path that triggers it
transpiler.transform("const {a, a} = b")schedules aTransformTask.run()parses on a worker thread; the parser pushes aMsgwithdata.location = {file: "input.tsx", line_text: "const {a, a} = b"}and one note with its own location, all allocated from the per-taskMimallocArena.- On scope exit, the new defer runs first (LIFO) and
msg.clone(bun.default_allocator)heap-dupes:data.text,data.location.file,data.location.line_text, the notes slice,notes[0].text,notes[0].location.file,notes[0].location.line_text. arena.deinit()reclaims the originals (correct — that's the UAF fix).then()runs on the main thread, builds the JS error, then callsthis.deinit().deinit()runsmsg.deinit(bun.default_allocator)for each message → freesdata.text,notes[0].text, and the notes slice.Location.deinitis a no-op, so the four duped location strings (2 ×file+ 2 ×line_text) stay on the global mimalloc heap forever.
Why existing code doesn't prevent it
this.log.deinit()only frees themsgslist backing storage, not per-message payloads.Msg.deinitis the maximally-correct API to call here and the author called it; the gap is one layer down inLocation.deinit, which has carried the comment "don't really know what's safe to deinit here!" since long before this PR.- Before this PR, these strings lived in the per-task arena and were swept by
arena.deinit()(with a UAF, which this PR correctly fixes). Moving them tobun.default_allocatoris what makes the no-opLocation.deinitmatter.
Impact
Per failing async transform(): (1 + #notes) × (len(file) + len(line_text)) bytes leaked from the global heap. For the PR's own test ("const {a, a} = b", 1 note), that's ≈ 2 × ("input.tsx" + "const {a, a} = b") ≈ 50 bytes per call. Error-path only; the success path has no log messages and leaks nothing.
Worth noting: the same Location.clone/Location.deinit asymmetry already bites BuildMessage.create → BuildMessage.finalize on this exact path (BuildMessage.zig:54/186), so this PR roughly doubles a pre-existing per-error leak rather than introducing a brand-new leak class.
Step-by-step proof
For one call to transpiler.transform("const {a, a} = b"):
Allocation in msg.clone(bun.default_allocator) |
Freed by msg.deinit(bun.default_allocator)? |
|---|---|
data.text ('"a" has already been declared') |
✅ Data.deinit line 214 |
data.location.file ("input.tsx") |
❌ Location.deinit no-op line 149 |
data.location.line_text ("const {a, a} = b") |
❌ Location.deinit no-op line 149 |
notes slice ([1]Data) |
✅ Msg.deinit line 484 |
notes[0].text |
✅ Data.deinit line 214 |
notes[0].location.file |
❌ Location.deinit no-op |
notes[0].location.line_text |
❌ Location.deinit no-op |
Four small allocations leak per failed transform.
How to fix
Either locally in TransformTask.deinit():
for (this.log.msgs.items) |*msg| {
if (msg.data.location) |*loc| {
bun.default_allocator.free(loc.file);
if (loc.line_text) |lt| bun.default_allocator.free(lt);
}
for (msg.notes) |*note| if (note.location) |*loc| {
bun.default_allocator.free(loc.file);
if (loc.line_text) |lt| bun.default_allocator.free(lt);
};
msg.deinit(bun.default_allocator);
}…or, better long-term, make Location.deinit actually free what Location.clone allocates (which would also fix the identical leak in BuildMessage.finalize). The latter is out of scope for this PR; flagging as a nit since the author explicitly added msg.deinit intending full cleanup.
2 participants
What does this PR do?
TransformTask.run()creates a per-taskMimallocArenaand points the transpiler's allocator at it for the duration of the parse. When the parser emits diagnostics (error text, notes arrays, location data), those allocations come from that arena. The arena is destroyed at the end ofrun()(worker thread), but the log messages are consumed later inthen()(main thread) vialog.toJS()→BuildMessage.create()→Msg.clone(), which reads the now-freed arena memory.This showed up as
AddressSanitizer: use-after-poisonwhenthen()ran after the destroyed arena's pages had been poisoned, and as garbage error text/notes otherwise.Fix
Before the arena is torn down, deep-clone any accumulated log messages into
bun.default_allocator. Deferred afterdefer arena.deinit()so it runs first (LIFO) while the arena is still valid, and covers every early-return path inrun().How did you verify your code works?
Minimal repro that crashes under ASAN before this change and passes after:
Added
test/js/bun/transpiler/transpiler-async-error-uaf.test.tswhich exercises multiple concurrent failing transforms and asserts that the errormessage,position, andnotesare intact.Found by Fuzzilli.