Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
21 changes: 13 additions & 8 deletions scripts/build/deps/mimalloc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import type { Dependency, NestedCmakeBuild, Provides } from "../source.ts";

const MIMALLOC_COMMIT = "1beadf9651a7bfdec6b5367c380ecc3fe1c40d1a";
const MIMALLOC_COMMIT = "9a5e1f52cdf4662f9590b69de104a4469140796f";

export const mimalloc: Dependency = {
name: "mimalloc",
Expand Down Expand Up @@ -68,8 +68,13 @@ export const mimalloc: Dependency = {
// so UBSan doesn't false-positive on mimalloc's type punning.
args.MI_DEBUG_UBSAN = "ON";
} else if (cfg.darwin) {
args.MI_OVERRIDE = "OFF";
args.MI_OSX_ZONE = "OFF";
// Register the mimalloc malloc_zone so leaks/heap/vmmap/Instruments can
// enumerate our allocations and the fork-prepare/child hooks fire.
// Interpose is OFF because we link the .o statically; the __interpose
// section only works for inserted dylibs. The zone-swap path in
// alloc-override-zone.c handles making mimalloc the default zone.
args.MI_OVERRIDE = "ON";
args.MI_OSX_ZONE = "ON";
Comment on lines +71 to +77
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.

⚠️ Potential issue | 🟡 Minor

Update the earlier macOS decision-matrix comment to match the new behavior.

This block now enables MI_OVERRIDE/MI_OSX_ZONE on Darwin, but the matrix comment above still says macOS override is OFF. That contradiction is easy to misread during future changes.

✏️ Proposed comment-only fix
-    //   macOS: OFF — macOS's malloc zones are sufficient and overriding
-    //          causes issues with system frameworks (SecureTransport, etc.)
-    //          that have their own allocator expectations.
+    //   macOS: ON — register mimalloc's malloc zone so tools can enumerate
+    //          allocations and fork hooks run; keep MI_OSX_INTERPOSE=OFF
+    //          because we're linking statically.
As per coding guidelines: "Be humble and honest - never overstate what you got done or what actually works in commits, PRs, or messages to the user."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/build/deps/mimalloc.ts` around lines 71 - 77, The top-level macOS
decision-matrix comment is now outdated: the code sets args.MI_OVERRIDE = "ON"
and args.MI_OSX_ZONE = "ON" for Darwin, but the comment still states the macOS
override is OFF; update that earlier comment to reflect the current behavior
(mention MI_OVERRIDE and MI_OSX_ZONE are enabled on Darwin and note
alloc-override-zone.c handles zone-swap) so readers aren’t confused when looking
at the args.MI_OVERRIDE / args.MI_OSX_ZONE settings.

args.MI_OSX_INTERPOSE = "OFF";
} else if (cfg.linux) {
args.MI_OVERRIDE = "ON";
Comment on lines 68 to 80
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.

🟡 The 'Override behavior' decision matrix comment in scripts/build/deps/mimalloc.ts still reads 'macOS: OFF — macOS's malloc zones are sufficient and overriding causes issues with system frameworks' but this PR changed the darwin block to set MI_OVERRIDE = "ON" and MI_OSX_ZONE = "ON". The decision matrix comment was not updated to reflect the new behavior and now directly contradicts the code.

Extended reasoning...

What the bug is

The decision matrix comment block (lines ~53–58) in scripts/build/deps/mimalloc.ts explicitly states:

//   macOS: OFF — macOS's malloc zones are sufficient and overriding
//          causes issues with system frameworks (SecureTransport, etc.)
//          that have their own allocator expectations.

However, this PR changed the darwin branch of the build configuration to:

args.MI_OVERRIDE = "ON";
args.MI_OSX_ZONE = "ON";

The decision matrix was not updated when the behavior changed.

The specific code path

The decision matrix at the top of the override behavior section is intended as the authoritative, at-a-glance summary of per-platform override decisions. The darwin block below it does include a new inline comment explaining the new rationale (zone registration for leaks/vmmap/Instruments and fork hooks), but the summary matrix above it still says OFF, creating a direct contradiction.

Why existing code doesn't prevent it

This is a documentation-only inconsistency with no runtime impact. There is no automated check that validates the decision matrix comment against the actual CMake argument values. The inline comment in the darwin block partially addresses this, but the authoritative matrix at the top of the section remains wrong.

Impact

A developer reading the decision matrix to understand platform behavior would conclude macOS overriding is OFF and rely on that understanding when reasoning about allocator behavior, debugging macOS-specific issues, or reviewing future changes. The actual runtime behavior is the opposite of what the matrix says. This discrepancy is particularly misleading because the old rationale ("causes issues with system frameworks") was the reason for the previous OFF setting — a future developer might even revert the change citing that comment as justification.

How to fix it

Update the macOS line in the decision matrix to reflect the new ON behavior and the new rationale, e.g.:

//   macOS: ON  — MI_OSX_ZONE registers the mimalloc malloc_zone so
//          leaks/heap/vmmap/Instruments can enumerate allocations and
//          fork-prepare/child hooks fire. Interpose remains OFF (static link).

Step-by-step proof

  1. Open scripts/build/deps/mimalloc.ts and read lines ~53–58: the matrix says macOS: OFF.
  2. Read the darwin block (lines ~68–78): args.MI_OVERRIDE = "ON" and args.MI_OSX_ZONE = "ON".
  3. The matrix says OFF; the code sets ON. Direct contradiction.

Expand All @@ -91,10 +96,10 @@ export const mimalloc: Dependency = {
args.MI_TRACK_VALGRIND = "ON";
}

// If mimalloc gets bumped to a version with MI_OPT_ARCH: pass
// MI_NO_OPT_ARCH=ON to stop it setting -march=armv8.1-a on arm64
// (SIGILLs on ARMv8.0 CPUs). Current pin has no arch-detection logic
// so our global -march=armv8-a+crc (via CMAKE_CXX_FLAGS) is sufficient.
// dev3 grew MI_OPT_ARCH which sets -march=armv8.1-a on arm64 — that
// SIGILLs on ARMv8.0 CPUs. Explicitly disable it; our global
// -march=armv8-a+crc (via CMAKE_CXX_FLAGS) is sufficient.
args.MI_NO_OPT_ARCH = "ON";

// ─── Windows: silence the vendored-C-as-C++ warning flood ───
// MI_USE_CXX=ON means .c files compile as C++. clang-cl then complains
Expand All @@ -121,7 +126,7 @@ export const mimalloc: Dependency = {
// to override this, so we have to mirror its naming logic.
let libname: string;
if (cfg.windows) {
libname = cfg.debug ? "mimalloc-static-debug" : "mimalloc-static";
libname = cfg.debug ? "mimalloc-debug" : "mimalloc";
Comment on lines 128 to +129
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.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check mimalloc CMakeLists.txt for static library naming logic

# Search for library naming patterns in mimalloc's CMake configuration
rg -n -C5 'mimalloc-static|OUTPUT_NAME|set_target_properties.*mimalloc' --type cmake

Repository: oven-sh/bun

Length of output: 37


🏁 Script executed:

cat -n scripts/build/deps/mimalloc.ts | head -50

Repository: oven-sh/bun

Length of output: 2108


🌐 Web query:

mimalloc v3 Windows static library names CMake MI_BUILD_STATIC

💡 Result:

For mimalloc v3 on Windows using CMake with MI_BUILD_STATIC enabled, the static library target is named "mimalloc-static". The output library file is mimalloc-static.lib (on Windows) or libmimalloc-static.a (on Unix-like systems). The CMake option MI_BUILD_STATIC (default ON) controls whether the static library is built: option(MI_BUILD_STATIC "Build static library" ON). To link, use target_link_libraries(myapp PUBLIC mimalloc-static) after find_package(mimalloc CONFIG REQUIRED). This applies to v3.x releases (e.g., v3.2.8 as of 2026), consistent with v2 behavior. Note that Visual Studio .sln builds mimalloc-lib.lib directly, but CMake uses the -static suffix to avoid naming conflicts with DLL import libs.

Citations:


🏁 Script executed:

sed -n '115,130p' scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 565


🏁 Script executed:

sed -n '115,145p' scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 1453


🏁 Script executed:

git log --oneline -20 --all -- scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 213


🏁 Script executed:

git show HEAD:scripts/build/deps/mimalloc.ts | sed -n '115,130p'

Repository: oven-sh/bun

Length of output: 565


🏁 Script executed:

git show f91d3cf9:scripts/build/deps/mimalloc.ts | sed -n '115,130p'

Repository: oven-sh/bun

Length of output: 565


🏁 Script executed:

git diff a8ee0d38^..a8ee0d38 -- scripts/build/deps/mimalloc.ts | head -60

Repository: oven-sh/bun

Length of output: 2100


🏁 Script executed:

git show a8ee0d38 | grep -A 5 -B 5 "mimalloc-debug\|mimalloc-static" | head -50

Repository: oven-sh/bun

Length of output: 950


🏁 Script executed:

# Check if there are any build/cmake files that reference the old naming
rg -n "mimalloc-static-debug|mimalloc-static" scripts/build/ || echo "No matches found"

Repository: oven-sh/bun

Length of output: 141


🏁 Script executed:

# Check the current HEAD to see both the targets and the libname usage
sed -n '100,130p' scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 1146


🏁 Script executed:

# Check what the actual library output files should be named
# by looking at how libname is used in the provides function
sed -n '115,160p' scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 1708


🏁 Script executed:

# Check the commit that changed the Windows library naming
git log --all --oneline --grep="mimalloc" -- scripts/build/deps/mimalloc.ts | head -5

Repository: oven-sh/bun

Length of output: 158


🏁 Script executed:

# Look for the actual commit that changed from mimalloc-static to mimalloc
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -A 10 -B 10 "mimalloc-debug.*mimalloc" | head -60

Repository: oven-sh/bun

Length of output: 37


🏁 Script executed:

git show f91d3cf9 --stat | head -20

Repository: oven-sh/bun

Length of output: 1133


🏁 Script executed:

git show f91d3cf9 -- scripts/build/deps/mimalloc.ts | head -100

Repository: oven-sh/bun

Length of output: 3941


🏁 Script executed:

# Check if there was a previous version of this file in CMake form
git log --all --oneline -- cmake/targets/BuildBun.cmake | head -5

Repository: oven-sh/bun

Length of output: 332


🏁 Script executed:

# Look for any PRs or discussions about the mimalloc library naming change
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -B 20 "mimalloc-debug\|mimalloc-static" | grep -A 5 -B 5 "Windows" | head -40

Repository: oven-sh/bun

Length of output: 37


🏁 Script executed:

# Find commits that changed the Windows libname from mimalloc-static to mimalloc
git log -p --all -- scripts/build/deps/mimalloc.ts | grep -B 5 -A 5 'libname = cfg.debug ? "mimalloc' | head -50

Repository: oven-sh/bun

Length of output: 37


🏁 Script executed:

# Check the commit just before f91d3cf9
git log --oneline -10 f91d3cf9^..HEAD -- scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 94


🏁 Script executed:

# Check the CMake deletion commit to see what it was before
git show ba05a729 -- cmake/targets/BuildBun.cmake | grep -i mimalloc | head -20

Repository: oven-sh/bun

Length of output: 254


🏁 Script executed:

# Look at the commit before ba05a729
git show ba05a729^:cmake/targets/BuildBun.cmake | grep -i mimalloc -A 2 -B 2 | head -40

Repository: oven-sh/bun

Length of output: 560


🏁 Script executed:

# Find all commits between a8ee0d38 and f91d3cf9 that modified mimalloc.ts
git log --oneline a8ee0d38..f91d3cf9 -- scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 158


🏁 Script executed:

# Look at the current HEAD commit message to understand the context
git show --quiet f91d3cf9

Repository: oven-sh/bun

Length of output: 691


🏁 Script executed:

# Check if oven-sh/mimalloc fork has its own CMakeLists that differs from upstream
# by looking at recent changes to the mimalloc fork
cd /tmp && git clone --depth 1 https://github.com/oven-sh/mimalloc.git 2>&1 | grep -i "fatal\|clone" || echo "Clone check"

Repository: oven-sh/bun

Length of output: 66


🏁 Script executed:

# Get commit info for the pinned mimalloc commit
git log --oneline --all | grep "09fed41b" || echo "Not in bun repo"

Repository: oven-sh/bun

Length of output: 70


🏁 Script executed:

# Check the actual cmake/targets/BuildBun.cmake from before deletion to see how mimalloc libs were linked
git show ba05a729^:cmake/targets/BuildBun.cmake | grep -i "mimalloc" -B 3 -A 3

Repository: oven-sh/bun

Length of output: 782


🏁 Script executed:

# Check how libname is used - look for references to it in the provides object
sed -n '115,154p' scripts/build/deps/mimalloc.ts

Repository: oven-sh/bun

Length of output: 1708


🏁 Script executed:

# Look at the git log for the original creation to see if there's context about the naming
git show a8ee0d38:scripts/build/deps/mimalloc.ts | sed -n '100,140p'

Repository: oven-sh/bun

Length of output: 1734


Fix Windows library name mismatch in CMake targets.

The CMake targets still build mimalloc-static (line 110), but the Windows provides() function looks for mimalloc and mimalloc-debug (lines 123–124). This mismatch will cause link failures. Either revert the Windows libname back to mimalloc-static-debug/mimalloc-static to match the targets, or update the targets array to remove the -static suffix to match the libname.

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

In `@scripts/build/deps/mimalloc.ts` around lines 123 - 124, The Windows library
name in provides() (the libname variable set in the provides() function) doesn't
match the CMake targets that build "mimalloc-static" and
"mimalloc-static-debug"; update either the libname values or the targets array
so they match: change libname in provides() to "mimalloc-static" when cfg.debug
is false and "mimalloc-static-debug" when cfg.debug is true, or alternatively
remove the "-static" suffix from the targets array so the built targets are
"mimalloc" / "mimalloc-debug"; locate the provides() function (where libname is
assigned) and the targets array and make them consistent.

} else if (cfg.debug) {
libname = cfg.asan ? "mimalloc-asan-debug" : "mimalloc-debug";
} else {
Expand Down
4 changes: 4 additions & 0 deletions scripts/verify-baseline-static/allowlist-aarch64.txt
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ __aarch64_ldclr1_acq_rel [LSE]
__aarch64_ldclr2_acq_rel [LSE]
__aarch64_ldclr4_acq_rel [LSE]
__aarch64_ldclr8_acq_rel [LSE]
__aarch64_ldclr8_relax [LSE]
Comment on lines 94 to +97
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.

🟡 The section header comment says (41 symbols) but this PR adds 4 new LSE atomic symbols (__aarch64_ldclr8_relax, __aarch64_ldset8_relax, __aarch64_swp8_rel, __aarch64_swp8_relax), bringing the actual count to 45. Update the comment to (45 symbols) to keep it accurate.

Extended reasoning...

What the bug is

The compiler-rt outline atomics section header in scripts/verify-baseline-static/allowlist-aarch64.txt carries the comment (41 symbols). This count was accurate before this PR, but the PR adds 4 new LSE atomic symbols to that section.

The specific code path that triggers it

The diff adds these four entries to the LSE section:

  • __aarch64_ldclr8_relax
  • __aarch64_ldset8_relax
  • __aarch64_swp8_rel
  • __aarch64_swp8_relax

Counting the actual [LSE]-tagged entries after the PR: 13 cas + 9 ldadd + 4 ldclr (incl. new) + 4 ldeor + 5 ldset (incl. new) + 10 swp (incl. 2 new) = 45. The comment was not updated to match.

Why existing code doesn't prevent it

The comment is a human-maintained annotation — there is no automated check that verifies the count in the comment matches the actual number of entries in the section. The allowlist itself works on symbol names, not on the count in the comment.

What the impact would be

Zero functional impact — the allowlist matcher uses symbol names, not the count. However, the comment exists to help maintainers quickly verify the allowlist is complete. A stale count of 41 when there are actually 45 entries could mislead a reviewer into thinking symbols are missing or that the section is incomplete, creating unnecessary confusion during audits.

How to fix it

Change the comment from (41 symbols) to (45 symbols) on the section header line.

Step-by-step proof

  1. Before this PR: the LSE section contained 41 entries; the comment said (41 symbols) — accurate.
  2. This PR adds __aarch64_ldclr8_relax, __aarch64_ldset8_relax, __aarch64_swp8_rel, __aarch64_swp8_relax — 4 new entries.
  3. After the PR: 41 + 4 = 45 entries; the comment still says (41 symbols) — stale.
  4. A maintainer auditing the section would count 45 entries but read 41 in the comment, creating a discrepancy of 4.

__aarch64_ldeor1_acq_rel [LSE]
__aarch64_ldeor2_acq_rel [LSE]
__aarch64_ldeor4_acq_rel [LSE]
Expand All @@ -102,6 +103,7 @@ __aarch64_ldset1_acq_rel [LSE]
__aarch64_ldset2_acq_rel [LSE]
__aarch64_ldset4_acq_rel [LSE]
__aarch64_ldset8_acq_rel [LSE]
__aarch64_ldset8_relax [LSE]
__aarch64_swp1_acq [LSE]
__aarch64_swp1_acq_rel [LSE]
__aarch64_swp1_relax [LSE]
Expand All @@ -110,6 +112,8 @@ __aarch64_swp4_acq [LSE]
__aarch64_swp4_acq_rel [LSE]
__aarch64_swp4_rel [LSE]
__aarch64_swp8_acq_rel [LSE]
__aarch64_swp8_rel [LSE]
__aarch64_swp8_relax [LSE]


# ----------------------------------------------------------------------------
Expand Down
75 changes: 47 additions & 28 deletions src/allocators/MimallocArena.zig
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,8 @@ const Self = @This();
///
/// This type is a `GenericAllocator`; see `src/allocators.zig`.
pub const Default = struct {
pub fn allocator(self: Default) std.mem.Allocator {
_ = self;
return Borrowed.getDefault().allocator();
pub fn allocator(_: Default) std.mem.Allocator {
return .{ .ptr = undefined, .vtable = &global_mimalloc_vtable };
}
};
Comment on lines 8 to 14
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.

🟡 The doc comment on Default (line 7) still reads '/// Uses the default thread-local heap. This type is zero-sized.' but this PR changed Default.allocator() to return .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }, routing through mi_malloc/mi_free directly with no thread-local heap pointer. The comment should be updated to reflect the new behavior (e.g. '/// Routes allocations through mi_malloc/mi_free directly; no per-thread heap pointer is cached. This type is zero-sized.').

Extended reasoning...

What the bug is and how it manifests

The doc comment on the Default struct at src/allocators/MimallocArena.zig line 7 reads:

/// Uses the default thread-local heap. This type is zero-sized.

This comment was accurate before this PR, when Default.allocator() called Borrowed.getDefault().allocator(), which in turn called getThreadHeap()mi_heap_get_default() — a genuinely thread-local heap pointer. After this PR, Default.allocator() returns .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }, which calls mi_malloc/mi_free directly through the global vtable. There is no per-thread heap pointer involved at all.

The specific code path that triggers it

Default.allocator() now routes through global_mimalloc_vtable, which dispatches to global_vtable_alloc()mimalloc.mi_malloc(). The .ptr field is undefined — not a *mi_heap_t. The PR description itself notes: 'In v3, mi_malloc/mi_free are already thread-local-fast, there is no per-thread default heap to cache.' The code was updated; the comment was not.

Why existing code doesn't prevent it

The stale comment is purely a documentation issue — there is no runtime check or assertion that catches incorrect comments. The implementation is correct; only the doc comment describing it is wrong.

What the impact would be

A developer reading the comment on Default would believe it caches or uses a per-thread mi_heap_t pointer, which is now false. This is particularly confusing because the Borrowed struct does use a per-heap pointer (heap_allocator_vtable), and the distinction is the core architectural change of this PR. Misleading documentation could lead to incorrect reasoning about allocator identity, thread-safety properties, or whether ownsPtr/gc are meaningful on a Default-derived allocator.

How to fix it

Update line 7 to reflect the new behavior:

-/// Uses the default thread-local heap. This type is zero-sized.
+/// Routes allocations through mi_malloc/mi_free directly; no per-thread heap pointer is cached.
+/// This type is zero-sized.

Step-by-step proof

  1. Before this PR: Default.allocator() called Borrowed.getDefault().allocator()getThreadHeap()mi_heap_get_default() → returns current thread's mi_heap_t. Comment '/// Uses the default thread-local heap' was accurate.
  2. After this PR: Default.allocator() returns .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }. The ptr is undefined; no heap is fetched. Calls go through global_vtable_allocmi_malloc() directly.
  3. Result: The comment describes v2 behavior that no longer exists in the code.


Expand All @@ -23,11 +22,20 @@ pub const Borrowed = struct {
#heap: BorrowedHeap,

pub fn allocator(self: Borrowed) std.mem.Allocator {
return .{ .ptr = self.#heap, .vtable = &c_allocator_vtable };
return .{ .ptr = self.#heap, .vtable = &heap_allocator_vtable };
}

/// Prefer `Default.allocator()` / `getThreadLocalDefault()` for a thread-safe
/// global allocator. This returns the process-wide main heap so that
/// `gc()` / `ownsPtr()` on the result remain meaningful.
pub fn getDefault() Borrowed {
return .{ .#heap = getThreadHeap() };
const heap = mimalloc.mi_heap_main();
if (comptime !safety_checks) return .{ .#heap = heap };
const S = struct {
threadlocal var dbg: ?DebugHeap = null;
};
if (S.dbg == null) S.dbg = .{ .inner = heap, .thread_lock = .initLocked() };
return .{ .#heap = &S.dbg.? };
}

pub fn gc(self: Borrowed) void {
Expand All @@ -42,7 +50,7 @@ pub const Borrowed = struct {
}

pub fn ownsPtr(self: Borrowed, ptr: *const anyopaque) bool {
return mimalloc.mi_heap_check_owned(self.getMimallocHeap(), ptr);
return mimalloc.mi_heap_contains(self.getMimallocHeap(), ptr);
}

fn fromOpaque(ptr: *anyopaque) Borrowed {
Expand Down Expand Up @@ -81,8 +89,8 @@ pub const Borrowed = struct {

pub fn downcast(std_alloc: std.mem.Allocator) Borrowed {
bun.assertf(
isInstance(std_alloc),
"not a MimallocArena (vtable is {*})",
std_alloc.vtable == &heap_allocator_vtable,
"not an owned MimallocArena heap (vtable is {*})",
.{std_alloc.vtable},
Comment on lines +92 to 94
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== MimallocArena.isInstance call sites ==="
rg -n -C3 --glob '*.zig' 'MimallocArena\.isInstance\s*\('

echo
echo "=== Borrowed.downcast call sites ==="
rg -n -C3 --glob '*.zig' 'MimallocArena\.Borrowed\.downcast\s*\(|Borrowed\.downcast\s*\('

Repository: oven-sh/bun

Length of output: 1677


🏁 Script executed:

# Check context around line 165 in src/safety/alloc.zig to see if downcast follows
cat -n src/safety/alloc.zig | sed -n '162,180p'

Repository: oven-sh/bun

Length of output: 838


🏁 Script executed:

# Verify the actual vtable checks in MimallocArena.zig at the mentioned lines
echo "=== Lines 92-94 (Borrowed.downcast assertion) ==="
cat -n src/allocators/MimallocArena.zig | sed -n '90,96p'

echo ""
echo "=== Lines 264-265 (isInstance function) ==="
cat -n src/allocators/MimallocArena.zig | sed -n '263,267p'

Repository: oven-sh/bun

Length of output: 670


🏁 Script executed:

# Check what vtables Default.allocator() and getThreadLocalDefault() produce
echo "=== Search for Default.allocator and getThreadLocalDefault ==="
rg -n -A5 'pub fn allocator\(\)' src/allocators/MimallocArena.zig

rg -n -A5 'pub fn getThreadLocalDefault' src/allocators/MimallocArena.zig

Repository: oven-sh/bun

Length of output: 384


🏁 Script executed:

# Check if there are any other Borrowed usages beyond downcast that might have implicit safety assumptions
echo "=== Borrowed struct and all its methods ==="
rg -n -B2 -A8 'pub.*Borrowed' src/allocators/MimallocArena.zig | head -60

Repository: oven-sh/bun

Length of output: 2186


🏁 Script executed:

# Look for Default.allocator() - the review mentions it produces global_mimalloc_vtable
echo "=== Default.allocator implementation ==="
rg -n -B2 -A5 'pub.*fn.*allocator.*\(\s*\)' src/allocators/MimallocArena.zig | grep -A5 "Default"

echo ""
echo "=== Check all public Default methods ==="
rg -n -B1 -A6 'pub const Default' src/allocators/MimallocArena.zig

Repository: oven-sh/bun

Length of output: 467


🏁 Script executed:

# Double-check for any Borrowed.downcast calls that might exist
echo "=== Any downcast calls anywhere in codebase ==="
rg -n 'downcast\s*\(' --glob '*.zig' | grep -i mimalloc

echo ""
echo "=== Check if any code path could trigger downcast on global_mimalloc_vtable ==="
# Search for patterns that might combine isInstance check with downcast
rg -n -B5 -A5 'downcast' src/allocators/MimallocArena.zig

Repository: oven-sh/bun

Length of output: 747


🏁 Script executed:

# Check the context around the downcast call in LinkerGraph.zig
echo "=== Context around downcast call in LinkerGraph.zig:420 ==="
cat -n src/bundler/LinkerGraph.zig | sed -n '410,430p'

echo ""
echo "=== Look at the class/struct definition for LinkerGraph to understand this.allocator ==="
rg -n -B5 -A15 'pub const LinkerGraph = struct' src/bundler/LinkerGraph.zig | head -40

Repository: oven-sh/bun

Length of output: 1070


🏁 Script executed:

# Search for LinkerGraph's allocator field definition
echo "=== LinkerGraph allocator field ==="
rg -n 'allocator.*:' src/bundler/LinkerGraph.zig | head -20

echo ""
echo "=== Find where LinkerGraph is instantiated/created to see what allocator is passed ==="
rg -n 'LinkerGraph.*=' src/bundler/LinkerGraph.zig | head -10

Repository: oven-sh/bun

Length of output: 325


🏁 Script executed:

# Search for LinkerGraph creation/init calls
echo "=== LinkerGraph creation patterns ==="
rg -n 'LinkerGraph\.init\(' --glob '*.zig' -B2 -A2

echo ""
echo "=== Alternative: search for LinkerGraph{...} construction ==="
rg -n 'LinkerGraph\s*\{' --glob '*.zig' -B2 -A2 | head -40

Repository: oven-sh/bun

Length of output: 580


Fix API contract between isInstance() and downcast().

isInstance() (line 265) claims allocators with &global_mimalloc_vtable are MimallocArena instances, but downcast() (line 92) rejects them. This breaks the safety contract: a predicate should reliably guard a cast. The code in src/bundler/LinkerGraph.zig:420 calls .downcast(this.allocator) without an isInstance() guard, relying on runtime assertions to catch misuse—but isInstance() would return true for Default.allocator() or getThreadLocalDefault(), leading to a crash.

Either narrow isInstance() to only owned heaps (&heap_allocator_vtable), or split into separate predicates:

 pub fn isInstance(alloc: std.mem.Allocator) bool {
-    return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
+    return alloc.vtable == &heap_allocator_vtable;
 }
+
+pub fn isMimallocBacked(alloc: std.mem.Allocator) bool {
+    return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
+}

Alternatively, add a guard before the downcast in LinkerGraph.takeAstOwnership() or document that only owned heaps may be passed.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std_alloc.vtable == &heap_allocator_vtable,
"not an owned MimallocArena heap (vtable is {*})",
.{std_alloc.vtable},
pub fn isInstance(alloc: std.mem.Allocator) bool {
return alloc.vtable == &heap_allocator_vtable;
}
pub fn isMimallocBacked(alloc: std.mem.Allocator) bool {
return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/allocators/MimallocArena.zig` around lines 92 - 94, The predicate/cast
contract is inconsistent: isInstance() currently treats allocators with
&global_mimalloc_vtable as MimallocArena but downcast() only accepts
&heap_allocator_vtable, causing unsafe casts (e.g., in
LinkerGraph.takeAstOwnership calling .downcast(this.allocator)). Fix by making
isInstance() and downcast() agree — either: (A) narrow isInstance() to only
return true for &heap_allocator_vtable (owned heaps) so downcast()’s check
remains correct, or (B) broaden downcast() to accept &global_mimalloc_vtable as
well; update the functions named isInstance() and downcast() and ensure callers
like LinkerGraph.takeAstOwnership(), Default.allocator(), and
getThreadLocalDefault() use the corrected predicate or add an explicit guard
before calling downcast().

);
return .fromOpaque(std_alloc.ptr);
Expand All @@ -98,19 +106,6 @@ const DebugHeap = struct {
pub const deinit = void;
};

threadlocal var thread_heap: if (safety_checks) ?DebugHeap else void = if (safety_checks) null;

fn getThreadHeap() BorrowedHeap {
if (comptime !safety_checks) return mimalloc.mi_heap_get_default();
if (thread_heap == null) {
thread_heap = .{
.inner = mimalloc.mi_heap_get_default(),
.thread_lock = .initLocked(),
};
}
return &thread_heap.?;
}

const log = bun.Output.scoped(.mimalloc, .hidden);

pub fn allocator(self: Self) std.mem.Allocator {
Expand All @@ -121,13 +116,11 @@ pub fn borrow(self: Self) Borrowed {
return .{ .#heap = if (comptime safety_checks) self.#heap.get() else self.#heap };
}

/// Internally, mimalloc calls mi_heap_get_default()
/// to get the default heap.
/// It uses pthread_getspecific to do that.
/// We can save those extra calls if we just do it once in here
/// In v3, `mi_malloc`/`mi_free` are already thread-local-fast — there is no
/// per-thread default heap to cache. Route through the global vtable.
pub fn getThreadLocalDefault() std.mem.Allocator {
if (bun.Environment.enable_asan) return bun.default_allocator;
return Borrowed.getDefault().allocator();
return .{ .ptr = undefined, .vtable = &global_mimalloc_vtable };
}

pub fn backingAllocator(_: Self) std.mem.Allocator {
Expand Down Expand Up @@ -251,17 +244,43 @@ fn vtable_remap(ptr: *anyopaque, buf: []u8, alignment: Alignment, new_len: usize
return @ptrCast(value);
}

fn global_vtable_alloc(_: *anyopaque, len: usize, alignment: Alignment, _: usize) ?[*]u8 {
log("Malloc: {d}\n", .{len});
const ptr: ?*anyopaque = if (mimalloc.mustUseAlignedAlloc(alignment))
mimalloc.mi_malloc_aligned(len, alignment.toByteUnits())
else
mimalloc.mi_malloc(len);
return if (ptr) |p| @ptrCast(p) else null;
}

fn global_vtable_resize(_: *anyopaque, buf: []u8, _: Alignment, new_len: usize, _: usize) bool {
Comment on lines 244 to +256
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.

🟡 The new global_vtable_alloc() is missing the debug-mode usable-size assertion that Borrowed.alignedAlloc() has. In debug builds, if mimalloc returns a block smaller than requested through the global path, it will silently go undetected instead of panicking.

Extended reasoning...

What the bug is:

Borrowed.alignedAlloc() (used by heap_allocator_vtable) includes a comptime-guarded debug assertion that validates mimalloc returned a block of at least the requested size:

if (comptime bun.Environment.isDebug) {
    const usable = mimalloc.mi_malloc_usable_size(ptr);
    if (usable < len) {
        std.debug.panic("mimalloc: allocated size is too small: {d} < {d}", .{ usable, len });
    }
}

The new global_vtable_alloc() introduced in this PR for global_mimalloc_vtable omits this check entirely, returning the pointer immediately after the mi_malloc/mi_malloc_aligned call.

The specific code path:

Default.allocator() and getThreadLocalDefault() now route through global_mimalloc_vtable, which calls global_vtable_alloc(). This is a new code path introduced by this PR. Previously both of these called Borrowed.getDefault().allocator(), which used heap_allocator_vtablevtable_alloc()alignedAlloc(), which has the check.

Why existing code doesn't prevent it:

The check in alignedAlloc() is guarded by comptime bun.Environment.isDebug so it only applies to debug builds. In production nothing changes. But global_vtable_alloc() doesn't have any equivalent guard — the assertion is simply absent.

Impact:

In debug builds, any allocation going through Default.allocator() or getThreadLocalDefault() (which is the majority of bun's allocations) will not have the invariant checked. If a mimalloc internal bug caused it to return an undersized block on the global path, it would silently proceed rather than panicking with a clear diagnostic. The scope is broad since Default.allocator() is the primary allocator throughout the codebase.

How to fix:

Add the same debug assertion in global_vtable_alloc() after the allocation:

fn global_vtable_alloc(_: *anyopaque, len: usize, alignment: Alignment, _: usize) ?[*]u8 {
    log("Malloc: {d}\n", .{len});
    const ptr: ?*anyopaque = if (mimalloc.mustUseAlignedAlloc(alignment))
        mimalloc.mi_malloc_aligned(len, alignment.toByteUnits())
    else
        mimalloc.mi_malloc(len);
    if (comptime bun.Environment.isDebug) {
        const usable = mimalloc.mi_malloc_usable_size(ptr);
        if (usable < len) {
            std.debug.panic("mimalloc: allocated size is too small: {d} < {d}", .{ usable, len });
        }
    }
    return if (ptr) |p| @ptrCast(p) else null;
}

Step-by-step proof:

  1. Caller calls bun.default_allocator.alloc(u8, 64) (which routes through Default.allocator())
  2. Default.allocator() returns .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }
  3. The allocator calls global_mimalloc_vtable.allocglobal_vtable_alloc(_, 64, ...)
  4. mi_malloc(64) is called; hypothetically returns a block of usable size 48 due to a mimalloc bug
  5. global_vtable_alloc returns the pointer with no check
  6. In contrast, the same scenario through heap_allocator_vtablealignedAlloc() would call mi_malloc_usable_size, detect 48 < 64, and panic with a clear error message

return mimalloc.mi_expand(buf.ptr, new_len) != null;
}

fn global_vtable_remap(_: *anyopaque, buf: []u8, alignment: Alignment, new_len: usize, _: usize) ?[*]u8 {
return @ptrCast(mimalloc.mi_realloc_aligned(buf.ptr, new_len, alignment.toByteUnits()));
}

pub fn isInstance(alloc: std.mem.Allocator) bool {
return alloc.vtable == &c_allocator_vtable;
return alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable;
}

const c_allocator_vtable = std.mem.Allocator.VTable{
/// VTable for owned heaps created with `mi_heap_new`.
const heap_allocator_vtable = std.mem.Allocator.VTable{
.alloc = vtable_alloc,
.resize = vtable_resize,
.remap = vtable_remap,
.free = vtable_free,
};

/// VTable for the process-wide default allocator (`mi_malloc`/`mi_free`).
const global_mimalloc_vtable = std.mem.Allocator.VTable{
.alloc = global_vtable_alloc,
.resize = global_vtable_resize,
.remap = global_vtable_remap,
.free = vtable_free,
};

Comment on lines +268 to +283
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.

🟡 The PR splits the single mimalloc vtable into two (heap_allocator_vtable for owned heaps, global_mimalloc_vtable for the global allocator), but isInstance() returns true for both while Borrowed.downcast() only accepts heap_allocator_vtable — any code following the natural isInstance-then-downcast idiom will panic at the assertf for global-vtable allocators. No existing callers use this pattern today, so there is no immediate breakage, but the API contract is misleading since isInstance() returning true no longer implies downcast() will succeed.

Extended reasoning...

What the bug is and how it manifests

This PR intentionally splits the old single c_allocator_vtable into two vtables: heap_allocator_vtable (used by Borrowed.allocator() for per-heap mi_heap_new arenas) and global_mimalloc_vtable (used by Default.allocator() and getThreadLocalDefault() routing through mi_malloc/mi_free directly). The isInstance() function was updated to return true for both vtables. However, Borrowed.downcast() was updated to assert std_alloc.vtable == &heap_allocator_vtable exclusively, panicking for global-vtable allocators with the message "not an owned MimallocArena heap".

The specific code path that triggers it

isInstance() at line ~268 returns alloc.vtable == &heap_allocator_vtable or alloc.vtable == &global_mimalloc_vtable. Borrowed.downcast() at lines ~268-283 asserts std_alloc.vtable == &heap_allocator_vtable and then calls fromOpaque(std_alloc.ptr). For global-vtable allocators, ptr = undefined, so if the assertion were somehow bypassed, calling fromOpaque(undefined) would be UB/crash.

Why existing code does not prevent it

Before this PR, downcast() called isInstance() internally as its guard condition, so they were always in sync. After this PR, downcast() no longer uses isInstance() — it checks the vtable directly but with the narrower predicate. No existing caller currently chains isInstance into downcast: LinkerGraph.zig:420 calls downcast() directly without any isInstance guard, and the two usages in safety/alloc.zig use isInstance() without downcast(). So the divergence is latent but not currently triggered.

What the impact would be

The conventional semantic of isInstance(alloc) is that it acts as a precondition for safely casting/downcasting the allocator. A future caller writing the natural idiom if (MimallocArena.isInstance(alloc)) { const b = Borrowed.downcast(alloc); } would get a bun.assertf panic in debug/CI builds when alloc comes from Default.allocator() or getThreadLocalDefault(). In release builds without the assertion, fromOpaque(undefined) would be called, which is undefined behavior. Additionally, any code inspecting alloc.ptr after a positive isInstance check (expecting it to be a valid heap pointer) would read undefined for the global vtable case.

How to fix it

Two reasonable options: (1) Rename isInstance to isMimallocBacked or isAnyMimallocInstance to make clear it covers both vtable variants and does not imply downcast() is safe; add a separate isOwnedInstance(alloc) that checks only heap_allocator_vtable. (2) Document clearly on both isInstance and downcast that they have different scopes, so callers know not to use isInstance as a downcast guard.

Step-by-step proof

  1. Call MimallocArena.getThreadLocalDefault() → returns .{ .ptr = undefined, .vtable = &global_mimalloc_vtable }
  2. Call MimallocArena.isInstance(alloc) → evaluates alloc.vtable == &global_mimalloc_vtablereturns true
  3. Believing downcast is safe (as it was before this PR), call Borrowed.downcast(alloc)
  4. downcast asserts alloc.vtable == &heap_allocator_vtablefalsebun.assertf fires: "not an owned MimallocArena heap"
  5. In a release build without the assertion, execution continues to fromOpaque(undefined) → UB

const std = @import("std");
const Alignment = std.mem.Alignment;

Expand Down
12 changes: 5 additions & 7 deletions src/allocators/mimalloc.zig
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,15 @@ pub const Heap = opaque {
return mi_heap_realloc(self, p, newsize);
}

pub fn isOwned(self: *Heap, p: ?*anyopaque) bool {
return mi_heap_check_owned(self, p);
pub fn isOwned(self: *Heap, p: ?*const anyopaque) bool {
return mi_heap_contains(self, p);
}
};
pub extern fn mi_heap_new() ?*Heap;
pub extern fn mi_heap_delete(heap: *Heap) void;
pub extern fn mi_heap_destroy(heap: *Heap) void;
pub extern fn mi_heap_set_default(heap: *Heap) *Heap;
pub extern fn mi_heap_get_default() *Heap;
pub extern fn mi_heap_get_backing() *Heap;
pub extern fn mi_heap_main() *Heap;
pub extern fn mi_heap_contains(heap: *const Heap, p: ?*const anyopaque) bool;
pub extern fn mi_heap_collect(heap: *Heap, force: bool) void;
pub extern fn mi_heap_malloc(heap: *Heap, size: usize) ?*anyopaque;
pub extern fn mi_heap_zalloc(heap: *Heap, size: usize) ?*anyopaque;
Expand Down Expand Up @@ -102,8 +101,6 @@ pub extern fn mi_heap_rezalloc_aligned(heap: *Heap, p: ?*anyopaque, newsize: usi
pub extern fn mi_heap_rezalloc_aligned_at(heap: *Heap, p: ?*anyopaque, newsize: usize, alignment: usize, offset: usize) ?*anyopaque;
pub extern fn mi_heap_recalloc_aligned(heap: *Heap, p: ?*anyopaque, newcount: usize, size: usize, alignment: usize) ?*anyopaque;
pub extern fn mi_heap_recalloc_aligned_at(heap: *Heap, p: ?*anyopaque, newcount: usize, size: usize, alignment: usize, offset: usize) ?*anyopaque;
pub extern fn mi_heap_contains_block(heap: *Heap, p: *const anyopaque) bool;
pub extern fn mi_heap_check_owned(heap: *Heap, p: *const anyopaque) bool;
pub extern fn mi_check_owned(p: ?*const anyopaque) bool;
pub const struct_mi_heap_area_s = extern struct {
blocks: ?*anyopaque,
Expand All @@ -112,6 +109,7 @@ pub const struct_mi_heap_area_s = extern struct {
used: usize,
block_size: usize,
full_block_size: usize,
reserved1: ?*anyopaque,
};
pub const mi_heap_area_t = struct_mi_heap_area_s;
pub const mi_block_visit_fun = *const fn (?*const Heap, [*c]const mi_heap_area_t, ?*anyopaque, usize, ?*anyopaque) callconv(.c) bool;
Expand Down
32 changes: 31 additions & 1 deletion src/bun.js/modules/BunJSCModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
#include "JSDOMConvertBase.h"
#include "ZigSourceProvider.h"
#include "mimalloc.h"
extern "C" char* mi_stats_get_json(size_t, char*);
extern "C" char* mi_heap_dump_json(bool include_blocks, bool hash_addresses);

#include <JavaScriptCore/ControlFlowProfiler.h>

Expand Down Expand Up @@ -219,7 +221,7 @@ createMemoryFootprintStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject)

JSC_DECLARE_HOST_FUNCTION(functionMemoryUsageStatistics);
JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics,
(JSGlobalObject * globalObject, CallFrame*))
(JSGlobalObject * globalObject, CallFrame* callFrame))
{

auto& vm = JSC::getVM(globalObject);
Expand Down Expand Up @@ -356,6 +358,34 @@ JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics,
#endif
#endif

mi_collect(false);
if (char* json = mi_stats_get_json(0, nullptr)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
parsed.isEmpty() ? jsNull() : parsed);
}
Comment on lines +361 to +367
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.

⚠️ Potential issue | 🟡 Minor

Always emit the mimalloc key for a stable heapStats() shape.

When mi_stats_get_json() returns null, this currently omits the property entirely. Prefer always setting mimalloc (fallback null) so consumers can rely on a consistent object contract.

🧩 Proposed fix
-    mi_collect(false);
-    if (char* json = mi_stats_get_json(0, nullptr)) {
-        JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
-        mi_free(json);
-        object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
-            parsed.isEmpty() ? jsNull() : parsed);
-    }
+    mi_collect(false);
+    JSValue mimallocStats = jsNull();
+    if (char* json = mi_stats_get_json(0, nullptr)) {
+        JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
+        mi_free(json);
+        mimallocStats = parsed.isEmpty() ? jsNull() : parsed;
+    }
+    object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s), mimallocStats);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
mi_collect(false);
if (char* json = mi_stats_get_json(0, nullptr)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
parsed.isEmpty() ? jsNull() : parsed);
}
mi_collect(false);
JSValue mimallocStats = jsNull();
if (char* json = mi_stats_get_json(0, nullptr)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
mimallocStats = parsed.isEmpty() ? jsNull() : parsed;
}
object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s), mimallocStats);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/modules/BunJSCModule.h` around lines 360 - 366, The current code
only sets the "mimalloc" property when mi_stats_get_json(0, nullptr) returns
non-null; always emit the property to keep heapStats() shape stable by calling
mi_collect(false) then calling mi_stats_get_json and if it returns non-null
parse and mi_free as before but ensure object->putDirect(vm,
Identifier::fromString(vm, "mimalloc"_s), parsed.isEmpty() ? jsNull() : parsed)
is executed in both branches; if mi_stats_get_json returns null, call
object->putDirect with jsNull() for the "mimalloc" key so the property is always
present.


// heapStats({ dump: true | "blocks" }) -> per-heap/per-page (and per-block) live snapshot.
JSValue arg0 = callFrame->argument(0);
if (arg0.isObject()) {
JSValue dump = arg0.getObject()->get(globalObject, Identifier::fromString(vm, "dump"_s));
if (dump.toBoolean(globalObject)) {
const bool includeBlocks = dump.isString() && dump.toWTFString(globalObject) == "blocks"_s;
#if BUN_DEBUG
const bool hashAddresses = false;
#else
const bool hashAddresses = true;
#endif
if (char* json = mi_heap_dump_json(includeBlocks, hashAddresses)) {
JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
mi_free(json);
object->putDirect(vm, Identifier::fromString(vm, "mimallocDump"_s),
parsed.isEmpty() ? jsNull() : parsed);
}
}
}
Comment on lines +369 to +387
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.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that functionMemoryUsageStatistics performs JS-visible operations
# without a local throw scope / exception checks.
rg -n -C3 'functionMemoryUsageStatistics|DECLARE_THROW_SCOPE|JSONParse\(|->get\(globalObject, Identifier::fromString' src/bun.js/modules/BunJSCModule.h

Repository: oven-sh/bun

Length of output: 8839


🏁 Script executed:

sed -n '223,395p' src/bun.js/modules/BunJSCModule.h | cat -n

Repository: oven-sh/bun

Length of output: 8469


Add throw scope and exception checking to functionMemoryUsageStatistics.

This function performs multiple JS-visible operations without exception handling: arg0.getObject()->get(...) can execute user getters and Proxy traps, and both JSONParse(...) calls can throw. Without a throw scope, exceptions from these operations are silently dropped, leaving the result object partially constructed and returning normally.

Proposed fix
 JSC_DEFINE_HOST_FUNCTION(functionMemoryUsageStatistics,
     (JSGlobalObject * globalObject, CallFrame* callFrame))
 {
     auto& vm = JSC::getVM(globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
 
     if (vm.heap.size() == 0) {
         vm.heap.collectNow(Sync, CollectionScope::Full);
     }
@@
     mi_collect(false);
     if (char* json = mi_stats_get_json(0, nullptr)) {
         JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
         mi_free(json);
+        RETURN_IF_EXCEPTION(scope, {});
         object->putDirect(vm, Identifier::fromString(vm, "mimalloc"_s),
             parsed.isEmpty() ? jsNull() : parsed);
     }
 
     // heapStats({ dump: true | "blocks" }) -> per-heap/per-page (and per-block) live snapshot.
     JSValue arg0 = callFrame->argument(0);
     if (arg0.isObject()) {
         JSValue dump = arg0.getObject()->get(globalObject, Identifier::fromString(vm, "dump"_s));
+        RETURN_IF_EXCEPTION(scope, {});
         if (dump.toBoolean(globalObject)) {
+            RETURN_IF_EXCEPTION(scope, {});
             const bool includeBlocks = dump.isString() && dump.toWTFString(globalObject) == "blocks"_s;
+            RETURN_IF_EXCEPTION(scope, {});
 `#if` BUN_DEBUG
             const bool hashAddresses = false;
 `#else`
             const bool hashAddresses = true;
 `#endif`
             if (char* json = mi_heap_dump_json(includeBlocks, hashAddresses)) {
                 JSValue parsed = JSONParse(globalObject, String::fromUTF8(json));
                 mi_free(json);
+                RETURN_IF_EXCEPTION(scope, {});
                 object->putDirect(vm, Identifier::fromString(vm, "mimallocDump"_s),
                     parsed.isEmpty() ? jsNull() : parsed);
             }
         }
     }
 
