Fix mi_cfree function missing certain big allocations - #915
Fix mi_cfree function missing certain big allocations#915Dmitry Yanovsky (kerambyte) wants to merge 1 commit into
Conversation
The way `mi_is_in_heap_region` is implemented right now (see [this](https://github.com/microsoft/mimalloc/blob/master/src/free.c#L112) code) it is possible to 'miss' certain huge allocations going through `mi_cfree`. So we either need to update the segment detection code (so that it is not limited to [e.g. 2GiB on 32-bit platforms](https://github.com/microsoft/mimalloc/blob/master/src/segment-map.c#L24)) or use the more expensive `mi_check_owned` check if we've failed the simpler heap check.
|
Ah, looks like the 32-bit 'address-too-high' issue has been fixed somewhat recently - a964322 So perhaps this issue is no more. We've seen cases where on 32-bit platforms with really big allocations sometimes we'd get a segment address bigger than 2GiB and then that segment couldn't be 'found'. I'll leave it to you to decide whether that commit fixes the issue or not, feel free to close this PR if it does. 👍 |
xhon-pelushi
left a comment
There was a problem hiding this comment.
Dmitry Yanovsky (@kerambyte) asked in the comment below whether a964322a already fixed this and left the decision open. That question has been sitting unanswered for two years, so here's an attempt to settle it: yes, and it landed six weeks before this PR was opened.
The 2 GiB limit was removed by the commit you found
a964322a ("revise the segment map to only apply to OS allocated segments and reduce the .BSS footprint", 2024-06-02) deleted exactly the constant you were hitting:
-#define MI_MAX_ADDRESS ((size_t)2 << 30) // 2Gb
+#define MI_SEGMENT_MAP_MAX_ADDRESS (MAX_UINT32)So on 32-bit the map went from covering 2 GiB to covering the entire 32-bit address space, and on 64-bit from 40 TiB to 48 TiB. That matches your report of "a segment address bigger than 2GiB [that] couldn't be found" precisely — same threshold, same symptom.
The same commit also changed how the lookup works, which matters more than the constant. mi_is_in_heap_region is now:
// src/segment-map.c:131
static bool mi_is_valid_pointer(const void* p) {
// first check if it is in an arena, then check if it is OS allocated
return (_mi_arena_contains(p) || _mi_segment_of(p) != NULL);
}Arena-backed memory is found through the arena, so for that memory the segment map's address range isn't consulted at all. Only OS-allocated segments depend on the map.
Two further rounds have hardened the 32-bit path since, which is the platform your report came from:
| commit | date | what |
|---|---|---|
3c13579fc |
2025-02-18 | fix pre-processor overflow (issue #1017) |
aed71f8b3 |
2025-02-20 | prevent segment map overflow on arm32 (issue #1017) |
c9b9c8c50 |
2026-04-29 | always perform a cookie check when using _mi_segment_of |
#1017 was a real overflow in the parts-based scheme a964322a introduced, i.e. a second bite at the same 32-bit problem, fixed at the root.
Empirically, nothing is missed on current main
Built main (c683b7c, v2.4.5) on x86-64 Linux and checked every allocation against both predicates:
8 bytes in_heap_region=1 check_owned=1
1024 bytes in_heap_region=1 check_owned=1
65536 bytes in_heap_region=1 check_owned=1
1048576 bytes in_heap_region=1 check_owned=1
4194304 bytes in_heap_region=1 check_owned=1
67108864 bytes in_heap_region=1 check_owned=1
268435456 bytes in_heap_region=1 check_owned=1
1073741824 bytes in_heap_region=1 check_owned=1
3221225472 bytes in_heap_region=1 check_owned=1
missed by in_heap_region: 0 of 9
ctest on that build is 4/4 passing.
If the check is kept anyway, two things worth knowing
It's safe, which was my first worry. mi_cfree is interposed as free and vfree on macOS (alloc-override.c:90-91), so it receives genuinely foreign pointers, and a check that dereferenced them would be a crash rather than a leak. It doesn't: mi_heap_check_owned rejects misaligned pointers up front and then only compares p against each of the heap's own page ranges in mi_heap_page_check_owned. It never touches *p.
But it wouldn't rescue the case it's aimed at. mi_check_owned consults only the calling thread's default heap:
bool mi_check_owned(const void* p) {
return mi_heap_check_owned(mi_prim_get_default_heap(), p);
}mimalloc supports freeing a block from a thread other than the one that allocated it, and for that case the fallback returns false:
allocated on thread A, checked from main thread:
small: in_heap_region=1 check_owned=0
huge : in_heap_region=1 check_owned=0
So a huge allocation made on another thread would still be missed if in_heap_region ever failed on it — which is exactly the shape of the original report. As written, the || only helps single-threaded ownership.
For completeness, the cost is modest rather than alarming — on the foreign-pointer path, with a warm heap:
live blocks=0 in_heap_region= 5 ns/call check_owned=17 ns/call
live blocks=20000 in_heap_region= 5 ns/call check_owned=21 ns/call
live blocks=200000 in_heap_region= 3 ns/call check_owned=12 ns/call
~3–5× on a few nanoseconds, and it didn't degrade with live-block count in my runs (page count grows much more slowly than block count). Real but small, on every foreign free under the macOS interposition.
Suggestion
Unless someone can still reproduce the miss, this looks superseded by a964322a plus the #1017 fixes, and closing seems right — Dmitry Yanovsky (@kerambyte) effectively said as much on the day they opened it. If a maintainer would rather keep a belt-and-braces check, the cross-thread gap above should be addressed or at least commented, so it isn't later mistaken for a complete ownership test.
Worth noting the decision only affects the v2 line: on main3 this code has moved on. mi_cfree now lives in src/free.c:352 and returns bool, src/segment-map.c is gone in favour of src/page-map.c, and include/mimalloc/types.h records that "free mi_cfree can still use the page map to validate pointers".
What I couldn't test
Everything above is x86-64 Linux. I have no 32-bit or arm32 toolchain here and didn't test macOS, so I can't directly re-run the platform where the original miss occurred — the 32-bit conclusion rests on reading a964322a and the #1017 commits rather than on reproducing them. Address layouts above 48 TiB are also untested.
The way
mi_is_in_heap_regionis implemented right now (see this code) it is possible to 'miss' certain huge allocations going throughmi_cfree. So we either need to update the segment detection code (so that it is not limited to e.g. 2GiB on 32-bit platforms) or use the more expensivemi_check_ownedcheck if we've failed the simpler heap check.