Fix use-after-free in Bun.Transpiler async transform() errors#29958
Fix use-after-free in Bun.Transpiler async transform() errors#29958robobun wants to merge 2 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 35 seconds. Comment |
|
Updated 11:37 AM PT - May 4th, 2026
❌ @robobun, your commit 3586dbf has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29958That installs a local version of the PR into your bun-29958 --bun |
There was a problem hiding this comment.
LGTM — small, well-understood UAF fix that mirrors the existing RuntimeTranspilerStore pattern, with a regression test.
Extended reasoning...
Overview
This PR fixes a use-after-free in Bun.Transpiler#transform() (the async variant). TransformTask.run() runs on a worker thread, creates a per-task MimallocArena, and points the transpiler at it. Parse diagnostics were being written into this.log with text slices that pointed into arena memory; the arena was destroyed at the end of run(), but then() later read this.log on the JS thread to build the rejection — reading freed memory. The fix routes diagnostics through a temporary arena-backed Log and, in a defer that fires before arena.deinit(), calls log.appendToWithRecycled(&this.log, true) to deep-copy message text/notes into bun.default_allocator. A new test hammers 1200 concurrent failing transforms to exercise the path that previously tripped ASAN.
Security risks
None. This is internal allocator-lifetime bookkeeping for diagnostic strings; there's no change to parsing semantics, trust boundaries, or input handling. The fix strictly removes a memory-safety hazard (UAF → potential crash/corruption).
Level of scrutiny
Low–medium. The diff is ~5 lines of Zig in one function plus a test. It exactly mirrors an established pattern at src/bun.js/RuntimeTranspilerStore.zig:323-327 and src/bun.js/api/server/HTMLBundle.zig:330. I verified cloneToWithRecycled in src/logger.zig:802-828 allocates the cloned text/notes from other.msgs.allocator (here bun.default_allocator), so the copies survive arena teardown. Defer ordering is correct: the clone defer is registered after defer arena.deinit(), so it executes first.
Other factors
- The only finding is a non-blocking nit:
Log.deinit()doesn't free the StringBuilder/notes buffers thatcloneToWithRecycledallocates, so each failing async transform leaks a small buffer. This is a pre-existing structural limitation oflogger.Log(TODO atlogger.zig:846), shared by every othercloneToWithRecycledcaller, error-path-only, and strictly better than the UAF it replaces. It's appropriately flagged as a follow-up rather than a blocker. log.levelis propagated fromthis.log.level, preserving the user-configuredlogLevel.- No CODEOWNERS cover this path; no outstanding human review comments.
|
CI failures are unrelated to this change:
This PR only touches |
|
@robobun fix the leak. |
|
Pushed a follow-up that tracks the StringBuilder buffer and notes array allocated by |
There was a problem hiding this comment.
The leak follow-up looks correct to me — recycled_string_bufs stores exactly what StringBuilder.allocate returned and is freed with the same msgs.allocator — but it changes Log.deinit() semantics for every cloneToWithRecycled caller (bundle_v2, BundleThread, ParseTask, RuntimeTranspilerStore, npm manifest parse, HTMLBundle, isolated install), including the by-value Log copies in npm.zig:1860 and BundleThread.zig:154/161, so it's worth a human pass.
Extended reasoning...
Overview
Two commits:
- JSTranspiler.zig: route parse diagnostics through a temporary arena-backed
LogandappendToWithRecycled(&this.log, true)before the arena is destroyed. This is the actual UAF fix and mirrorsRuntimeTranspilerStoreexactly. - logger.zig (follow-up after Jarred's request): add
recycled_string_bufs/recycled_notes_bufstoLog, populate them incloneToWithRecycled, and free them inLog.deinit(). This addresses the small error-path leak I flagged on the first commit and also plugs the same pre-existing leak at every othercloneToWithRecycledcall site.
Plus a 1200-iteration regression test that triggered the original ASAN use-after-poison.
Security risks
None. No untrusted input handling changes; this is allocator-lifetime bookkeeping.
Level of scrutiny
The first commit on its own would have been low-risk (pattern-match of an existing idiom). The second commit raises the bar: logger.Log is used pervasively across the bundler, transpiler, install, and dev server, and deinit() (aliased as clearAndFree) now performs additional frees. I checked:
StringBuilder.allocatereturnsallocator.alloc(u8, cap); the stashed sliceptr.?[0..cap]matches whatStringBuilder.deinitwould free, so the free is valid.- The bufs are allocated with
other.msgs.allocatorand freed withlog.msgs.allocator; the only place that reassignsmsgs.allocator(bundle_v2.zig:953) does so at init time before any clone, so allocator mismatch shouldn't occur. ArrayListUnmanaged.clearAndFreeleaves the list in a reusable.emptystate, so theclearAndFree = deinitalias remains safe for logs that are cleared and reused.- By-value
Logcopies (npm.zig:1860log.* = cloned_log; BundleThread.zigcompletion.log = out_log) carry the unmanaged lists correctly since they're just slice+capacity. Log.reset()does not clear the new lists, so a log that isreset()repeatedly between clones will accumulate buffer pointers untildeinit()— not a correctness issue, just worth noting.
I did not find a path that would double-free or free with the wrong allocator.
Other factors
A maintainer is already engaged on this PR and explicitly requested the second commit. Given that the second commit touches memory-ownership semantics in a core shared type with ~10 distinct callers across hot paths, I'd rather have a human confirm the by-value-copy and reset() interactions than auto-approve.
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Do not make logger.Log larger.
|
Reworked to keep |
79ba228 to
0621520
Compare
There was a problem hiding this comment.
LGTM — the rework addresses the leak by tracking log_string_buf/log_notes_buf on the task and freeing them in deinit().
Extended reasoning...
Overview
Fixes a use-after-free in Bun.Transpiler.transform() (async path). TransformTask.run() previously pointed the transpiler's log at this.log while parsing inside a per-run MimallocArena, so error message text lived in arena memory that was destroyed before then() read it on the JS thread. The fix parses into a local arena-backed Log, then cloneArenaLog() (via defer, which fires before arena.deinit()) copies each Msg into this.log using the existing Msg.count / Msg.cloneWithBuilder machinery from src/logger.zig. The two heap buffers backing the cloned strings and notes are stored on the task (log_string_buf, log_notes_buf) and freed in TransformTask.deinit(). A new regression test hammers 1200 concurrent failing transforms.
Security risks
None. This is internal memory-lifetime management for diagnostic strings; no auth, network, filesystem, or untrusted-input parsing semantics changed. The fix strictly removes a UAF that was previously reachable from user JS.
Level of scrutiny
Medium. It touches cross-thread lifetime handling, but the change is narrow and entirely contained in TransformTask: it reuses the established cloneWithBuilder pattern already used by cloneToWithRecycled in logger.zig, the defer ordering is correct (clone → ast_scope.exit() → arena.deinit()), run() executes exactly once per task so the buffers are assigned once, and freeing empty slices on the success path is a no-op. The earlier broader approach that modified logger.Log was backed out in favor of this localized fix, which keeps blast radius minimal.
Other factors
My prior nit about the StringBuilder/notes buffers leaking has been addressed — deinit() now frees both, and the inline comment is resolved. Jarred requested the leak fix and the author delivered. CI passed on all platforms for the new test; the two reported failures are known pre-existing flakes (bun-create GitHub rate-limit, HTTP/2 client ASAN) unrelated to this diff.
TransformTask.run() allocates parse error messages from a per-task MimallocArena that is destroyed when run() returns. TransformTask.then() later reads those messages to build the rejection value, reading freed memory. Use a temporary log during parsing and clone its messages into the task's default-allocator-backed log before the arena is torn down, the same way RuntimeTranspilerStore already does.
Instead of appendToWithRecycled (which leaks its StringBuilder/notes allocations because Log.deinit does not free them), clone the arena-backed diagnostics directly in TransformTask, stash the backing buffers on the task, and free them in TransformTask.deinit().
0621520 to
3586dbf
Compare
There was a problem hiding this comment.
LGTM — the rework addresses the leak nit by tracking the cloned string/notes buffers on TransformTask and freeing them in deinit().
Extended reasoning...
Overview
This PR fixes a use-after-free in Bun.Transpiler.transform()'s async error path. TransformTask.run() previously pointed the transpiler's log directly at this.log while parsing inside a per-run MimallocArena; parse-error message text was allocated from that arena, the arena was destroyed at the end of run(), and then() later read the freed memory via this.log.toJS(). The fix routes diagnostics through a local arena-backed log and, on scope exit, clones them into bun.default_allocator-owned buffers (log_string_buf, log_notes_buf) stored on the task, which are freed in TransformTask.deinit(). A regression test hammers 1200 concurrent failing transforms.
Security risks
None. This is internal memory-lifetime management for diagnostic strings; no auth, crypto, parsing of untrusted control data, or external I/O is touched. The change strictly reduces risk by eliminating a UAF reachable from user JS.
Level of scrutiny
Medium — it's allocator/lifetime code in the runtime, but the implementation is a near-verbatim inline of the existing, well-exercised Log.cloneToWithRecycled pattern (Msg.count → StringBuilder.allocate → Msg.cloneWithBuilder), with the only behavioral difference being that the two backing buffers are stashed on the task so they can be freed. Defer ordering is correct (cloneArenaLog runs before ast_scope.exit() and arena.deinit()). The success path (no diagnostics) early-returns with zero allocations, and zero-length frees in deinit() are safe.
Other factors
- My earlier nit about
appendToWithRecycledleaking the StringBuilder/notes buffers was acted on: the maintainer asked for a fix, and the final revision keepslogger.Loguntouched while plugging the leak locally. - CI (including the ASAN job) passed; the two unrelated flakes are pre-existing and tracked.
- Net diff is confined to
TransformTaskinsrc/runtime/api/JSTranspiler.zigplus a new test; no CODEOWNERS cover this path. - Removing the redundant
this.log.msgs.allocator = bun.default_allocatorline is safe —this.logis already initialized withbun.default_allocatorincreate().
What
TransformTask.run()creates aMimallocArena, points the transpiler at it, parses the input, and destroys the arena before returning. When parsing fails, the lexer/parser write error messages whose text is allocated from that arena intothis.log.TransformTask.then()later callsthis.log.toJS()on the JS thread to build the rejection value, which reads the (now destroyed) arena memory.ASAN reports this as
use-after-poisononce the freed arena pages are reused and re-poisoned by a subsequent task, so it only shows up under load.Fix
Collect parse diagnostics into a temporary arena-backed
Logduringrun()andappendToWithRecycled(&this.log, true)before the arena is torn down, so the strings are duplicated intobun.default_allocator. This mirrors whatRuntimeTranspilerStorealready does for the same reason.Repro
Before: ASAN
use-after-poisoninlogger.Data.clone→logger.Msg.clone→BuildMessage.create→Log.toJS→TransformTask.then(or a straight SIGSEGV at higher concurrency).After: resolves with the expected
BuildMessageerrors.Found by Fuzzilli.