Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions src/runtime/api/JSTranspiler.zig
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,9 @@ pub const TransformTask = struct {
this.transpiler.setAllocator(allocator);
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));
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +506 to +508

@claude claude Bot May 5, 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.

Done — added msg.deinit(bun.default_allocator) for each message in TransformTask.deinit() before log.deinit().

Comment on lines +506 to +508

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.

🟡 Minor: the msg.deinit(bun.default_allocator) cleanup added in TransformTask.deinit() won't actually free everything msg.clone() allocated — Location.deinit is a no-op (logger.zig:149), so the duped location.file and location.line_text strings (for the message and each note) leak on the global heap per failing async transform(). Tiny error-path-only leak and the root asymmetry is pre-existing in Location, so not a blocker for this UAF fix — but if you want full cleanup here you'd need to free those two fields manually (or fix Location.deinit).

Extended reasoning...

What the bug is

The new defer in TransformTask.run() deep-clones each log message into bun.default_allocator:

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

Msg.cloneData.cloneLocation.clone (src/logger/logger.zig:113-123), which does allocator.dupe(u8, this.file) and allocator.dupe(u8, this.line_text.?). And Msg.clone also calls bun.clone(this.notes, allocator), which (per src/bun.zig:551-557) detects Data.clone and deep-clones every note, so each note's location.file / location.line_text is also duped onto bun.default_allocator.

The matching cleanup added in TransformTask.deinit():

for (this.log.msgs.items) |*msg| msg.deinit(bun.default_allocator);

calls Msg.deinitData.deinitLocation.deinit. But Location.deinit is intentionally stubbed:

// don't really know what's safe to deinit here!
pub fn deinit(_: *Location, _: std.mem.Allocator) void {}

So Data.deinit frees .text and the notes slice is freed, but every Location.file and Location.line_text that was just duped onto the global heap is never freed.

Code path that triggers it

  1. transpiler.transform("const {a, a} = b") schedules a TransformTask.
  2. run() parses on a worker thread; the parser pushes a Msg with data.location = {file: "input.tsx", line_text: "const {a, a} = b"} and one note with its own location, all allocated from the per-task MimallocArena.
  3. On scope exit, the new defer runs first (LIFO) and msg.clone(bun.default_allocator) heap-dupes: data.text, data.location.file, data.location.line_text, the notes slice, notes[0].text, notes[0].location.file, notes[0].location.line_text.
  4. arena.deinit() reclaims the originals (correct — that's the UAF fix).
  5. then() runs on the main thread, builds the JS error, then calls this.deinit().
  6. deinit() runs msg.deinit(bun.default_allocator) for each message → frees data.text, notes[0].text, and the notes slice. Location.deinit is a no-op, so the four duped location strings (2 × file + 2 × line_text) stay on the global mimalloc heap forever.

Why existing code doesn't prevent it

  • this.log.deinit() only frees the msgs list backing storage, not per-message payloads.
  • Msg.deinit is the maximally-correct API to call here and the author called it; the gap is one layer down in Location.deinit, which has carried the comment "don't really know what's safe to deinit here!" since long before this PR.
  • Before this PR, these strings lived in the per-task arena and were swept by arena.deinit() (with a UAF, which this PR correctly fixes). Moving them to bun.default_allocator is what makes the no-op Location.deinit matter.

Impact

Per failing async transform(): (1 + #notes) × (len(file) + len(line_text)) bytes leaked from the global heap. For the PR's own test ("const {a, a} = b", 1 note), that's ≈ 2 × ("input.tsx" + "const {a, a} = b") ≈ 50 bytes per call. Error-path only; the success path has no log messages and leaks nothing.

Worth noting: the same Location.clone/Location.deinit asymmetry already bites BuildMessage.createBuildMessage.finalize on this exact path (BuildMessage.zig:54/186), so this PR roughly doubles a pre-existing per-error leak rather than introducing a brand-new leak class.

Step-by-step proof

For one call to transpiler.transform("const {a, a} = b"):

Allocation in msg.clone(bun.default_allocator) Freed by msg.deinit(bun.default_allocator)?
data.text ('"a" has already been declared') Data.deinit line 214
data.location.file ("input.tsx") Location.deinit no-op line 149
data.location.line_text ("const {a, a} = b") Location.deinit no-op line 149
notes slice ([1]Data) Msg.deinit line 484
notes[0].text Data.deinit line 214
notes[0].location.file Location.deinit no-op
notes[0].location.line_text Location.deinit no-op

Four small allocations leak per failed transform.

How to fix

Either locally in TransformTask.deinit():

for (this.log.msgs.items) |*msg| {
    if (msg.data.location) |*loc| {
        bun.default_allocator.free(loc.file);
        if (loc.line_text) |lt| bun.default_allocator.free(lt);
    }
    for (msg.notes) |*note| if (note.location) |*loc| {
        bun.default_allocator.free(loc.file);
        if (loc.line_text) |lt| bun.default_allocator.free(lt);
    };
    msg.deinit(bun.default_allocator);
}

…or, better long-term, make Location.deinit actually free what Location.clone allocates (which would also fix the identical leak in BuildMessage.finalize). The latter is out of scope for this PR; flagging as a nit since the author explicitly added msg.deinit intending full cleanup.


const jsx = if (this.tsconfig != null)
this.tsconfig.?.mergeJSX(this.transpiler.options.jsx)
Expand Down Expand Up @@ -587,6 +590,7 @@ pub const TransformTask = struct {
}

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
44 changes: 44 additions & 0 deletions test/js/bun/transpiler/transpiler-async-error-uaf.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { expect, test } from "bun:test";

test("concurrent async transform() rejections do not use-after-free", async () => {
const transpiler = new Bun.Transpiler({ loader: "tsx" });

const inputs = [
"const {a, a} = b",
"class X { @invalid }",
"const x: string = ;",
"@#$%^ invalid syntax !!!",
"function (",
];

const promises: Promise<unknown>[] = [];
for (let i = 0; i < 20; i++) {
for (const input of inputs) {
promises.push(transpiler.transform(input).catch(e => e));
}
}

const results = await Promise.all(promises);
expect(results).toHaveLength(20 * inputs.length);
for (const result of results) {
expect(result).toBeDefined();
expect(typeof result).toBe("object");
}
});

test("async transform() error preserves message and notes", async () => {
const transpiler = new Bun.Transpiler({ loader: "tsx" });

const errors = await Promise.all(
Array.from({ length: 8 }, () => transpiler.transform("const {a, a} = b").catch(e => e)),
);

for (const err of errors) {
expect(err.message).toBe('"a" has already been declared');
expect(err.position?.file).toBe("input.tsx");
expect(err.position?.lineText).toBe("const {a, a} = b");
expect(err.notes).toHaveLength(1);
expect(err.notes[0].message).toBe('"a" was originally declared here');
expect(err.notes[0].position?.lineText).toBe("const {a, a} = b");
}
});
Loading