glob: fix allocator mismatch in finalize and remove dead code - #29381
Conversation
The Glob struct and its pattern string were allocated with bun.default_allocator but freed with VirtualMachine.get().allocator (a mimalloc arena). This works today only because mimalloc's mi_free ignores heap ownership, but is a contract violation that breaks under heap_breakdown or future allocator changes. - Use bun.new/bun.destroy for the Glob struct (canonical helpers). - Free pattern with bun.default_allocator to match toSliceClone. - Drop the unnecessary @constcast. - Remove dead pattern_codepoints field and convertUtf8 function (no callers anywhere in the tree). - Fix debugPatternComopnents typo.
|
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)
WalkthroughRemoved precomputed codepoint tracking from the Glob struct, refactored memory allocation in the constructor to use Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@robobun adopt |
|
✅ Adopted. Builds green on all platforms, no glob test failures. Remaining CI red is pre-existing on
Ready to merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct allocator pairing, clean dead-code removal, and trivial typo fix.
Extended reasoning...
Overview
Two files changed: src/bun.js/api/glob.zig (constructor/finalize logic) and src/glob/GlobWalker.zig (debug helper rename). The PR fixes an allocator mismatch, removes unreachable code, drops a redundant cast, and fixes a function-name typo.
Security risks
None. No auth, crypto, or permission-related code is touched. The allocator fix closes a latent memory-safety issue (use-after-free under heap_breakdown or alternative allocator configurations), making the code more correct, not less safe.
Level of scrutiny
Low. Each change is a mechanical fix with a clear 1:1 mapping between cause and remedy:
bun.new/bun.destroy(backed bybun.default_allocator) replaces the mismatchedalloc.create/VirtualMachine.get().allocator.destroypair.bun.default_allocator.free(this.pattern)matches the allocator used bytoSliceClone.@constCastremoval follows naturally from changing the slice type to[]const u8.pattern_codepointsfield andconvertUtf8function had zero callers and are safely deleted.- The typo rename updates both the definition and its only call site.
Other factors
No existing tests are affected by the dead-code removal. The function rename (debugPatternComopnents → debugPatternComponents) is guarded by bun.Environment.allow_assert so it only runs in debug builds. CI is already running on the commit.
…h#29381) The `Glob` struct and its pattern string were allocated with `bun.default_allocator` but freed with `VirtualMachine.get().allocator` (a mimalloc arena). This works today only because mimalloc's `mi_free` ignores heap ownership, but is a contract violation that breaks under `heap_breakdown` or future allocator changes. - Use `bun.new`/`bun.destroy` for the `Glob` struct. - Free pattern with `bun.default_allocator` to match `toSliceClone`. - Drop the unnecessary `@constCast`. - Remove dead `pattern_codepoints` field and `convertUtf8` function (no callers anywhere in the tree). - Fix `debugPatternComopnents` typo.
…h#29381) The `Glob` struct and its pattern string were allocated with `bun.default_allocator` but freed with `VirtualMachine.get().allocator` (a mimalloc arena). This works today only because mimalloc's `mi_free` ignores heap ownership, but is a contract violation that breaks under `heap_breakdown` or future allocator changes. - Use `bun.new`/`bun.destroy` for the `Glob` struct. - Free pattern with `bun.default_allocator` to match `toSliceClone`. - Drop the unnecessary `@constCast`. - Remove dead `pattern_codepoints` field and `convertUtf8` function (no callers anywhere in the tree). - Fix `debugPatternComopnents` typo.
3 participants
The
Globstruct and its pattern string were allocated withbun.default_allocatorbut freed withVirtualMachine.get().allocator(a mimalloc arena). This works today only because mimalloc'smi_freeignores heap ownership, but is a contract violation that breaks underheap_breakdownor future allocator changes.bun.new/bun.destroyfor theGlobstruct.bun.default_allocatorto matchtoSliceClone.@constCast.pattern_codepointsfield andconvertUtf8function (no callers anywhere in the tree).debugPatternComopnentstypo.