-    return JSValue::encode(object);
+    RELEASE_AND_RETURN(scope, JSValue::encode(object));
 }

Comment on lines 358 to +387
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.

🟡 The new heapStats dump section in functionMemoryUsageStatistics has two issues: (1) mi_stats_get_json returning null silently omits the mimalloc property entirely, breaking the stable shape callers expect — it should always emit jsNull() as a fallback; (2) the dump section calls arg0.getObject()->get() and dump.toBoolean() — both can throw via Proxy traps or Symbol.toPrimitive — but the function has no DECLARE_THROW_SCOPE and no RETURN_IF_EXCEPTION guards, so a pending JS exception is silently swallowed while execution continues into the native mi_heap_dump_json call and returns normally.

Extended reasoning...

Bug 1 — Missing null fallback for mimalloc property

The mimalloc property is only set on the heapStats() result when mi_stats_get_json(0, nullptr) returns non-null. If it returns null (OOM allocating the internal JSON buffer), the key is simply absent from the returned object. Callers relying on a stable shape — including the new test at heapStats-mimalloc.test.ts:7 which accesses s.mimalloc.mimalloc_version — would receive a TypeError when s.mimalloc is undefined. The fix is trivial: hoist a JSValue mimallocStats = jsNull() before the if-block and move the putDirect call outside it, unconditionally emitting the property.

