Skip to content

Fix use-after-free in Bun.Transpiler async transform error messages #30229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 2 commits into from
Closed

Fix use-after-free in Bun.Transpiler async transform error messages #30229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d88bae
Fix use-after-free in Bun.Transpiler async transform error messages
robobun May 4, 2026
0416262
Free cloned log message contents in TransformTask.deinit
robobun May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/bun.js/api/JSTranspiler.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,10 @@
this.transpiler.setLog(&this.log);
this.log.msgs.allocator = bun.default_allocator;

defer for (this.log.msgs.items) |*msg| {
msg.* = bun.handleOom(msg.clone(bun.default_allocator));
};

Check failure on line 509 in src/bun.js/api/JSTranspiler.zig

View check run for this annotation

Claude / Claude Code Review

Memory leak: cloned Location.file and line_text are never freed 

This introduces a memory leak: `msg.clone(bun.default_allocator)` deep-clones `Location.file` and `Location.line_text` (via `Location.clone`, src/logger.zig:113-123), but `Location.deinit` is a no-op (src/logger.zig:149), so the new `msg.deinit(bun.default_allocator)` in `TransformTask.deinit` never frees them. Every rejected async `transform()` now leaks at least two `bun.default_allocator` allocations per error message (and per note); previously these strings lived in the per-task arena and we
Comment on lines +507 to +509

@claude claude Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 This introduces a memory leak: msg.clone(bun.default_allocator) deep-clones Location.file and Location.line_text (via Location.clone, src/logger.zig:113-123), but Location.deinit is a no-op (src/logger.zig:149), so the new msg.deinit(bun.default_allocator) in TransformTask.deinit never frees them. Every rejected async transform() now leaks at least two bun.default_allocator allocations per error message (and per note); previously these strings lived in the per-task arena and were reclaimed by arena.deinit(). Either make Location.deinit free file/line_text, or free them explicitly in the TransformTask.deinit loop.

Extended reasoning...

What the bug is

The PR fixes a use-after-free by deep-cloning each log message out of the per-task MimallocArena into bun.default_allocator before the arena is torn down, and then freeing those clones in TransformTask.deinit. However, the clone path duplicates more than the deinit path frees: Msg.clone → Data.clone → Location.clone calls allocator.dupe(u8, this.file) and allocator.dupe(u8, this.line_text.?) (src/logger.zig:113-123), but Location.deinit is literally pub fn deinit(_: *Location, _: std.mem.Allocator) void {} with the comment "don't really know what's safe to deinit here!" (src/logger.zig:149). Data.deinit (src/logger.zig:209-215) calls loc.deinit() (no-op) and then frees only data.text. So the cloned location.file and location.line_text strings are leaked.

Code path that triggers it

  1. TransformTask.run() parses invalid source; the lexer/parser appends an error Msg whose data.location is populated by Location.initOrNull (src/logger.zig:163-193) — file = source.path.text (e.g. "input.js") and line_text = <slice of source.contents>.
  2. The new defer for (this.log.msgs.items) |*msg| msg.* = bun.handleOom(msg.clone(bun.default_allocator)); runs, which calls Data.clone → Location.clone, duping both file and line_text into bun.default_allocator. The same happens for every note (Msg.clone clones each note's Data too).
  3. TransformTask.deinit() runs for (this.log.msgs.items) |*msg| msg.deinit(bun.default_allocator); → Data.deinit frees data.text and calls the no-op Location.deinit; then Msg.deinit frees the notes slice. file and line_text are never freed.

Why existing code doesn't prevent it

Location.deinit has always been a no-op because in every other caller the Location strings are borrowed slices (into source.path.text / source.contents or into an arena). Before this PR, TransformTask was in the same boat: the strings lived in the per-task MimallocArena and were reclaimed wholesale by arena.deinit(). This PR is the first place that puts Location.clone's heap-owned copies into bun.default_allocator and expects msg.deinit to clean them up — but msg.deinit was never designed to do that.

Impact

Every rejected async transform() leaks at least two small heap allocations (the "input.<ext>" filename and the offending source line) per error message, plus two more per note (e.g. the redeclaration error in the new test has a note with its own location). For the new regression test alone that's ~400+ leaked allocations per run. It's a small per-call leak, but it is a new leak introduced by a PR whose purpose is memory correctness, and it accumulates unbounded in long-running processes that repeatedly transpile invalid input.

Step-by-step proof

Take the second test case, transpiler.transform("const x = 1; const x = 2;", "js"):

  1. Parser emits a redeclaration error Msg with data.location = { file: "input.js", line_text: "const x = 1; const x = 2;", ... } and one note Data with its own location pointing at the original declaration.
  2. The new defer clones the message: Location.clone dupes "input.js" (8 bytes) and "const x = 1; const x = 2;" (25 bytes) for data.location, and again for the note's location — four bun.default_allocator.dupe calls.
  3. then() rejects the promise, then deinit() runs: msg.deinit → data.deinit frees data.text, calls Location.deinit (no-op); iterates notes, frees each note's text, calls Location.deinit (no-op); frees the notes slice.
  4. The four duped location strings are never passed to allocator.free. With 100 iterations × 4 allocations ≈ 400 leaked allocations for this one test.

How to fix

Either:

  • Free the location fields explicitly in the new TransformTask.deinit loop (and for each note), since this is the only place that owns heap-allocated Location strings in bun.default_allocator; or
  • Use the StringBuilder-based Msg.cloneWithBuilder path so all cloned strings share a single allocation that can be freed in one shot; or
  • Make Location.deinit actually free file and line_text (riskier — other callers pass borrowed slices and would double-free/invalid-free).


const jsx = if (this.tsconfig != null)
this.tsconfig.?.mergeJSX(this.transpiler.options.jsx)
else
Expand Down Expand Up @@ -587,6 +591,7 @@
}

