JSTranspiler: fix use-after-free of log messages in async transform()#30180
JSTranspiler: fix use-after-free of log messages in async transform()#30180robobun wants to merge 1 commit 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 (3)
WalkthroughModified ChangesAsync Log Memory Safety
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 15 seconds. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
f38be6a to
2724a5e
Compare
2724a5e to
8f17903
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/transpiler/transpiler-async-log-uaf.test.ts`:
- Around line 10-16: The test forces GC immediately after queuing transforms
which can race with the worker's run() teardown; update the repro to wait for
the worker run to finish before calling Bun.gc(true) by following the repo UAF
pattern: run the repro inside a spawned subprocess, after scheduling the 50
transpiler.transform calls block the JS thread long enough for the worker's
run() to complete (e.g., use Bun.sleepSync(N) in the subprocess) then call
Bun.gc(true) and only afterwards consume the Promise results with
Promise.allSettled; reference transpiler.transform and the worker run()
lifecycle when making the change so GC happens after run() teardown.
🪄 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: b546c2e8-0f39-4448-bb43-ab01eeba93f6
📒 Files selected for processing (2)
src/bun.js/api/JSTranspiler.zigtest/js/bun/transpiler/transpiler-async-log-uaf.test.ts
ac81a64 to
267263c
Compare
| var log = logger.Log.init(allocator); | ||
| log.level = this.log.level; | ||
| defer bun.handleOom(log.cloneToWithRecycled(&this.log, true)); |
There was a problem hiding this comment.
🟡 Nit (pre-existing pattern, not blocking): cloneToWithRecycled() allocates the string-builder backing buffer and notes_buf from bun.default_allocator, but Log.deinit() only frees the msgs ArrayList — so each async transform() that emits parse errors now leaks those two small allocations. This is the same leak that already exists in RuntimeTranspilerStore.zig:326 (and other cloneToWithRecycled callers), and trading the UAF for a bounded error-path leak is the right call here; the proper fix belongs in logger.zig (have Log track/free the cloned buffers) as a follow-up.
Extended reasoning...
What the bug is
cloneToWithRecycled() (src/logger.zig:797-823) deep-copies log messages by computing the total byte length of all message text/locations, calling string_builder.allocate(other.msgs.allocator) for one contiguous string buffer, and other.msgs.allocator.alloc(Data, notes_count) for the notes array. In TransformTask.run(), other is this.log, which was initialized in TransformTask.create() with logger.Log.init(bun.default_allocator) — so both allocations come from bun.default_allocator.
The cloned Msg structs hold slices into these two buffers, but the Log struct itself (logger.zig:604-610) has no field tracking the backing allocations. Log.deinit() (logger.zig:835-836) is just msgs.clearAndFree(), which frees the ArrayList(Msg) storage but never frees the string-builder buffer or the notes buffer. TransformTask.deinit() calls this.log.deinit() and nothing else touches these buffers, so they leak.
Code path
TransformTask.create()→this.log = logger.Log.init(bun.default_allocator), sothis.log.msgs.allocator == bun.default_allocator.TransformTask.run()parses with an arena-backed locallog; on scope exit,defer log.cloneToWithRecycled(&this.log, true)runs.- Inside
cloneToWithRecycled:string_builder.allocate(this.log.msgs.allocator)andthis.log.msgs.allocator.alloc(Data, notes_count)allocate frombun.default_allocator. The localstring_buildergoes out of scope; the only references to its buffer are theMsg.data.text/.locationslices. then()callsthis.log.toJS()→BuildMessage.create→Msg.clonewhichallocator.dupe()s the strings again into JS-owned memory. Ownership of the original buffers is never transferred.TransformTask.deinit()→this.log.deinit()→msgs.clearAndFree(). The string buffer and notes buffer are never freed.
Why existing code doesn't prevent it
Log simply has no ownership model for the auxiliary buffers cloneToWithRecycled creates — there is no field to store them and deinit() doesn't iterate messages. This is a design gap in logger.zig, not something specific to this call site. The identical leak already exists at src/bun.js/RuntimeTranspilerStore.zig:326 (the pattern this PR explicitly follows), and in other cloneToWithRecycled callers like bundle_v2.zig / HTMLBundle.zig.
Impact
Small, error-path-only leak: each async transform() call that produces parse errors/warnings leaks one string buffer (sized to the total error text) plus one []Data for notes, from bun.default_allocator. Before this PR the same strings lived in the arena, which was freed (hence the UAF) — so this PR converts a crash into a bounded leak, which is a strict improvement.
Step-by-step proof
Repro: for (let i = 0; i < N; i++) await t.transform('const x: = y;', 'ts').catch(()=>{});
- Each iteration emits ≥1 error message →
cloneToWithRecycledallocates a string buffer of ~tens of bytes frombun.default_allocator. then()rejects,deinit()runs,msgs.clearAndFree()frees theMsg[]array.- No code path holds a pointer to the start of the string-builder allocation, and
Loghas no field for it, so it cannot be freed. - RSS grows linearly in N.
How to fix (follow-up, not for this PR)
Fix in logger.zig rather than at each call site: either have Log store the string_builder buffer / notes_buf handles populated by cloneToWithRecycled and free them in Log.deinit(), or have cloneToWithRecycled allocate per-message owned strings that Msg.deinit can free. Given the same pattern is used in RuntimeTranspilerStore, a single fix there cleans up all call sites.
There was a problem hiding this comment.
LGTM — the dead-code nit was addressed and the fix mirrors the existing RuntimeTranspilerStore pattern.
Extended reasoning...
Overview
This PR fixes a use-after-free in Bun.Transpiler.prototype.transform() (async variant). TransformTask.run() parses on a worker thread using a MimallocArena that is freed when run() returns, but log message text/locations were allocated from that arena and later read by then() on the JS thread. The fix introduces a local arena-backed logger.Log, points the transpiler at it, and defers a cloneToWithRecycled(&this.log, true) deep-copy into the task's persistent log before the arena is torn down. This is a verbatim copy of the pattern already used at src/bun.js/RuntimeTranspilerStore.zig:323-326. Two new test files add a subprocess-based regression test that queues 64 failing transforms, sleeps to let workers finish, forces GC, then asserts the rejections contain the expected parse error and that ASAN did not abort.
Security risks
None. This is a memory-safety hardening change with no new inputs, parsing, auth, or external I/O. It strictly reduces the window where freed memory can be read.
Level of scrutiny
Low-to-moderate. The Zig change is ~10 lines, mechanical, and follows an established in-repo idiom for the identical problem. The defer ordering is correct: cloneToWithRecycled runs before ast_scope.exit() and arena.deinit() (LIFO), so the source strings are still live during the copy. this.log is initialized with bun.default_allocator in create(), so the cloned buffers outlive run(). The previously-flagged dead assignment this.log.msgs.allocator = bun.default_allocator has been removed in the latest revision.
Other factors
Both prior review threads on this PR are resolved: my dead-code nit was fixed, and the test-timing concern was addressed by moving the repro to a spawned subprocess with Bun.sleepSync(100) before Bun.gc(true). My remaining note about the small error-path leak in cloneToWithRecycled is explicitly a pre-existing logger.zig design issue shared with other call sites and is not blocking. A possible duplicate (#29958) was flagged by the bot, which maintainers may want to reconcile, but that does not affect the correctness of this change. No bugs were found by the bug-hunting system on this revision.
…sync transform TransformTask.run() parses on a worker thread with an arena allocator that is freed when run() returns. Error messages written to the log during parsing had their text/location strings allocated from that arena, so when then() later converted the log to JS on the main thread it read from freed memory. Use a temporary arena-backed log during run() and clone it into the task's persistent log with cloneToWithRecycled() before the arena is torn down, matching what RuntimeTranspilerStore already does.
267263c to
e8d7d63
Compare
What does this PR do?
Bun.Transpiler.prototype.transform()(the async variant) runs parsing on a worker thread using a temporaryMimallocArenathat is destroyed whenTransformTask.run()returns. Error messages written to the log during parsing have theirtextandlocationstrings allocated from that arena.Previously the task's
this.logwas passed directly to the transpiler, so whenthen()later calledthis.log.toJS()on the JS thread,Msg.clone→Data.clonewouldallocator.dupe()strings that pointed into the already-freed arena.Fix by using a temporary arena-backed log during
run()and deep-copying it intothis.logviacloneToWithRecycled()before the arena is torn down — the same patternRuntimeTranspilerStorealready uses.How did you verify your code works?
Minimal repro that crashed ~1/12 runs under ASAN before and passes 50/50 after:
Added as
test/js/bun/transpiler/transpiler-async-log-uaf.test.ts. Existingtest/bundler/transpiler/transpiler.test.jsandtest/js/bun/transpiler/suites pass.