Fix use-after-free in Bun.Transpiler.transform() error path#30027
Fix use-after-free in Bun.Transpiler.transform() error path#30027robobun wants to merge 1 commit into
Conversation
TransformTask.run() runs the parser on a worker thread with a MimallocArena, then destroys the arena before returning. Parser/lexer error messages had their text allocated from that arena, so when the task's then() ran on the JS thread and tried to clone the messages into a BuildMessage, it read from freed memory. Clone the log messages with the default allocator before the arena is destroyed.
|
Updated 12:55 AM PT - May 1st, 2026
✅ @robobun, your commit 749c06dfe13b84032f7c33e3e55761977d647ed6 passed in 🧪 To try this PR locally: bunx bun-pr 30027That installs a local version of the PR into your bun-30027 --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 (2)
WalkthroughThis PR fixes a memory management issue in JSTranspiler where log messages were being accessed after their arena was deallocated. A defer handler now clones log entries into a persistent allocator, and a regression test verifies the fix under allocator/GC stress conditions. 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: 1/5 review remaining, refill in 41 minutes and 3 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29958 which already has the more complete fix (tracks and frees the cloned buffers in |
| // Log messages produced during parsing/printing borrow text allocated | ||
| // from `arena`. Clone them into `bun.default_allocator` before the | ||
| // arena is destroyed so `then()` can safely read them on the JS thread. | ||
| defer { | ||
| for (this.log.msgs.items) |*msg| { | ||
| msg.* = bun.handleOom(msg.clone(bun.default_allocator)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The cloned log messages are leaked: Msg.clone() dupes .text, location.file, location.line_text, and notes into bun.default_allocator, but TransformTask.deinit() only calls this.log.deinit() which is just msgs.clearAndFree() and never frees per-message allocations (and log.toJS() → BuildMessage.create() clones again rather than taking ownership). Before this PR these strings lived in the arena and were reclaimed by arena.deinit(); the simple fix is to iterate this.log.msgs.items and call msg.deinit(bun.default_allocator) in TransformTask.deinit() before this.log.deinit().
Extended reasoning...
What the bug is
The new defer block in TransformTask.run() deep-clones every accumulated log message into bun.default_allocator:
defer {
for (this.log.msgs.items) |*msg| {
msg.* = bun.handleOom(msg.clone(bun.default_allocator));
}
}Msg.clone() (src/logger.zig:437) calls Data.clone() (line 230) which does allocator.dupe(u8, this.text) and Location.clone() (line 113) which dupes .file and .line_text; it also calls bun.clone(this.notes, allocator) to dupe the notes slice. All of these now live in bun.default_allocator.
However, nothing ever frees them.
Why existing code doesn't free it
There are two places that touch these messages afterwards, and neither takes ownership:
then()→this.log.toJS()→BuildMessage.create()(src/bun.js/BuildMessage.zig:54) doesmsg.clone(allocator)again — it makes its own third copy and stores that in theBuildMessage. It does not take ownership of the incomingMsg.TransformTask.deinit()→this.log.deinit()(src/logger.zig:840) is justlog.msgs.clearAndFree(). That frees theArrayListbacking storage only; it never iterates the items to callMsg.deinit(), so the duped.text/.file/.line_text/.notesslices are dropped on the floor.
Before this PR these strings lived in the per-task MimallocArena and were reclaimed wholesale by arena.deinit(). By moving them to bun.default_allocator without adding a corresponding free, the fix trades a use-after-free for a leak.
Step-by-step proof
Take the regression test added in this PR: transpiler.transform("const x = 1 ++ 2 ++ 3;").
TransformTask.run()runs on the worker thread. The parser fails and pushes aMsgintothis.logwhosedata.text(Expected ";" but found "2") anddata.locationwere allocated fromarena.- The new
deferruns:msg.clone(bun.default_allocator)allocates fresh heap copies oftext,location.file("input.tsx"),location.line_text, and thenotesslice, and overwritesmsg.*with them. arena.deinit()frees the originals (fine — we no longer reference them).- On the JS thread,
then()seesthis.log.hasAny()and callsthis.log.toJS(). Internally that callsBuildMessage.create(globalThis, bun.default_allocator, msg), which at line 54 doesmsg.clone(allocator)— a third copy. TheBuildMessageowns copy Copy source lines when generating error messages #3; copy Fix calling #private() functions in classes #2 is still sitting inthis.log.msgs.items[0]. then()'sdefer this.deinit()runs →this.log.deinit()→msgs.clearAndFree(). TheArrayListbuffer is freed, but the heap allocations inside copy Fix calling #private() functions in classes #2 (text,file,line_text,notes) are never passed tobun.default_allocator.free().
The test loops 10 times, so it leaks 10 sets of these strings. Any long-running process that repeatedly calls transform() on invalid input will leak unboundedly.
Impact
Error-path-only and small per occurrence (an error string + file path + line text + notes slice), so this is not severe — and trading a flaky UAF crash for a small leak is clearly the right direction. But it is a regression introduced by this exact change and is trivially fixable, so worth doing in the same PR.
Suggested fix
In TransformTask.deinit(), free the per-message allocations before dropping the list:
pub fn deinit(this: *TransformTask) void {
for (this.log.msgs.items) |*msg| {
msg.deinit(bun.default_allocator);
}
this.log.deinit();
...
}(Note: Location.deinit is currently a no-op with a // don't really know what's safe to deinit here! comment, so location.file/line_text would still leak even with this; if you want to be fully clean you'd need to free those explicitly too. But Msg.deinit at least reclaims data.text and the notes slice, which is the bulk of it and matches what the rest of the codebase does.)
What does this PR do?
Fixes a use-after-free in the async
Bun.Transpiler().transform()error path found by Fuzzilli (fingerprintAddress:use-after-poison:bun-debug+0x827621e).TransformTask.run()executes the parser on a worker thread using aMimallocArenathat is destroyed at the end ofrun(). When parsing fails, the lexer/parser format their error messages withallocPrintusing that arena allocator, and the resultingMsg.data.textslices are appended (shallowly, viaappendToMaybeRecycled) intothis.log.Later,
then()runs on the JS thread and callsthis.log.toJS(), which clones each message. At that point the arena is already gone, soData.clone'sallocator.dupe(u8, this.text)reads freed memory. Whether this crashes depends on whether the arena's pages have been handed back out and re-poisoned, which is why it was flaky.The fix: before tearing down the arena, deep-clone any accumulated log messages into
bun.default_allocatorso they outlive the worker thread.How did you verify your code works?
Repro (crashed ~8/30 runs under ASAN before, 0/50 after):
Added a regression test in
test/bundler/transpiler/transpiler.test.jsthat asserts the rejection message is intact after allocation churn. It crashes the unfixed debug build at the samelogger.Data.clone → Allocator.dupeframe the fuzzer hit and passes with the fix.