pub fn deinit(this: *TransformTask) void {
for (this.log.msgs.items) |*msg| msg.deinit(bun.default_allocator);
this.log.deinit();
this.input_code.deinitAndUnprotect();
this.output_code.deref();
Expand Down
29 changes: 29 additions & 0 deletions test/js/bun/transpiler/transpiler-transform-error-uaf.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { expect, test } from "bun:test";

test("concurrent async transform() with parse errors produces correct error messages", async () => {
const transpiler = new Bun.Transpiler();

const promises: Promise<unknown>[] = [];
for (let i = 0; i < 100; i++) {
promises.push(transpiler.transform("const x = ;", "js").catch(e => e));
}

const errors = await Promise.all(promises);
for (const err of errors) {
expect(String(err)).toContain("Unexpected");
Comment on lines +6 to +13

@coderabbitai coderabbitai Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert rejection status explicitly to prevent false positives.

On Line 8 and Line 22, .catch(e => e) turns failures and successes into fulfilled values. The "x" assertion can pass even if transform() unexpectedly resolves. Verify each call is actually rejected before checking message text.

Suggested fix 
-  const promises: Promise<unknown>[] = [];
+  const promises: Promise<unknown>[] = [];
   for (let i = 0; i < 100; i++) {
-    promises.push(transpiler.transform("const x = ;", "js").catch(e => e));
+    promises.push(transpiler.transform("const x = ;", "js"));
   }

-  const errors = await Promise.all(promises);
-  for (const err of errors) {
-    expect(String(err)).toContain("Unexpected");
+  const results = await Promise.allSettled(promises);
+  for (const result of results) {
+    expect(result.status).toBe("rejected");
+    if (result.status === "rejected") {
+      expect(String(result.reason)).toContain("Unexpected");
+    }
   }
@@
-  const promises: Promise<unknown>[] = [];
+  const promises: Promise<unknown>[] = [];
   for (let i = 0; i < 100; i++) {
-    promises.push(transpiler.transform("const x = 1; const x = 2;", "js").catch(e => e));
+    promises.push(transpiler.transform("const x = 1; const x = 2;", "js"));
   }

-  const errors = await Promise.all(promises);
-  for (const err of errors) {
-    expect(String(err)).toContain(`"x"`);
+  const results = await Promise.allSettled(promises);
+  for (const result of results) {
+    expect(result.status).toBe("rejected");
+    if (result.status === "rejected") {
+      expect(String(result.reason)).toContain(`"x"`);
+    }
   }

Also applies to: 20-27

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/js/bun/transpiler/transpiler-transform-error-uaf.test.ts` around lines 6
- 13, The test currently uses .catch(e => e) which converts rejections into
fulfilled values and can mask unexpected successes; change the logic to use
Promise.allSettled on the array of transpiler.transform(...) calls (or replace
.catch with no handler and await each with try/catch), then for each result
assert result.status === "rejected" and that String(result.reason) contains
"Unexpected". Update references to the promises array and transpiler.transform
calls accordingly so each invocation's rejection status is asserted before
checking the error message.

}
});

test("concurrent async transform() with redeclaration errors produces correct error messages", async () => {
const transpiler = new Bun.Transpiler();

const promises: Promise<unknown>[] = [];
for (let i = 0; i < 100; i++) {
promises.push(transpiler.transform("const x = 1; const x = 2;", "js").catch(e => e));
}

const errors = await Promise.all(promises);
for (const err of errors) {
expect(String(err)).toContain(`"x"`);
}
});
Loading