Repository navigation
Conversation
… expansion `$.braces()` and brace expansion in a script created a `bun_alloc::Arena` per call. An arena is a whole mimalloc heap, and only a nested group used it: a flat expansion paid for a heap create/destroy pair that it never touched, and a nested one for the pages as well. The nested AST was built from arena slices that held `Atom::Text(SmolStr)`. An arena does not run `Drop`, so every atom longer than the 15 inline bytes of a `SmolStr` leaked its heap buffer on each expansion. The AST now owns its slices (`OwnedSlice`, a `Box<[T]>` held as a raw pointer because `expand_nested` writes backrefs into the elements), so `braces::expand` and `braces::Parser` take no arena and the atoms drop.
|
Status Reproduced on main (c6b7fcb) and on 1.3.14 with release builds: import { heapStats } from "bun:jsc";
const before = heapStats().mimalloc.heaps.total;
for (let i = 0; i < 100; i++) Bun.$.braces("a{1,2,3}");
console.log(heapStats().mimalloc.heaps.total - before); // main: 100, this PR: 0
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. WalkthroughBrace parsing and expansion now use heap-owned boxed slices instead of temporary arenas. Runtime callers use the updated interfaces. Tests cover recursive expansion and memory behavior. ChangesBrace expansion ownership
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The ownership migration preserves the documented lifetimes and error boundaries, with no verified behavior regression requiring resolution before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked the OwnedSlice drop path against the bubble_up backrefs in expand_nested: the root Group is moved onto expand's stack frame before any backref is written, Group has no Drop that dereferences bubble_up, and the nested Expansion -> OwnedSlice<Group> -> Atom chain frees recursively without touching the raw parent pointers, so the ownership change does not introduce a use-after-free or double free.
Extended reasoning...
The PR replaces bump-arena slices with a raw-pointer-wrapped Box<[T]> (OwnedSlice) in the brace-expansion AST. The one verified finding is a style nit (open-coded Box::into_raw/Box::from_raw instead of bun_core::heap). I independently traced the unsafe interplay: parse() returns the root Group by value and expand holds it on its own frame before expand_nested writes bubble_up pointers into child groups, so no Group moves after a backref points at it; OwnedSlice::drop only frees its own boxed slice and the contained Group/Atom values drop their own OwnedSlice/SmolStr fields recursively, never following bubble_up. The remaining ruled-out candidates (ASAN lane timing for the RSS test, Miri aliasing on as_ptr, debug-lane coverage of the leak test) concern test infrastructure rather than correctness of the fix.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the updated push and found no bugs; the earlier Box::into_raw/Box::from_raw nit is addressed in fc1f031 (bun_core::heap::into_raw/destroy). Because the change re-homes ownership of an AST that expand_nested still mutates through raw bubble_up backrefs, a human look at the unsafe contract is still worthwhile.
What was reviewed:
- Drop chain
OwnedSlice<Group>->Group->GroupAtoms::Many->Atom::Expansion->OwnedSlice<Group>: each box freed exactly once, noClone/Copyon any AST type, recursion depth bounded byMAX_BRACE_GROUPS(256). - Parser error paths (
parse/parse_expansionreturningErrmid-build): partially builtVecs drop normally before anyOwnedSlice::new, so nothing is orphaned as a raw pointer. - Call sites:
BunObject.rsandExpansion.rsare the only callers ofParser::init/expand; both updated, no stale arena argument remains. - Not verified here: the
heapStats().mimalloc.heaps.totalfield the new heap-count test reads comes frommi_stats_get_json, and the vendored mimalloc source is not in this checkout, so I could not confirm the key name from source.
Extended reasoning...
Overview
The PR removes the per-call bun_alloc::Arena (a full mimalloc heap) from brace expansion in src/runtime/api/BunObject.rs and src/runtime/shell/states/Expansion.rs, and replaces the arena-owned *mut [Atom]/*mut [Group] slices in src/shell_parser/braces.rs with a small ast::OwnedSlice<T> wrapper (a Box<[T]> stored raw, freed in Drop via bun_core::heap::destroy). Parser::init and expand lose the &Bump parameter; parse/parse_expansion build plain Vecs and convert with OwnedSlice::new. The expand_nested recursion is unchanged apart from reading slices through as_ptr(). Two Rust unit tests and two JS tests (heap-count and RSS-delta) were added.
Security risks
None specific to this change. Input is bounded by the pre-existing check_brace_group_count (MAX_BRACE_GROUPS = 256), which also caps the recursive Drop depth of the nested AST. No new parsing of untrusted data was introduced; the lexer and expansion arithmetic are untouched.
Level of scrutiny
Moderate-to-high: this is unsafe Rust in the "most-blocked category" of the repo's review guidance. The ownership move is small and self-contained, and I checked the points that matter for it: OwnedSlice has no Clone/Copy so no double-free is reachable; every OwnedSlice::new is reached only after the Vec is fully built (all ? propagations occur before the conversion, so error paths drop plain Vecs); as_ptr() returns the stored raw pointer with its original provenance and no &mut to the slice is ever materialized; the aliasing pattern in expand_nested (writes only into child Groups while a parent is reached via raw pointer) is unchanged from the arena version. The previously raised nit about using bun_core::heap::{into_raw, destroy} instead of Box::into_raw/Box::from_raw was addressed in the follow-up commit. Because the remaining risk is the raw-pointer aliasing contract that I can only verify by reading (the PR states Miri passes on the new unit tests, which I could not run here), a human confirmation is still worthwhile rather than an unattended approve.
Other factors
All callers of Parser::init/braces::expand were updated (grep found only the two runtime sites). The RSS test uses the existing expectRssDeltaBelow harness helper, uses Buffer.alloc instead of "x".repeat, skips on debug with a stated reason, and uses test.skipIf(...).concurrent, a chain already used elsewhere in the suite. The one thing I could not confirm from source is the mimalloc.heaps.total key that the heap-count test reads: heapStats parses mi_stats_get_json output in src/jsc/modules/BunJSCModule.h, but the vendored mimalloc tree is absent from this shallow checkout and no other test references that key, so its existence rests on the author's statement that the test fails on main and passes on the branch.
There was a problem hiding this comment.
I re-reviewed the latest push (fc1f0314, d3d6f45d) and found no bugs; the earlier bun_core::heap nit is addressed. Because this changes who owns and frees the brace AST behind raw pointers, a human look at the unsafe ownership model is still worthwhile.
What was reviewed:
OwnedSlice<T>lifecycle: created once fromVec::into_boxed_slice, freed once inDrop;Group/Atom/Expansionderive neitherClonenorCopy, so no duplicate owner can arise.bubble_upbackrefs: they remain non-owning raw pointers into boxed slices (stable addresses) or the stack root, so recursiveDropof the tree cannot double-free through them.- Callers in
BunObject.rsandExpansion.rs: only the arena argument was removed; error mapping and theopts.parsepath are unchanged. - Tests: the heap-count check and the release-only RSS test both depend on the new ownership (bound 16 MiB vs. ~37 MiB unfixed); the Rust unit tests use >15-byte atoms so Miri would flag a leak.
Extended reasoning...
Overview
The PR removes the per-call bun_alloc::Arena from the shell brace-expansion parser and replaces bump-owned AST slices with ast::OwnedSlice<T>, a Box<[T]> held as a raw pointer with a Drop impl that frees it via bun_core::heap::destroy. Parser::init and expand lose their bump parameter; the two callers (Bun.$.braces in src/runtime/api/BunObject.rs and the shell Expansion state in src/runtime/shell/states/Expansion.rs) drop their arena creation. Tests are added in test/js/bun/shell/brace.test.ts (heap count, release-only RSS delta) and as Rust unit tests in src/shell_parser/braces.rs.
Security risks
None specific to this change. Input is still bounded by check_brace_group_count / MAX_BRACE_GROUPS before the AST is built, so recursive Drop depth is bounded the same way recursion in expand_nested already was. No new parsing of untrusted data, no new allocation sized from user-controlled counts beyond what the previous arena path already did. The memory-safety surface is the raw-pointer ownership: I checked that OwnedSlice is neither Copy nor Clone (it has a Drop impl, and the containing AST types derive nothing), as_ptr only hands out a borrowed view, and bubble_up pointers target boxed-slice elements or the stack-resident root, both of which are address-stable for the duration of expand_nested.
Level of scrutiny
Medium. The diff is small and the ownership model is simple (one owner per slice, freed in Drop), but it is unsafe Rust that rewrote how the AST is freed, and the expand_nested recursion mutates child Groups through raw pointers while the parent is borrowed. The bug hunt ran to a dry streak without findings and my own reading found no double-free, use-after-free, or leak path, but a human familiar with the shell parser should confirm the ownership story matches their mental model before merge.
Other factors
The previous run's only comment (use bun_core::heap::{into_raw, destroy} instead of Box::into_raw/Box::from_raw) was addressed in fc1f0314, and d3d6f45d trimmed comments. The SAFETY comments were updated to describe box ownership instead of the arena. Test coverage is adequate: the heap-count test runs on all builds and the RSS test is skipped on debug with a stated reason and a threshold well below the unfixed leak; the Rust unit tests exercise both flat and nested expansion with heap-backed SmolStr atoms so Miri can catch a leak. No outstanding third-party objections appear in the timeline metadata.
|
Updated 8:10 AM PT - Sep 16th, 2026
✅ @robobun, your commit 5932fe8e321bc31df3a90161e3124cf9bf3cdb81 passed in 🧪 To try this PR locally: bunx bun-pr 42927That installs a local version of the PR into your bun-42927 --bun |
This branch has not been deployed
2 participants
Problem
$.braces("a{1,2,3}")costs 3x what it cost in 1.3.14 (612 ns, now 1869 ns). A nested group costs 8x (687 ns, now 5747 ns).bun_alloc::Arenais a whole mimalloc heap.$.bracescreates one per call (src/runtime/api/BunObject.rs:397). A script creates one per word that expands (src/runtime/shell/states/Expansion.rs:323). Only a nested group uses it.Atom::Text(SmolStr)in arena slices, and an arena does not runDrop.Fix
OwnedSlice<T>is aBox<[T]>held as a raw pointer, and itsDropfrees the box.braces::expandandbraces::Parsertake no arena.expand_nestedwritesbubble_upbackrefs into child groups while it reads the parent that owns them. That access pattern does not change.test/js/bun/shell/brace.test.tsfail on main (200 heaps for 200 calls, 40 MiB of RSS growth). New unit tests inbraces.rspass under Miri.Background
mi_heap_new,mi_heap_destroy) frees all of its blocks at destroy. One pair costs a thread-local slot, fresh 64 KiB pages when the heap is used, and up to four merges of the heap statistics.SmolStrstores up to 15 bytes inline. A longer string is a heap allocation thatDropfrees.Group,Atom,Expansion).Notes
Numbers
Release builds of main (c6b7fcb) and this branch, same toolchain, plus the 1.3.14 release binary. Median of 3 interleaved runs, in ns per call. The machine is a shared container.
$.braces("abc")$.braces("a{1,2,3}")$.braces("x{1,2}{a,b}")$.braces("{a,{b,c}}")$.escape("a b'c" + i)+$.braces("a{1,2,3}")The Zig code used
std.heap.ArenaAllocatorhere, which costs nothing until it is used. In a sampling profile of$.braces("abc")on main, 72% of the samples are heap create and destroy, andmi_stats_addalone is 47%.The leak
Parser::advanceclones each text token into the AST. The clone of a long atom owns a heap buffer, and the arena slice that held it was freed withoutDrop. 20 000 calls of$.braces("{" + "x".repeat(4000) + ",{b,c}}")grow RSS by 90 MB on main (20 000 x 4000 bytes) and by 7 MB here. A script leaks the same way: 1000 runs of$`echo {${long},{b,c}}`with a 64 KiBlonggrow RSS by 63 MB on main and by 3 MB here.The new unit test
expand_nested_groupsuses atoms longer than 15 bytes.bun_shell_parseris in the Miri crate set (scripts/rust-miri.ts). With theDropofOwnedSlicedisabled, Miri reports each leaked allocation. Before this change the nested path needed a mimalloc heap, which Miri cannot call, so it had no unit test.The RSS test
a nested expansion frees its atomsskips on a debug build. The brace lexer handles 256 KiB in about 100 ms there, so the test takes 14 s. On a release build it takes 0.3 s. The heap count test runs on every build.Related
Bun.$script creates two mimalloc heaps for its parse arena. The two PRs share no file.braces.rs. They touch the lexer and$.bracesquoting, not the AST ownership.Other test results
Ran
brace.test.ts,bunshell.test.ts,bunshell-default.test.ts,lex.test.tsandparse.test.tswith the debug ASAN build: 543 pass, 0 fail.cargo test -p bun_shell_parser,bun run rust:miri -p bun_shell_parser, clippy andtest/internal/source-lintspass.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file