Bug 2 — Missing exception propagation in the dump section

Before this PR, functionMemoryUsageStatistics accepted an unnamed CallFrame* argument and performed zero JS operations on user input. This PR adds a new code path that reads callFrame->argument(0): it calls arg0.getObject()->get(globalObject, ...) and then dump.toBoolean(globalObject). Both operations take a JSGlobalObject* and can throw JS exceptions — get() via a Proxy trap or throwing getter, toBoolean() via Symbol.toPrimitive or valueOf().

The function has no DECLARE_THROW_SCOPE anywhere. Every other JS-calling host function in BunJSCModule.h (lines 79, 130, 446, 481, etc.) declares one. Neither call is followed by RETURN_IF_EXCEPTION. If an exception is raised by get(), execution falls through to dump.toBoolean() then into the native mi_heap_dump_json C function (which runs while a JS exception is pending), then JSONParse, then mi_free, then putDirect, and finally returns JSValue::encode(object) as if nothing went wrong. The pending exception is silently swallowed or overwritten.

Why existing code does not prevent this

JSC contract: any host function passing a JSGlobalObject* to a throwing operation must own a throw scope and check for exceptions before proceeding. Without a throw scope there is no compile-time enforcement. All other functions in the file follow this pattern; the new code does not.

Proof for Bug 2

  1. Construct: const p = new Proxy({}, { get() { throw new Error('trap') } })
  2. Call heapStats(p) — arg0.isObject() is true
  3. arg0.getObject()->get(globalObject, ...dump...) triggers the trap, sets pending exception
  4. No RETURN_IF_EXCEPTION check; execution continues
  5. dump.toBoolean(globalObject) runs with pending exception
  6. mi_heap_dump_json is called with a pending exception in the VM
  7. Function returns JSValue::encode(object) — the exception from step 3 is lost

