ResolveMessage/BuildMessage: match allocator in finalize and destroy struct#29840
Conversation
…struct
`ResolveMessage.create()` and `BuildMessage.create()` allocate both the
wrapper struct and the cloned `logger.Msg` with the caller-supplied
allocator (the VM's MimallocArena in practice), but `finalize()` was:
* freeing the cloned msg with the hard-coded `bun.default_allocator`
instead of `this.allocator`, and
* never calling `allocator.destroy(this)`, leaking every struct.
Under ASAN with mimalloc's per-heap tracking this surfaced as a flaky
use-after-poison in the resolver after many failed `require()` + GC
cycles in a long-running process (Fuzzilli REPRL). Free with the same
allocator that was used to create, and destroy the struct.
|
Updated 11:16 AM PT - Apr 28th, 2026
❌ @robobun, your commit 3921990 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29840That installs a local version of the PR into your bun-29840 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFinalize implementations for BuildMessage and ResolveMessage now free and destroy using each instance's allocator; two regression tests were added to stress allocator-consistent finalization under GC for build and resolve failures. ini error conversion now uses Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/resolve/resolve-error.test.ts`:
- Around line 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.
🪄 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: 73005ac0-a54a-4fe2-a898-7c7e3574b164
📒 Files selected for processing (3)
src/bun.js/BuildMessage.zigsrc/bun.js/ResolveMessage.zigtest/js/bun/resolve/resolve-error.test.ts
| 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(); | ||
| }); |
There was a problem hiding this comment.
🧹 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.
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/resolve/build-error.test.ts`:
- Around line 29-32: The test currently iterates r.logs without ensuring it's
non-empty, so add an explicit assertion that r.logs contains at least one
message before consuming them; specifically, after calling Bun.build (the
variable r) and before the for loop over r.logs (and before any handling of
BuildMessage), assert r.logs.length > 0 (e.g.,
expect(r.logs.length).toBeGreaterThan(0) or toHaveLength) so the test always
exercises the finalize/consumption path for BuildMessage.
🪄 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: 62f10bbf-5c44-472d-8110-5a80ea048965
📒 Files selected for processing (1)
test/js/bun/resolve/build-error.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
BuildMessage/ResolveMessage store the allocator they are created with and dereference it in finalize() during GC. The npmrc test helper was passing a stack-local ArenaAllocator that is deinit()'d before the function returns, so finalize would run with both `this` (allocated inside the arena) and `this.allocator.ptr` dangling. Pre-existing, but the new `this.allocator.destroy(this)` in finalize made it a guaranteed call through a dead vtable pointer rather than just a read of freed arena memory. Also assert the BuildMessage stress test actually produces logs.
| this.msg.deinit(this.allocator); | ||
| this.allocator.destroy(this); |
There was a problem hiding this comment.
🟣 🟣 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)
- The new test parses
function bad( {→ the bundler emits at least one parse-errorBuildMessagewhosemsg.data.locationis non-null (parse errors carryfile+line_text). BuildMessage.create()callsmsg.clone(allocator)(BuildMessage.zig:54) →Msg.clone(logger.zig:437) →Data.clone(logger.zig:230) →Location.clone(logger.zig:113), whichallocator.dupesfileandline_text.Bun.gc(true)sweeps the wrapper →finalize()callsthis.msg.deinit(this.allocator).Msg.deinit→Data.deinitcallsloc.deinit(allocator)(no-op) thenallocator.free(d.text). The dupedfileandline_textare never passed toallocator.free.- Result: ~2 string allocations leak per
BuildMessageper iteration × 20 iterations.
Step-by-step proof (leak 2)
- The new test reads
void e.notes;on eachBuildMessage. - For each entry in
this.msg.notes,getNotescallsnote.clone(bun.default_allocator)→Data.clonedupes.text(andLocation.clonedupesfile/line_textif present). Call this allocation set A. - The wrapped
Msg{ .data = cloned }is passed toBuildMessage.create(), which callsmsg.clone(allocator)→Data.cloneagain, producing allocation set B stored in the childBuildMessage. - When the child is GC'd,
finalize()frees B. A is never referenced again and leaks per note per.notesaccess.
(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 }),
);
}
There was a problem hiding this comment.
LGTM — alloc/free are now symmetric and the one short-lived-allocator caller (ini.zig) is fixed.
Extended reasoning...
Overview
Three-line core change: finalize() in BuildMessage and ResolveMessage now calls this.msg.deinit(this.allocator) (instead of hardcoded bun.default_allocator) and adds this.allocator.destroy(this) to free the wrapper struct that create() allocates with allocator.create(). A one-line follow-up in src/ini.zig switches the test-only loadNpmrcFromJS error path from a stack-local arena to bun.default_allocator so the stored allocator outlives GC. Two new stress tests exercise ~500 ResolveMessage and ~20 Bun.build BuildMessage create→GC cycles while touching every lazy getter.
Security risks
None. This is allocator-pairing in error-object finalizers; no auth, crypto, parsing, or user-controlled data flow is touched.
Level of scrutiny
Memory management in a hot path (every failed require/import/Bun.resolveSync), so worth careful review — but the change is the textbook fix: pair allocator.create() with allocator.destroy() and free the cloned msg with the same allocator that cloned it. I audited every BuildMessage.create / ResolveMessage.create and log.toJS call site; all pass either bun.default_allocator or globalThis.allocator() / VirtualMachine.get().allocator, which outlive GC. The single offender (ini.zig stack-local arena) is fixed in this PR.
Other factors
All prior review feedback has been addressed: CodeRabbit's BuildMessage stress test + non-empty-logs assertion, and my ini.zig allocator-lifetime note. My remaining inline comment about Location.deinit / getNotes double-clone is explicitly pre-existing and non-blocking. The musl build-bun CI failures are build-step infra failures, not test failures. The new tests give reasonable regression coverage under ASAN.
Jarred-Sumner
deleted the
farm/777d8566/resolve-message-finalize-allocator
branch
…struct (oven-sh#29840) `ResolveMessage.create()` and `BuildMessage.create()` allocate both the wrapper struct and the cloned `logger.Msg` with the caller-supplied allocator — `VirtualMachine.get().allocator` (a `MimallocArena`) in the resolve path — and store it in `this.allocator`. But `finalize()` was: * freeing the cloned msg with the hard-coded `bun.default_allocator` instead of `this.allocator`, and * never calling `allocator.destroy(this)`, leaking every struct (one per failed `require()` / `import` / `Bun.resolveSync`). Under ASAN with mimalloc's per-heap tracking (`MI_TRACK_ASAN=1`) this surfaced as a flaky `use-after-poison` in the resolver after many failed `require()` + `Bun.gc(true)` cycles accumulating in a long-running Fuzzilli REPRL process: ``` ==89334==ERROR: AddressSanitizer: use-after-poison on address 0x7296d93a0020 READ of size 12 at 0x7296d93a0020 thread T0 #0 in __interceptor_memcpy ... oven-sh#25 in functionImportMeta__resolveSync* ``` ```js try { this.require("resolvedOptions"); } catch (e) {} Bun.gc(true); ``` Free with the same allocator that was used to create, and destroy the struct. Adds a stress test that creates and GCs ~500 `ResolveMessage` objects while reading every lazy getter. --------- Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
ResolveMessage.create()andBuildMessage.create()allocate both the wrapper struct and the clonedlogger.Msgwith the caller-supplied allocator —VirtualMachine.get().allocator(aMimallocArena) in the resolve path — and store it inthis.allocator. Butfinalize()was:bun.default_allocatorinstead ofthis.allocator, andallocator.destroy(this), leaking every struct (one per failedrequire()/import/Bun.resolveSync).Under ASAN with mimalloc's per-heap tracking (
MI_TRACK_ASAN=1) this surfaced as a flakyuse-after-poisonin the resolver after many failedrequire()+Bun.gc(true)cycles accumulating in a long-running Fuzzilli REPRL process:Free with the same allocator that was used to create, and destroy the struct.
Adds a stress test that creates and GCs ~500
ResolveMessageobjects while reading every lazy getter.