resolver: free formatted error text after ResolveMessage.create clones it#30292
resolver: free formatted error text after ResolveMessage.create clones it#30292robobun wants to merge 1 commit into
Conversation
…ones it resolveMaybeNeedsTrailingSlash allocPrints the "Cannot find package '...' from '...'" text into the VM arena, then passes it to ResolveMessage.create which immediately msg.clone()s it into a second arena allocation owned by the ResolveMessage. The original was never freed, so every failed require()/import of a bare specifier leaked one string into the same mimalloc heap that ResolveMessage structs are destroyed back into on GC. Same pattern for the NameTooLong fast path (bun.default_allocator) and the empty-log branch of processFetchLog. In a long-running Fuzzilli REPRL process this accumulated across thousands of iterations and surfaced as an intermittent use-after-poison (12-byte read in a poisoned ~280-byte arena block) with the same signature as the issue addressed in 9a2997b. Also switch the create() call to use the already-bound jsc_vm.allocator instead of re-fetching VirtualMachine.get().allocator.
|
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)
WalkthroughThis pull request fixes memory leaks in error message formatting within the resolve functionality. The VirtualMachine.zig file now properly frees formatted message buffers in error paths and refactors text allocation in ChangesMemory Leak Fixes in Resolve Error Paths
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward leak fix; verified that ResolveMessage/BuildMessage.create clone the message text and BabyString metadata is offset-based, so the deferred frees are safe.
Extended reasoning...
Overview
This PR fixes three small memory leaks in src/jsc/VirtualMachine.zig where error-message strings were allocPrinted, passed to ResolveMessage.create / BuildMessage.create (which immediately msg.clone() them), and then never freed. The fix adds defer ... free() for each, plus a matching stress test in resolve-error.test.ts. It also replaces a redundant VirtualMachine.get().allocator with the already-bound jsc_vm.allocator.
Correctness verification
I checked ResolveMessage.create (src/jsc/ResolveMessage.zig:174) and BuildMessage.create (src/jsc/BuildMessage.zig:54) — both call msg.clone(allocator). Msg.clone → Data.clone does allocator.dupe(u8, this.text) (src/logger/logger.zig:232), so the original buffer is no longer referenced after create() returns. The .metadata field is shallow-copied, but BabyString is an offset+len pair (src/logger/logger.zig:371), not a pointer, so it remains valid against the duped text. The printed.len > 0 guard correctly avoids freeing the "" literal when the log already contained a resolve message, and each free is paired with the same allocator used for the corresponding allocPrint/fmt.
Security risks
None. This is purely allocator hygiene on an error path; no user-controlled data flow, parsing, or boundary changes.
Level of scrutiny
Low-to-medium. The change is ~20 lines, mechanical, and mirrors the pattern from #29840 in the same file. The added test follows the existing adjacent test exactly.
Other factors
No prior reviews or comments on the PR. No bugs flagged by the bug-hunting system. The processFetchLog refactor is a pure restructuring (labeled-block → if-expression) with identical semantics plus the new defer free.
|
CI failure is the pre-existing flaky
|
|
Updated 5:39 PM PT - May 5th, 2026
❌ @robobun, your commit 5607d4f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 30292That installs a local version of the PR into your bun-30292 --bun |
1 participant
What
resolveMaybeNeedsTrailingSlashallocPrints the"Cannot find package '...' from '...'"text into the VM'sMimallocArena, then hands it toResolveMessage.create, which immediatelymsg.clone()s it into a second arena allocation that theResolveMessageowns and frees infinalize(). The original allocation was never freed, so every failedrequire()/importof a bare specifier leaked one string into the same mimalloc heap thatResolveMessagestructs are destroyed back into on GC.Same pattern fixed in:
NameTooLongfast path (leaked intobun.default_allocator)processFetchLog(leaked intoglobalThis.allocator())Also switched the
create()call to use the already-boundjsc_vm.allocatorinstead of re-fetchingVirtualMachine.get().allocator— they are the same heap, but the local is already in hand.Why
Fuzzilli hit a very-flaky
use-after-poison(12-byte read inside a poisoned ~280-byte arena block, same__interceptor_memcpy→ … →functionImportMeta__resolveSyncshape as the crash addressed by #29840) with:run thousands of times in the REPRL process. I was unable to reproduce the poison hit deterministically (20,000+ REPRL iterations clean), but the leaked
printedstring is the one remaining allocation in this path that accumulates unbounded in the exact arena the crash points into, so it is the most plausible remaining contributor — and a real leak regardless.Test
Added a bare-specifier stress to
resolve-error.test.tsalongside the existing relative-specifier one from #29840.