Fix use-after-free in Bun.Transpiler async transform() parse errors#30029
Fix use-after-free in Bun.Transpiler async transform() parse errors#30029robobun wants to merge 1 commit into
Conversation
TransformTask.run() allocates the parser/lexer arena and destroys it before returning to the thread pool. Error messages added to the log during parsing have their text allocated in that arena. When then() later runs on the JS thread and calls log.toJS(), it reads text that points into the destroyed arena. Deep-copy any log messages into the default allocator before the arena is torn down so then() can safely read them.
|
Updated 8:36 PM PT - Apr 30th, 2026
❌ @robobun, your commit 707064d has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30029That installs a local version of the PR into your bun-30029 --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)
WalkthroughImplements deferred log-rebinding in JSTranspiler.zig's TransformTask.run to create fresh logger.Log instances when per-task logs accumulate messages. Adds test validating async transform parse errors don't access freed arena memory through concurrent transpiler calls. 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: 3/5 reviews remaining, refill in 22 minutes and 21 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29958, which already incorporates review feedback (tracks the cloned string/notes buffers on the task so they're freed in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| defer { | ||
| if (this.log.msgs.items.len > 0) { | ||
| var new_log = logger.Log.init(bun.default_allocator); | ||
| new_log.level = this.log.level; | ||
| bun.handleOom(this.log.appendToWithRecycled(&new_log, true)); | ||
| this.log = new_log; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Note that appendToWithRecycled allocates a single StringBuilder buffer and a notes_buf in bun.default_allocator, and Log.deinit() only frees the msgs ArrayList — so the cloned message text/notes leak on every failing async transform(). This is the same pattern used by ~10 other call sites (RuntimeTranspilerStore, BundleThread, ParseTask, bundle_v2, etc.) and trading a UAF for a small error-path leak is clearly the right call here; just flagging it as a follow-up for the Log ownership model rather than something to block on.
Extended reasoning...
What happens
cloneToWithRecycled (src/logger.zig:802-829) makes two heap allocations using other.msgs.allocator (which is bun.default_allocator in this call site):
string_builder.allocate(other.msgs.allocator)at line 818 — one contiguous buffer holding all message text, location text, and note text.other.msgs.allocator.alloc(Data, notes_count)at line 819 — the notes array.
Neither pointer is stored anywhere on the Log. The individual Msg/Data text fields are slices into the shared StringBuilder buffer, so freeing them per-message would be wrong anyway.
Log.deinit() (logger.zig:840-844) only does log.msgs.clearAndFree(), which frees the ArrayList(Msg) backing storage but not the StringBuilder buffer or notes_buf. TransformTask.deinit() calls this.log.deinit(), so both allocations are leaked.
Step-by-step
t.transform("@@", "js")schedulesTransformTask.run()on a worker thread.- The parser hits
@@, callslog.addErrorFmt(...); the message text and location strings are allocated in theMimallocArena. run()returns; the newdeferblock fires (after thereturnbut beforearena.deinit()).appendToWithRecycled(&new_log, true)runscloneToWithRecycled: it counts ~a few dozen bytes of text, allocates oneStringBuilderbuffer + onenotes_bufinbun.default_allocator, and rewrites everyMsgto point into those buffers.this.log = new_log.arena.deinit()frees the original arena text — fine, nothing references it anymore (UAF fixed ✅).- On the JS thread,
then()reads the cloned text viathis.log.toJS(...)— works correctly. TransformTask.deinit()→this.log.deinit()→msgs.clearAndFree(). The StringBuilder buffer andnotes_bufare never freed.
Each failing call leaks on the order of tens to a couple hundred bytes (error text + path/line strings + notes array). The test's 200-iteration loop leaks ~200× that; a long-running process that repeatedly transforms invalid input would grow unbounded.
Why existing code doesn't prevent it
Msg.deinit / Data.deinit exist but Log.deinit never calls them — and even if it did, they'd try to free individual slices that all alias one shared buffer, which would be invalid. The StringBuilder buffer handle is simply dropped on the floor after cloneToWithRecycled returns; nothing in Log records it.
Why this is a nit, not a blocker
This is the established pattern in the codebase. The same appendToWithRecycled / appendToMaybeRecycled → Log.deinit lifecycle appears in RuntimeTranspilerStore.zig:326, BundleThread.zig:153/160, ParseTask.zig:358/369/380, bundle_v2.zig:4223/4371, HTMLBundle.zig:330, npm.zig:1859, Installer.zig:172, etc. — all of which have the identical leak. The PR correctly follows convention to move arena-backed messages into a stable allocator, and the trade (UAF/crash → small error-path-only leak) is unambiguously net-positive.
How to fix (follow-up)
The clean fix belongs in the Log API, not here: e.g. have cloneToWithRecycled stash the StringBuilder ptr/cap and notes_buf on the destination Log (or on each Msg) and free them in Log.deinit, or switch to per-message Msg.clone(allocator) so Log.deinit can iterate and free each one. That would fix this call site and the ~10 others simultaneously.
1 participant
What
Bun.Transpiler.prototype.transform()(the async variant) could read freed memory when reporting parse errors, producing garbage error messages in release builds and an ASAN crash in debug builds.Why
TransformTask.run()runs on a thread-pool thread and creates aMimallocArenafor parsing. The parser and lexer allocate their error message text (vialog.addErrorFmtetc.) using that arena as the allocator. The arena is torn down at the end ofrun().TransformTask.then()runs afterwards on the JS thread and callsthis.log.toJS(), which clones each message — readingmsg.data.text. At this point the text still points into the destroyed arena, so the read is a use-after-free.Setting
this.log.msgs.allocator = bun.default_allocatoronly covers themsgsArrayList backing storage, not the per-message text payload.Fix
Before the arena is destroyed, deep-copy any accumulated log messages into
bun.default_allocatorusing the existingappendToWithRecycled(..., true)path, which rebuilds each message (text, location, notes) via aStringBuilder.Testing
The new test runs 200 concurrent async
transform()calls that all fail to parse, withMIMALLOC_PURGE_DELAY=0so freed arena pages are purged immediately rather than being reused verbatim from the abandoned-page pool. Before this change, the error messages come back as garbage bytes (or ASAN aborts with use-after-poison). After, every rejection carries the expectedExpected identifier but found "@"message.Found by Fuzzilli.