fix(windows): implement bmalloc SystemHeap using mimalloc to fix GC crashes#28249
fix(windows): implement bmalloc SystemHeap using mimalloc to fix GC crashes#28249robobun wants to merge 3 commits into
Conversation
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughAdds a Windows-specific mimalloc-backed SystemHeap implementation and enables it at process startup by setting Malloc=1 via _putenv; documents why Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/BmallocSystemHeapOverride.cpp`:
- Around line 38-40: Add instrumentation parity with WebKit by including
<bmalloc/pas_mte.h> and wrapping all pas_system_heap allocation calls with
PAS_PROFILE(SYSTEM_HEAP_ALLOCATION, ...) and PAS_MTE_HANDLE before delegating to
SystemHeap::getExisting()->...; specifically update the functions
pas_system_heap_malloc, pas_system_heap_memalign, pas_system_heap_realloc and
their pas_system_heap_*_compact variants so each allocation/deallocation call is
wrapped with PAS_PROFILE and PAS_MTE_HANDLE to restore profiling/tagging
behavior.
In `@src/main.zig`:
- Around line 32-34: The call to _putenv("Malloc=1") inside the
Environment.isWindows branch currently discards the return value, allowing
silent failure; change this to check the return value from _putenv and handle
failure (e.g., log an error and exit or return an error) so that Malloc=1 is
guaranteed to be visible to convertEnvToWTF8 which reads GetEnvironmentStringsW;
update the Windows initialization flow that calls _putenv in src/main.zig to
detect a non‑zero/-1 return and fail fast (or clearly document and assert why
silent failure is acceptable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb6b84d4-2e69-4f9f-86b2-a58c25a8c173
📒 Files selected for processing (4)
cmake/targets/BuildBun.cmakesrc/bun.js/bindings/BmallocSystemHeapOverride.cppsrc/c-headers-for-zig.hsrc/main.zig
| #if BENABLE(LIBPAS) | ||
| #include <bmalloc/pas_system_heap.h> | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BmallocSystemHeapOverride.cpp" -type fRepository: oven-sh/bun
Length of output: 106
🏁 Script executed:
find . -name "SystemHeap.cpp" -type fRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
wc -l ./src/bun.js/bindings/BmallocSystemHeapOverride.cppRepository: oven-sh/bun
Length of output: 110
🏁 Script executed:
cat -n ./src/bun.js/bindings/BmallocSystemHeapOverride.cppRepository: oven-sh/bun
Length of output: 8808
🌐 Web query:
site:raw.githubusercontent.com WebKit SystemHeap.cpp pas_system_heap_malloc PAS_PROFILE
💡 Result:
In WebKit’s Source/bmalloc/bmalloc/SystemHeap.cpp (commit 1034e0634fd9bc7f1413cea076d0536b0e7cf87c), the libpas glue defines pas_system_heap_malloc() and instruments it with PAS_PROFILE before delegating to SystemHeap:
pas_system_heap_malloc(size_t size):- gets
auto systemHeap = SystemHeap::getExisting(); - calls
PAS_PROFILE(SYSTEM_HEAP_ALLOCATION, systemHeap, size, 0, pas_non_compact_allocation_mode); - returns
systemHeap->malloc(size, FailureAction::ReturnNull);[1]
- gets
The corresponding declarations live in Source/bmalloc/libpas/src/libpas/pas_system_heap.h (same commit), declaring pas_system_heap_malloc, pas_system_heap_memalign, etc., as BEXPORT extern ... when PAS_BMALLOC is enabled. [2]
Sources: [1], [2]
🌐 Web query:
site:raw.githubusercontent.com WebKit SystemHeap.cpp PAS_MTE_HANDLE
💡 Result:
In WebKit’s repo, PAS_MTE_HANDLE appears in Source/bmalloc/bmalloc/SystemHeap.cpp (the pas_system_heap_* allocation wrappers), e.g. these functions call it:
pas_system_heap_mallocpas_system_heap_memalignpas_system_heap_reallocpas_system_heap_malloc_compactpas_system_heap_memalign_compactpas_system_heap_realloc_compact[1]
Raw files:
https://raw.githubusercontent.com/WebKit/WebKit/main/Source/bmalloc/bmalloc/SystemHeap.cpp
https://raw.githubusercontent.com/WebKit/WebKit/main/Source/bmalloc/bmalloc/SystemHeap.h
``` [1][2]
**Sources:** [1] [2]
---
</details>
**Mirror the PAS_PROFILE and PAS_MTE_HANDLE instrumentation from WebKit's canonical SystemHeap.cpp.**
This override currently acts as a symbol-only replacement without the profiling/tagging behavioral parity. WebKit's `SystemHeap.cpp` wraps all `pas_system_heap_*` allocations with `PAS_PROFILE(SYSTEM_HEAP_ALLOCATION, ...)` and `PAS_MTE_HANDLE` before delegating. On Windows LIBPAS builds, dropping these hooks means profiling and tagging data is incomplete.
Add `#include <bmalloc/pas_mte.h>` at line 39, then wrap the allocation calls in all six functions (`pas_system_heap_malloc`, `memalign`, `realloc`, and their `_compact` variants at lines 174–207) with both profiling hooks before the `SystemHeap::getExisting()->` delegation to maintain instrumentation consistency.
<details>
<summary>♻️ Suggested parity patch</summary>
```diff
`#if` BENABLE(LIBPAS)
+#include <bmalloc/pas_mte.h>
`#include` <bmalloc/pas_system_heap.h>
`#endif`
@@
void* pas_system_heap_malloc(size_t size)
{
- return SystemHeap::getExisting()->malloc(size, FailureAction::ReturnNull);
+ auto systemHeap = SystemHeap::getExisting();
+ PAS_PROFILE(SYSTEM_HEAP_ALLOCATION, systemHeap, size, 0, pas_non_compact_allocation_mode);
+ PAS_MTE_HANDLE(SYSTEM_HEAP_ALLOCATION, systemHeap, size, 0, pas_non_compact_allocation_mode);
+ return systemHeap->malloc(size, FailureAction::ReturnNull);
}
Apply the same pattern to memalign, realloc, and their *_compact variants.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/BmallocSystemHeapOverride.cpp` around lines 38 - 40, Add
instrumentation parity with WebKit by including <bmalloc/pas_mte.h> and wrapping
all pas_system_heap allocation calls with PAS_PROFILE(SYSTEM_HEAP_ALLOCATION,
...) and PAS_MTE_HANDLE before delegating to SystemHeap::getExisting()->...;
specifically update the functions pas_system_heap_malloc,
pas_system_heap_memalign, pas_system_heap_realloc and their
pas_system_heap_*_compact variants so each allocation/deallocation call is
wrapped with PAS_PROFILE and PAS_MTE_HANDLE to restore profiling/tagging
behavior.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (Environment.isWindows) { | ||
| _ = _bun.c._putenv("Malloc=1"); | ||
| } |
There was a problem hiding this comment.
🟡 The _putenv("Malloc=1") call added to main.zig (line 33) to force bmalloc to route all allocations through SystemHeap/mimalloc is missing from the test entry point src/main_test.zig. The test binary's Windows init block (lines 25-34) has the same uv_replace_allocator and environ setup but lacks this call, so the test build won't get the full libpas bypass on Windows. Add _ = bun.c._putenv("Malloc=1"); before the uv_replace_allocator call in main_test.zig.
Extended reasoning...
What the bug is
This PR adds _putenv("Malloc=1") to src/main.zig to force bmalloc to use SystemHeap (backed by mimalloc) instead of libpas on Windows. However, Bun has two entry points: src/main.zig for the main binary and src/main_test.zig for the test binary (bun-*-test). The fix was only applied to the main entry point.
The specific code path
In main_test.zig:22-34, the main() function has a Windows initialization block that calls uv_replace_allocator and sets up environ/_environ, mirroring the same block in main.zig. The new _putenv("Malloc=1") call was added to main.zig before this block but was not replicated in main_test.zig.
Why existing code doesn't prevent it
The two entry points are completely separate files with duplicated initialization logic. Changes to one are not automatically reflected in the other. The PR description and commit only modified main.zig, leaving main_test.zig untouched.
Impact
Without Malloc=1 being set, bmalloc::Environment::isSystemHeapEnabled() returns false in the test binary, so libpas remains the primary allocator rather than being bypassed through SystemHeap/mimalloc. The crash itself is still prevented because BmallocSystemHeapOverride.cpp provides working SystemHeap implementations (so when SystemHeap IS invoked during GC, it uses mimalloc instead of crashing). However, the test binary does not get the full allocation routing through mimalloc that the PR intends.
Step-by-step proof
- Build the test binary on Windows (cmake with
-DTEST=ON), which usesmain_test.zigas the entry point. - The test binary starts, enters
main()atmain_test.zig:22. - The Windows block at line 25 runs
uv_replace_allocatorand sets environ, but never calls_putenv("Malloc=1"). - When bmalloc's
Environmentinitializes, it checks for theMallocenv var, finds it unset, and does NOT enable SystemHeap for all allocations. - libpas remains the primary allocator in the test binary, unlike the main binary where
Malloc=1causes all allocations to route through SystemHeap/mimalloc.
How to fix
Add _ = bun.c._putenv("Malloc=1"); at the beginning of the Windows block in main_test.zig, before the uv_replace_allocator call, matching what was done in main.zig.
…rashes bmalloc's SystemHeap on Windows is unimplemented — all allocation methods crash with RELEASE_BASSERT_NOT_REACHED. When libpas triggers SystemHeap allocations during GC cycles under memory pressure, this causes segfaults in JSC's Structure/WriteBarrier code paths. This adds a working SystemHeap implementation backed by mimalloc that overrides bmalloc.lib's crashing stubs at link time. Combined with setting Malloc=1 at startup to force bmalloc to route through SystemHeap, this bypasses libpas for all allocations on Windows. Fixes #22349 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
robobun
force-pushed
the
claude/fix-windows-bmalloc-systemheap-22349
branch
from
dd6b2d3 to
77922fd
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Re: CodeRabbit review comments: PAS_PROFILE/PAS_MTE_HANDLE (BmallocSystemHeapOverride.cpp): Intentionally omitted. These macros instrument Apple's Memory Tagging Extension and allocation profiling — neither is relevant on Windows. The upstream WebKit code only uses them in the Darwin path. Adding them here would add dead code with no benefit. _putenv return value (main.zig): Good catch. Added an assert so failure is caught immediately rather than silently deferring to a crash later when bmalloc tries to use the unimplemented libpas path. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 `@src/bun.js/bindings/BmallocSystemHeapOverride.cpp`:
- Around line 108-118: In SystemHeap::freeLarge, using m_sizeMap[base] silently
inserts a zero-sized entry on misses causing vmDeallocate(base, 0) and potential
corruption; change the lookup to use m_sizeMap.find(base), RELEASE_BASSERT that
the iterator is not end(), read size from it->second, then erase the entry via
m_sizeMap.erase(it) before calling vmDeallocate; ensure RELEASE_BASSERT
references the find result so we only deallocate tracked allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 983b1a5f-b3c6-4c3c-a002-18fce3f42fca
📒 Files selected for processing (4)
cmake/targets/BuildBun.cmakesrc/bun.js/bindings/BmallocSystemHeapOverride.cppsrc/c-headers-for-zig.hsrc/main.zig
| void SystemHeap::freeLarge(void* base) | ||
| { | ||
| if (!base) | ||
| return; | ||
|
|
||
| size_t size; | ||
| { | ||
| LockHolder locker(mutex()); | ||
| size = m_sizeMap[base]; | ||
| size_t numErased = m_sizeMap.erase(base); | ||
| RELEASE_BASSERT(numErased == 1); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
header="$(fd 'SystemHeap\.h$' . | head -n1)"
test -n "$header"
echo "== inspecting ${header} =="
rg -n -C5 '\bclass SystemHeap\b|\bm_sizeMap\b|\btake\s*\(|\bfind\s*\(|\berase\s*\(' "$header"Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, let's check the .cpp file directly
cpp_file="src/bun.js/bindings/BmallocSystemHeapOverride.cpp"
if [ ! -f "$cpp_file" ]; then
echo "File not found at expected location"
exit 1
fi
echo "=== Examining the .cpp file for includes and m_sizeMap usage ==="
head -n 150 "$cpp_file"Repository: oven-sh/bun
Length of output: 4000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for SystemHeap.h in the repository
echo "=== Looking for SystemHeap.h ==="
find . -name "SystemHeap.h" -type f 2>/dev/null | head -20
# Also search for bmalloc headers
echo -e "\n=== Looking for bmalloc directory ==="
find . -type d -name "bmalloc" 2>/dev/null | head -10Repository: oven-sh/bun
Length of output: 126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any bmalloc references or comments
echo "=== Searching for bmalloc references in the repo ==="
rg -l "bmalloc" --type-list 2>/dev/null | head -10
# Look for any comments or documentation mentioning SystemHeap
echo -e "\n=== Searching for SystemHeap references ==="
rg "SystemHeap" --type-list 2>/dev/null | head -5
# Search within the cpp file for m_sizeMap or map container hints
echo -e "\n=== Searching for map/HashMap patterns in bindings directory ==="
fd "\.h$|\.cpp$" src/bun.js/bindings/ | xargs rg "sizeMap|HashMap|map<" 2>/dev/null | head -20Repository: oven-sh/bun
Length of output: 107
🌐 Web query:
bmalloc SystemHeap m_sizeMap container type find method
💡 Result:
In WebKit’s bmalloc::SystemHeap, m_sizeMap is a C++ standard hash table:
- Container type:
std::unordered_map<void*, size_t> m_sizeMap;(maps the base pointer of a “large” VM allocation to its size). [1]
To look up an entry, use std::unordered_map::find:
auto it = m_sizeMap.find(base);
if (it != m_sizeMap.end()) {
size_t size = it->second;
}(Internally, the code also uses erase(base) when freeing a large allocation.) [2]
Sources:
[1] WebKit SystemHeap.h (declaration of m_sizeMap)
[2] WebKit SystemHeap.cpp (use of m_sizeMap for large alloc/free)
Use find() instead of operator[] to avoid silent insertion on lookup miss.
m_sizeMap[base] creates a default entry {base, 0} if base is not in the map. This means if a large allocation was never tracked (bug elsewhere), erase(base) still succeeds and returns 1, the assertion passes, but size is 0. This causes vmDeallocate(base, 0) to be called with an incorrect size, leading to memory corruption.
Replace with:
auto it = m_sizeMap.find(base);
RELEASE_BASSERT(it != m_sizeMap.end());
size_t size = it->second;
m_sizeMap.erase(it);This ensures the allocation was actually tracked before proceeding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun.js/bindings/BmallocSystemHeapOverride.cpp` around lines 108 - 118, In
SystemHeap::freeLarge, using m_sizeMap[base] silently inserts a zero-sized entry
on misses causing vmDeallocate(base, 0) and potential corruption; change the
lookup to use m_sizeMap.find(base), RELEASE_BASSERT that the iterator is not
end(), read size from it->second, then erase the entry via m_sizeMap.erase(it)
before calling vmDeallocate; ensure RELEASE_BASSERT references the find result
so we only deallocate tracked allocations.
|
Re: CodeRabbit review on freeLarge (BmallocSystemHeapOverride.cpp:118): This is intentional — the code matches upstream WebKit's |
|
Closing as duplicate of #28230 — nearly identical |
1 participant
Summary
SystemHeapimplementation for Windows backed by mimalloc (BmallocSystemHeapOverride.cpp)Malloc=1environment variable at startup to force bmalloc to route all allocations through SystemHeap#include <stdlib.h>to Windows C headers for_putenvaccessRoot cause
bmalloc's
SystemHeapon Windows is completely unimplemented — every allocation method (malloc,memalign,realloc,free) callsRELEASE_BASSERT_NOT_REACHED(). When libpas/bmalloc triggersSystemHeapallocations during GC cycles under memory pressure, this causes segmentation faults that manifest in JSC'sStructure::maxOffset→WriteBarrierBase::setMayBeNull→StructureCache::createEmptyStructurecode path.This was originally fixed in #20931 by removing
bmalloc.libfrom Windows link targets. However, the WebKit update in #26381 re-addedbmalloc.lib, reintroducing the crash.bmalloc.libcannot simply be removed becauseWTF.libreferences bmalloc symbols.How the fix works
BmallocSystemHeapOverride.cppdefines the same symbols as bmalloc.lib'sSystemHeap.cpp.o, using mimalloc instead of the crashing stubs. Since object files are processed before static libraries, the linker uses these definitions.main.zigsetsMalloc=1via_putenvbefore any bmalloc allocation, causingbmalloc::Environmentto enableSystemHeapfor all allocations.Test plan
#if BOS(WINDOWS)/Environment.isWindowsguards)Closes #22349
🤖 Generated with Claude Code