Skip to content

ResolveMessage/BuildMessage: match allocator in finalize and destroy struct #29840

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

Merged
Jarred-Sumner merged 3 commits into from
Apr 28, 2026
Merged

ResolveMessage/BuildMessage: match allocator in finalize and destroy struct #29840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a9ada4
ResolveMessage/BuildMessage: match allocator in finalize and destroy …
robobun Apr 28, 2026
526dc09
Add BuildMessage finalize stress test
robobun Apr 28, 2026
3921990
ini: don't pass stack-local arena to log.toJS
robobun Apr 28, 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
3 changes: 2 additions & 1 deletion src/bun.js/BuildMessage.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ pub const BuildMessage = struct {
}

pub fn finalize(this: *BuildMessage) void {
this.msg.deinit(bun.default_allocator);
this.msg.deinit(this.allocator);
this.allocator.destroy(this);
Comment on lines +186 to +187

@claude claude Bot Apr 28, 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.

🟣 🟣 Pre-existing (not introduced by this PR): two leaks remain in the exact BuildMessage clone/finalize lifecycle this PR is making leak-free. (1) Location.deinit (src/logger.zig:149) is a no-op even though Location.clone dupes file and line_text, so every BuildMessage/ResolveMessage whose msg.data.location is non-null still leaks those two strings on finalize. (2) BuildMessage.getNotes calls note.clone(bun.default_allocator) and then passes the result to BuildMessage.create(), which clones again — the first clone's .text/location strings are never freed. Both are exercised by the new build-error.test.ts stress loop (e.position, e.notes); not blocking, but might be worth folding in while you're touching this.

Extended reasoning...

What the bugs are

Two distinct leaks remain on the logger.Msg clone/deinit chain that BuildMessage.finalize() now drives. Neither is introduced by this PR — both were present before — but both sit in the exact lifecycle this PR is making leak-free, and both are exercised by the new build-error.test.ts stress test.

(1) Location.deinit is a no-op even though Location.clone allocates

src/logger.zig:113-123:

pub fn clone(this: Location, allocator: std.mem.Allocator) !Location {
    return Location{
        .file = try allocator.dupe(u8, this.file),
        ...
        .line_text = if (this.line_text != null) try allocator.dupe(u8, this.line_text.?) else null,
        ...
    };
}

src/logger.zig:148-149:

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

The deinit chain on finalize is BuildMessage.finalize → Msg.deinit (logger.zig:502) → Data.deinit (logger.zig:209) → Location.deinit (logger.zig:149). Data.deinit calls loc.deinit(allocator) and then frees only d.text; Location.deinit itself does nothing. So the duped file and line_text strings produced by Location.clone are never freed.

(2) BuildMessage.getNotes double-clones each note

src/bun.js/BuildMessage.zig:16-29:

for (notes, 0..) |note, i| {
    const cloned = try note.clone(bun.default_allocator);   // clone #1: Data.clone dupes .text + location
    try array.putIndex(
        globalThis,
        @intCast(i),
        try BuildMessage.create(globalThis, bun.default_allocator,
            logger.Msg{ .data = cloned, .kind = .note }),    // create() calls msg.clone() → clone #2
    );
}

BuildMessage.create() at line 54 does try msg.clone(allocator), which calls Data.clone again on the already-cloned data. Clone #2 is stored in the new BuildMessage and freed by its finalize(); clone #1 (cloned) is a stack-local Data whose heap-backed .text (and .location strings) are never deinit'd. The note.clone() call is completely redundant.

Step-by-step proof (leak 1)

  1. The new test parses function bad( { → the bundler emits at least one parse-error BuildMessage whose msg.data.location is non-null (parse errors carry file + line_text).
  2. BuildMessage.create() calls msg.clone(allocator) (BuildMessage.zig:54) → Msg.clone (logger.zig:437) → Data.clone (logger.zig:230) → Location.clone (logger.zig:113), which allocator.dupes file and line_text.
  3. Bun.gc(true) sweeps the wrapper → finalize() calls this.msg.deinit(this.allocator).
  4. Msg.deinit → Data.deinit calls loc.deinit(allocator) (no-op) then allocator.free(d.text). The duped file and line_text are never passed to allocator.free.
  5. Result: ~2 string allocations leak per BuildMessage per iteration × 20 iterations.

Step-by-step proof (leak 2)

  1. The new test reads void e.notes; on each BuildMessage.
  2. For each entry in this.msg.notes, getNotes calls note.clone(bun.default_allocator) → Data.clone dupes .text (and Location.clone dupes file/line_text if present). Call this allocation set A.
  3. The wrapped Msg{ .data = cloned } is passed to BuildMessage.create(), which calls msg.clone(allocator) → Data.clone again, producing allocation set B stored in the child BuildMessage.
  4. When the child is GC'd, finalize() frees B. A is never referenced again and leaks per note per .notes access.

(Caveat: whether function bad( { produces a message with non-empty notes depends on the parser; if notes.len == 0 the loop body doesn't run and only leak (1) is hit by this specific test. The double-clone is real regardless.)

Why existing code doesn't prevent it

This PR correctly switches finalize() to this.msg.deinit(this.allocator) and adds this.allocator.destroy(this), but it relies on Msg.deinit to free everything Msg.clone allocated. The chain bottoms out in Location.deinit, which is intentionally empty (// don't really know what's safe to deinit here!). And getNotes is unchanged by this PR, so the redundant intermediate clone is still there.

Impact

Small, bounded per-message leaks (a couple of short strings each). They will not cause crashes or correctness issues, only gradual RSS growth in long-running processes that repeatedly surface build/resolve errors with locations or notes — exactly the Fuzzilli-REPRL-style scenario this PR is targeting. Severity: pre-existing / non-blocking.

Suggested fixes

For (1), make Location.deinit symmetric with Location.clone:

pub fn deinit(this: *Location, allocator: std.mem.Allocator) void {
    allocator.free(this.file);
    if (this.line_text) |t| allocator.free(t);
}

This is safe on the BuildMessage/ResolveMessage path because the Location was produced by Location.clone with the same allocator now passed to deinit. (Other callers of Data.deinit that did not go through clone would need auditing — which is presumably why the original author left it a no-op — so an alternative is to free location.file/line_text directly in Msg.deinit only when called from the cloned-msg path, or to add a Location.deinitCloned variant.)

For (2), drop the redundant clone in getNotes — create() already deep-copies:

for (notes, 0..) |note, i| {
    try array.putIndex(
        globalThis,
        @intCast(i),
        try BuildMessage.create(globalThis, bun.default_allocator, logger.Msg{ .data = note, .kind = .note }),
    );
}

}
Comment thread
claude[bot] marked this conversation as resolved.
};

Expand Down
3 changes: 2 additions & 1 deletion src/bun.js/ResolveMessage.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,11 @@ pub const ResolveMessage = struct {
}

pub fn finalize(this: *ResolveMessage) callconv(.c) void {
this.msg.deinit(bun.default_allocator);
this.msg.deinit(this.allocator);
if (this.referrer) |referrer| {
this.allocator.free(referrer.text);
}
this.allocator.destroy(this);
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/ini.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,7 +634,7 @@ pub const IniTestingAPIs = struct {
var configs = std.array_list.Managed(ConfigIterator.Item).init(allocator);
defer configs.deinit();
loadNpmrc(allocator, install, env, ".npmrc", &log, source, &configs) catch {
return log.toJS(globalThis, allocator, "error");
return log.toJS(globalThis, bun.default_allocator, "error");
};

const default_registry_url, const default_registry_token, const default_registry_username, const default_registry_password, const default_registry_email = brk: {
Expand Down
24 changes: 24 additions & 0 deletions test/js/bun/resolve/build-error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { tempDir } from "harness";
import { join } from "node:path";

test("BuildError is modifiable", async () => {
try {
await import("../util/inspect-error-fixture-bad.js");
Expand All @@ -15,3 +18,24 @@ test("BuildError is modifiable", async () => {
expect(error!.message).toBe("new message");
expect(error!.message).not.toBe(message);
});

test("BuildMessage finalize frees with the same allocator it was created with", async () => {
// BuildMessage.create() clones the message with the passed allocator
// but finalize() was freeing it with bun.default_allocator and never
// destroying the struct itself.
using dir = tempDir("build-message-finalize", { "bad.js": "function bad( {" });
const entry = join(String(dir), "bad.js");
for (let i = 0; i < 20; i++) {
const r = await Bun.build({ entrypoints: [entry], throw: false });
expect(r.success).toBe(false);
expect(r.logs.length).toBeGreaterThan(0);
for (const e of r.logs) {
void e.message;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
void e.level;
void e.position;
void e.notes;
void String(e);
}
Bun.gc(true);
}
});
33 changes: 33 additions & 0 deletions test/js/bun/resolve/resolve-error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,39 @@ describe("ResolveMessage", () => {
expect(err.referrer).toStartWith("/tmp/caf");
expect(err.referrer).toEndWith("/file.js");
});

it("finalize frees with the same allocator it was created with", () => {
// ResolveMessage.create() clones the message with the VM's arena
// allocator but finalize() was freeing it with bun.default_allocator
// and never destroying the struct itself. Under ASAN with mimalloc's
// per-heap tracking this surfaced as a flaky use-after-poison in the
// resolver after many failed require()s + GCs in a long-running
// process (Fuzzilli REPRL). Use relative specifiers so auto-install
// does not kick in.
for (let i = 0; i < 50; i++) {
let errs: any[] = [];
for (let j = 0; j < 10; j++) {
try {
Bun.resolveSync("./does-not-exist-" + j, import.meta.dir);
} catch (e) {
errs.push(e);
}
}
for (const e of errs) {
void e.message;
void e.code;
void e.specifier;
void e.referrer;
void e.level;
void e.importKind;
void e.position;
void String(e);
}
errs = [];
Bun.gc(true);
}
expect().pass();
});
Comment on lines +80 to +111

@coderabbitai coderabbitai Bot Apr 28, 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.

🧹 Nitpick | 🔵 Trivial

Add a matching BuildMessage regression.

This stress case only exercises the Bun.resolveSync / ResolveMessage path. src/bun.js/BuildMessage.zig received the same allocator-sensitive finalize change, so a regression there would still pass this suite. Please add an analogous Bun.build()/bundler-error stress case that forces BuildMessage finalization too.

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

In `@test/js/bun/resolve/resolve-error.test.ts` around lines 80 - 111, The test
only covers ResolveMessage/ResolveSync finalization; add a parallel regression
that exercises BuildMessage finalization by creating a Bun.build() stress case
that repeatedly triggers bundler errors (e.g., attempt to build
non-existent/invalid entry points or a small broken bundle) in a loop similar to
the Resolve test, collect thrown errors, touch their properties (message, code,
specifier/referrer-like fields, level, position, String(e)), force Bun.gc(true)
between iterations, and assert pass; this will exercise the BuildMessage path
(corresponding to BuildMessage/BuildMessage.zig) and catch allocator mismatches.

});

// These tests reproduce panics where the module resolver wrote past fixed-size
Expand Down
Loading