From b549c88e6c85d4a1057a9ccf15ac35ebf38e9366 Mon Sep 17 00:00:00 2001 From: daanx Date: Tue, 18 Apr 2023 19:48:40 -0700 Subject: [PATCH 1/5] review realloc --- src/alloc.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/alloc.c b/src/alloc.c index da647ca6..3ee4dad1 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -700,14 +700,12 @@ void* _mi_heap_realloc_zero(mi_heap_t* heap, void* p, size_t newsize, bool zero) if (zero && newsize > size) { // also set last word in the previous allocation to zero to ensure any padding is zero-initialized const size_t start = (size >= sizeof(intptr_t) ? size - sizeof(intptr_t) : 0); - memset((uint8_t*)newp + start, 0, newsize - start); + _mi_memzero((uint8_t*)newp + start, newsize - start); } if mi_likely(p != NULL) { - if mi_likely(_mi_is_aligned(p, sizeof(uintptr_t))) { // a client may pass in an arbitrary pointer `p`.. - const size_t copysize = (newsize > size ? size : newsize); - mi_track_mem_defined(p,copysize); // _mi_useable_size may be too large for byte precise memory tracking.. - _mi_memcpy_aligned(newp, p, copysize); - } + const size_t copysize = (newsize > size ? size : newsize); + mi_track_mem_defined(p,copysize); // _mi_useable_size may be too large for byte precise memory tracking.. + _mi_memcpy(newp, p, copysize); mi_free(p); // only free the original pointer if successful } } From 4cb5b45178835c9df706194edf837d02a64efc6f Mon Sep 17 00:00:00 2001 From: daanx Date: Fri, 21 Apr 2023 09:37:05 -0700 Subject: [PATCH 2/5] fix possible underflow (issue #731) --- src/segment.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/segment.c b/src/segment.c index 41c28065..359bd258 100644 --- a/src/segment.c +++ b/src/segment.c @@ -1273,9 +1273,12 @@ void _mi_segment_huge_page_reset(mi_segment_t* segment, mi_page_t* page, mi_bloc mi_assert_internal(page->used == 1); // this is called just before the free mi_assert_internal(page->free == NULL); if (segment->allow_decommit && page->is_committed) { - const size_t usize = mi_usable_size(block) - sizeof(mi_block_t); - uint8_t* p = (uint8_t*)block + sizeof(mi_block_t); - _mi_os_reset(p, usize, &_mi_stats_main); + size_t usize = mi_usable_size(block); + if (usize > sizeof(mi_block_t)) { + usize = usize - sizeof(mi_block_t); + uint8_t* p = (uint8_t*)block + sizeof(mi_block_t); + _mi_os_reset(p, usize, &_mi_stats_main); + } } } #endif From 3bc577004ad0ad348f69e8926244d2b9d69b1863 Mon Sep 17 00:00:00 2001 From: daanx Date: Fri, 21 Apr 2023 09:37:25 -0700 Subject: [PATCH 3/5] clarify return codes of VirtualAlloc (issue #731) --- src/prim/windows/prim.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/prim/windows/prim.c b/src/prim/windows/prim.c index 1544c641..61532737 100644 --- a/src/prim/windows/prim.c +++ b/src/prim/windows/prim.c @@ -261,7 +261,7 @@ int _mi_prim_alloc(size_t size, size_t try_alignment, bool commit, bool allow_la int _mi_prim_commit(void* addr, size_t size) { void* p = VirtualAlloc(addr, size, MEM_COMMIT, PAGE_READWRITE); - return (p == addr ? 0 : (int)GetLastError()); + return (p != NULL ? 0 : (int)GetLastError()); } int _mi_prim_decommit(void* addr, size_t size, bool* needs_recommit) { @@ -274,11 +274,11 @@ int _mi_prim_reset(void* addr, size_t size) { void* p = VirtualAlloc(addr, size, MEM_RESET, PAGE_READWRITE); mi_assert_internal(p == addr); #if 1 - if (p == addr && addr != NULL) { + if (p != NULL) { VirtualUnlock(addr,size); // VirtualUnlock after MEM_RESET removes the memory from the working set } #endif - return (p == addr ? 0 : (int)GetLastError()); + return (p != NULL ? 0 : (int)GetLastError()); } int _mi_prim_protect(void* addr, size_t size, bool protect) { From 012f7164851acedfb9ae20d2909c7d402b2f4974 Mon Sep 17 00:00:00 2001 From: daanx Date: Fri, 21 Apr 2023 10:37:22 -0700 Subject: [PATCH 4/5] add is_zero flag to prim_commit --- include/mimalloc/prim.h | 3 ++- src/os.c | 11 ++++++++--- src/prim/unix/prim.c | 3 ++- src/prim/wasi/prim.c | 3 ++- src/prim/windows/prim.c | 18 +++++++++++++++--- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/include/mimalloc/prim.h b/include/mimalloc/prim.h index 40f5d2d7..9e560696 100644 --- a/include/mimalloc/prim.h +++ b/include/mimalloc/prim.h @@ -47,7 +47,8 @@ int _mi_prim_alloc(size_t size, size_t try_alignment, bool commit, bool allow_la // Commit memory. Returns error code or 0 on success. // For example, on Linux this would make the memory PROT_READ|PROT_WRITE. -int _mi_prim_commit(void* addr, size_t size); +// `is_zero` is set to true if the memory was zero initialized (e.g. on Windows) +int _mi_prim_commit(void* addr, size_t size, bool* is_zero); // Decommit memory. Returns error code or 0 on success. The `needs_recommit` result is true // if the memory would need to be re-committed. For example, on Windows this is always true, diff --git a/src/os.c b/src/os.c index f243f7a4..1b435335 100644 --- a/src/os.c +++ b/src/os.c @@ -405,12 +405,17 @@ bool _mi_os_commit(void* addr, size_t size, bool* is_zero, mi_stats_t* tld_stats if (csize == 0) return true; // commit - int err = _mi_prim_commit(start, csize); + bool os_is_zero = false; + int err = _mi_prim_commit(start, csize, &os_is_zero); if (err != 0) { _mi_warning_message("cannot commit OS memory (error: %d (0x%x), address: %p, size: 0x%zx bytes)\n", err, err, start, csize); + return false; } - mi_assert_internal(err == 0); - return (err == 0); + if (os_is_zero && is_zero != NULL) { + *is_zero = true; + mi_assert_expensive(mi_mem_is_zero(start, csize)); + } + return true; } static bool mi_os_decommit_ex(void* addr, size_t size, bool* needs_recommit, mi_stats_t* tld_stats) { diff --git a/src/prim/unix/prim.c b/src/prim/unix/prim.c index 4349f578..9a542d3e 100644 --- a/src/prim/unix/prim.c +++ b/src/prim/unix/prim.c @@ -346,8 +346,9 @@ static void unix_mprotect_hint(int err) { -int _mi_prim_commit(void* start, size_t size) { +int _mi_prim_commit(void* start, size_t size, bool* is_zero) { // commit: ensure we can access the area + *is_zero = false; int err = mprotect(start, size, (PROT_READ | PROT_WRITE)); if (err != 0) { err = errno; } unix_mprotect_hint(err); diff --git a/src/prim/wasi/prim.c b/src/prim/wasi/prim.c index bf78a258..50511f0b 100644 --- a/src/prim/wasi/prim.c +++ b/src/prim/wasi/prim.c @@ -128,8 +128,9 @@ int _mi_prim_alloc(size_t size, size_t try_alignment, bool commit, bool allow_la // Commit/Reset/Protect //--------------------------------------------- -int _mi_prim_commit(void* addr, size_t size) { +int _mi_prim_commit(void* addr, size_t size, bool* is_zero) { MI_UNUSED(addr); MI_UNUSED(size); + *is_zero = false; return 0; } diff --git a/src/prim/windows/prim.c b/src/prim/windows/prim.c index 61532737..bde48a7d 100644 --- a/src/prim/windows/prim.c +++ b/src/prim/windows/prim.c @@ -259,14 +259,26 @@ int _mi_prim_alloc(size_t size, size_t try_alignment, bool commit, bool allow_la #pragma warning(disable:6250) // suppress warning calling VirtualFree without MEM_RELEASE (for decommit) #endif -int _mi_prim_commit(void* addr, size_t size) { +int _mi_prim_commit(void* addr, size_t size, bool* is_zero) { + *is_zero = false; + /* + // zero'ing only happens on an initial commit... but checking upfront seems expensive.. + _MEMORY_BASIC_INFORMATION meminfo; _mi_memzero_var(meminfo); + if (VirtualQuery(addr, &meminfo, size) > 0) { + if ((meminfo.State & MEM_COMMIT) == 0) { + *is_zero = true; + } + } + */ + // commit void* p = VirtualAlloc(addr, size, MEM_COMMIT, PAGE_READWRITE); - return (p != NULL ? 0 : (int)GetLastError()); + if (p == NULL) return (int)GetLastError(); + return 0; } int _mi_prim_decommit(void* addr, size_t size, bool* needs_recommit) { BOOL ok = VirtualFree(addr, size, MEM_DECOMMIT); - *needs_recommit = true; // for safetly, assume always decommitted even in the case of an error. + *needs_recommit = true; // for safety, assume always decommitted even in the case of an error. return (ok ? 0 : (int)GetLastError()); } From e47adc2d22ddcf9201c4110bc7c87bbb4cd000e2 Mon Sep 17 00:00:00 2001 From: daanx Date: Fri, 21 Apr 2023 11:33:50 -0700 Subject: [PATCH 5/5] track objects in heap destroy for ETW --- include/mimalloc/track.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mimalloc/track.h b/include/mimalloc/track.h index f78e8daa..9545f750 100644 --- a/include/mimalloc/track.h +++ b/include/mimalloc/track.h @@ -79,7 +79,7 @@ defined, undefined, or not accessible at all: // windows event tracing #define MI_TRACK_ENABLED 1 -#define MI_TRACK_HEAP_DESTROY 0 +#define MI_TRACK_HEAP_DESTROY 1 #define MI_TRACK_TOOL "ETW" #define WIN32_LEAN_AND_MEAN