Impact

Bug 1: Consumers see undefined for mimalloc instead of null when the stats buffer allocation fails. Extremely unlikely in practice but the API shape is unstable. The new test would also fail with a confusing TypeError rather than a clear test assertion failure.

Bug 2: Passing a Proxy with a throwing dump getter to heapStats() silently swallows the exception and returns a result object. This is an internal debug API so real-world impact is low, but the JSC invariant violation is real.

Fix

For Bug 1: move putDirect for mimalloc outside the null check, always emitting the property (jsNull() when mi_stats_get_json fails).
For Bug 2: add DECLARE_THROW_SCOPE(vm) at the top of functionMemoryUsageStatistics and add RETURN_IF_EXCEPTION(scope, {}) after the get() call and after the toBoolean() call.


return JSValue::encode(object);
}

Expand Down
65 changes: 65 additions & 0 deletions test/js/bun/jsc/heapStats-mimalloc.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { heapStats } from "bun:jsc";
import { describe, expect, test } from "bun:test";

describe("heapStats() mimalloc integration", () => {
test("mimalloc aggregate stats are present", () => {
const s = heapStats();
expect(s.mimalloc).toBeDefined();
expect(s.mimalloc.mimalloc_version).toBeGreaterThan(3000);
expect(s.mimalloc.pages.current).toBeGreaterThan(0);
expect(s.mimalloc.committed.current).toBeGreaterThan(0);
expect(Array.isArray(s.mimalloc.malloc_bins)).toBe(true);
});

test("heapStats({dump: true}) returns per-heap pages", () => {
const s = heapStats({ dump: true });
expect(s.mimallocDump).toBeDefined();
expect(Array.isArray(s.mimallocDump.heaps)).toBe(true);
expect(s.mimallocDump.heaps.length).toBeGreaterThan(0);
const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(main).toBeDefined();
expect(Array.isArray(main.pages)).toBe(true);
expect(main.pages.length).toBeGreaterThan(0);
const page = main.pages[0];
expect(typeof page.id).toBe("number");
expect(page.block_size).toBeGreaterThan(0);
expect(page.used).toBeGreaterThanOrEqual(0);
expect(page.reserved).toBeGreaterThan(0);
expect(typeof page.thread_id).toBe("number");
// pages-only mode: no blocks
expect(main.blocks).toBeUndefined();
});

test("heapStats({dump: 'blocks'}) includes per-block ids", () => {
const s = heapStats({ dump: "blocks" });
const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(Array.isArray(main.blocks)).toBe(true);
expect(main.blocks.length).toBeGreaterThan(0);
const [id, size] = main.blocks[0];
expect(typeof id).toBe("number");
expect(size).toBeGreaterThan(0);
// every block size should match some page's block_size
const pageSizes = new Set(main.pages.map((p: any) => p.block_size));
for (const [, sz] of main.blocks.slice(0, 50)) {
expect(pageSizes.has(sz)).toBe(true);
Comment on lines +39 to +44
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.

🟡 The 'heapStats({dump: "blocks"})' test accesses main.blocks without first asserting main is defined, unlike the parallel 'dump: true' test which correctly guards with expect(main).toBeDefined(). If heaps.find(h => h.seq === 0) returns undefined for any reason, the test will throw a TypeError: Cannot read properties of undefined (reading 'blocks') instead of a descriptive assertion failure.

Extended reasoning...

What the bug is and how it manifests

In test/js/bun/jsc/heapStats-mimalloc.test.ts, the two dump-mode tests follow the same pattern — finding the main heap by seq === 0 and then accessing properties on it. The 'dump: true' test (lines 19–20) correctly does:

const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(main).toBeDefined();  // guard present
expect(Array.isArray(main.pages)).toBe(true);

The 'dump: "blocks"' test (lines 35–36) skips the guard entirely:

const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
// no expect(main).toBeDefined() here
expect(Array.isArray(main.blocks)).toBe(true);  // will throw TypeError if main is undefined

The specific code path that triggers it

Array.prototype.find returns undefined when no element matches the predicate. If heaps contains no entry with seq === 0, main is undefined. Accessing undefined.blocks throws TypeError: Cannot read properties of undefined (reading 'blocks') — a JavaScript runtime error rather than a Bun test assertion failure.

Why existing code doesn't prevent it

The 'dump: true' test deliberately adds expect(main).toBeDefined() as a safety guard before touching main.pages. The 'dump: "blocks"' test was written without mirroring this pattern. There is no structural mechanism that enforces the guard.

What the impact would be

In practice, the process-wide main heap (mi_heap_main(), always seq === 0) will always be present in the dump. So this bug will not cause false failures under normal conditions. However, if the mimalloc internals ever change how heaps are numbered, or if a platform-specific quirk causes the main heap to be absent or differently sequenced, the resulting error message — TypeError: Cannot read properties of undefined — is unhelpful and will mask the real failure. The 'dump: true' test would catch the same situation with a clear expect(main).toBeDefined() failure message pointing directly at the missing heap.

How to fix it

Add the missing guard at line 37 (between the find call and the blocks access):

const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0);
expect(main).toBeDefined();  // add this line
expect(Array.isArray(main.blocks)).toBe(true);

Step-by-step proof

  1. heapStats({ dump: "blocks" }) is called; assume hypothetically that mimallocDump.heaps contains no entry with seq === 0
  2. const main = s.mimallocDump.heaps.find((h: any) => h.seq === 0)main is undefined
  3. expect(Array.isArray(main.blocks)) → JS evaluates main.blocksTypeError: Cannot read properties of undefined (reading 'blocks') is thrown
  4. Bun test reports an unhandled error with no actionable failure message, rather than something like expected undefined to be defined
  5. Compare: the 'dump: true' test would instead hit expect(main).toBeDefined() and report a clear assertion failure at the correct line

}
});

test("dump reflects new heaps and allocations", () => {
const before = heapStats({ dump: true }).mimallocDump.heaps.length;
// MimallocArena is internal; trigger via something that creates a heap.
// Transpiler creates a per-call arena.
const t = new Bun.Transpiler();
const out = t.transformSync("export const x = 1");
expect(out.length).toBeGreaterThan(0);
const after = heapStats({ dump: true }).mimallocDump.heaps;
// Either a new heap was created (and may already be destroyed), or main grew.
// We assert the dump is still well-formed and >= before.
expect(after.length).toBeGreaterThanOrEqual(1);
for (const h of after) {
expect(typeof h.seq).toBe("number");
expect(Array.isArray(h.pages)).toBe(true);
}
void before;
});
Comment on lines +48 to +64
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.

⚠️ Potential issue | 🟡 Minor

This test never checks the before/after signal it describes.

before is discarded on Line 63, so the test only proves the post-transformSync() dump is still well-formed. That will not catch a regression where the allocator activity no longer changes the snapshot in the way this test name/comments claim. Either assert against before or rename/remove the dead baseline so the test contract matches what is actually being verified.

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

In `@test/js/bun/jsc/heapStats-mimalloc.test.ts` around lines 48 - 64, The test
"dump reflects new heaps and allocations" records a baseline in the variable
before but never uses it; fix by asserting the after dump against that baseline
(e.g., change the assertion to compare after.length to before via
expect(after.length).toBeGreaterThanOrEqual(before)) so heapStats({dump:true})
before and after transformSync() are actually compared, or alternatively remove
the unused before variable and update the test name/comments to match that it
only validates the post-transform dump shape; look for the variables before,
after, heapStats, t.transformSync and the test block to apply the change.

Comment on lines +48 to +64
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.

🟡 The test "dump reflects new heaps and allocations" records a baseline heap count in before but never uses it — the assertion is expect(after.length).toBeGreaterThanOrEqual(1) instead of toBeGreaterThanOrEqual(before), and void before on line 63 explicitly discards the variable. The test name and inline comment both claim a before/after comparison is performed, but the check only verifies the dump still has at least one heap, so a regression where allocations cause heaps to shrink would go undetected as long as one heap remains.

Extended reasoning...

What the bug is and how it manifests

The test "dump reflects new heaps and allocations" (lines 48–64) captures before = heapStats({dump:true}).mimallocDump.heaps.length before triggering allocations via Bun.Transpiler.transformSync, then captures the post-allocation dump in after. The intent — spelled out in both the test name and the inline comment "We assert the dump is still well-formed and >= before" — is to verify that the allocation activity produced at least as many heaps as were present before. However, the actual assertion is expect(after.length).toBeGreaterThanOrEqual(1), which only checks that at least one heap exists, completely ignoring before.

The specific code path that triggers it

Line 49 sets const before = heapStats(...).mimallocDump.heaps.length. Line 58 asserts expect(after.length).toBeGreaterThanOrEqual(1). Line 63 has void before — a deliberate TypeScript idiom for suppressing unused-variable lint warnings — confirming the author knew before was never referenced in any assertion. The comment between lines 56–59 says ">= before" but the code says ">= 1".

Why existing code does not prevent it

There is no mechanism that enforces a captured baseline must appear in an assertion. The test compiles and runs, and will pass as long as the dump returns any heap at all — even if allocations somehow caused the heap count to drop from 50 to 1.

What the impact would be

If a mimalloc regression caused heaps to stop being tracked after allocation (e.g., a refcount bug that destroys heaps prematurely), the heap count in the dump could shrink. The test would still pass as long as one heap remained. The invariant the test claims to enforce — that allocations produce a dump at least as large as the pre-allocation snapshot — is never actually checked.

How to fix it

Change the assertion on line 58 from toBeGreaterThanOrEqual(1) to toBeGreaterThanOrEqual(before) and remove the void before on line 63. Alternatively, if the author intentionally chose not to compare (the comment notes heaps may be destroyed between snapshots), update the test name and comment to reflect what is actually being verified.

Step-by-step proof

  1. Before allocation: heaps.length = 5, so before = 5
  2. After allocation: mimalloc regression causes heaps.length = 1
  3. expect(after.length).toBeGreaterThanOrEqual(1) evaluates 1 >= 1, which passes
  4. The intended check expect(after.length).toBeGreaterThanOrEqual(before) would evaluate 1 >= 5, which would fail and catch the regression
  5. The test passes despite the regression, providing false confidence

});
11 changes: 4 additions & 7 deletions test/js/bun/perf/static-initializers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,12 @@ describe("static initializers", () => {
.map(a => a.trim())
.filter(line => line.includes("running initializer") && line.includes(bunExe()));

// On both architectures, we have one initializer "__GLOBAL__sub_I_static.c".
// On arm64, mimalloc v3 adds one more static initializer (total: 2).
// On x86_64, we also have:
// - one from ___cpu_indicator_init due to our CPU feature detection
// - one from mimalloc v3
// (total: 3)
// mimalloc v3 with MI_OSX_ZONE=ON contributes three: __GLOBAL__sub_I_static.c,
// mi_process_attach, and _mi_macos_override_malloc (the zone-swap constructor).
// On x86_64 there is also ___cpu_indicator_init from CPU feature detection.
expect(
bunInitializers.length,
`Do not add static initializers to Bun. Static initializers are called when Bun starts up, regardless of whether you use the variables or not. This makes Bun slower.`,
).toBe(process.arch === "arm64" ? 1 : 2);
).toBe(process.arch === "arm64" ? 3 : 4);
});
});
Loading