From 4c9213887b31267c741a808d7e7e4cb681ffb936 Mon Sep 17 00:00:00 2001 From: Nathan Moinvaziri Date: Thu, 22 Aug 2019 14:47:08 -0700 Subject: [PATCH 1/9] Fixed compiler warning about converting from bool to BOOL (performance warning) --- src/os.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/os.c b/src/os.c index bcce5d7d..09aa8061 100644 --- a/src/os.c +++ b/src/os.c @@ -123,14 +123,14 @@ void _mi_os_init(void) { // Set "Lock pages in memory" permission in the group policy editor // HANDLE token = NULL; - ok = OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token); + ok = OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &token) != 0; if (ok) { TOKEN_PRIVILEGES tp; - ok = LookupPrivilegeValue(NULL, TEXT("SeLockMemoryPrivilege"), &tp.Privileges[0].Luid); + ok = LookupPrivilegeValue(NULL, TEXT("SeLockMemoryPrivilege"), &tp.Privileges[0].Luid) != 0; if (ok) { tp.PrivilegeCount = 1; tp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED; - ok = AdjustTokenPrivileges(token, FALSE, &tp, 0, (PTOKEN_PRIVILEGES)NULL, 0); + ok = AdjustTokenPrivileges(token, FALSE, &tp, 0, (PTOKEN_PRIVILEGES)NULL, 0) != 0; if (ok) { err = GetLastError(); ok = (err == ERROR_SUCCESS); From b7e506ad9d615694ca7d58783d3c63a8cea5741c Mon Sep 17 00:00:00 2001 From: daan Date: Tue, 3 Sep 2019 19:33:38 -0700 Subject: [PATCH 2/9] fix for incorrect region count --- src/memory.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/memory.c b/src/memory.c index 222b87c2..0fe3594c 100644 --- a/src/memory.c +++ b/src/memory.c @@ -152,15 +152,12 @@ static bool mi_region_commit_blocks(mem_region_t* region, size_t idx, size_t bit else { // failed, another thread allocated just before us! // we assign it to a later slot instead (up to 4 tries). - // note: we don't need to increment the region count, this will happen on another allocation for(size_t i = 1; i <= 4 && idx + i < MI_REGION_MAX; i++) { - void* s = mi_atomic_read_ptr(®ions[idx+i].start); - if (s == NULL) { // quick test - if (mi_atomic_cas_ptr_strong(®ions[idx+i].start, start, NULL)) { - start = NULL; - break; - } - } + if (mi_atomic_cas_ptr_strong(®ions[idx+i].start, start, NULL)) { + mi_atomic_increment(®ions_count); + start = NULL; + break; + } } if (start != NULL) { // free it if we didn't succeed to save it to some other region From e302737830bfbf9ed7308ad9eb0cd8594ce64f56 Mon Sep 17 00:00:00 2001 From: daan Date: Wed, 4 Sep 2019 12:14:59 -0700 Subject: [PATCH 3/9] reserve huge pages returns actual number of pages reserved --- include/mimalloc.h | 2 +- src/init.c | 2 +- src/os.c | 21 +++++++++++++-------- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/include/mimalloc.h b/include/mimalloc.h index 4e291c65..78921a98 100644 --- a/include/mimalloc.h +++ b/include/mimalloc.h @@ -195,7 +195,7 @@ typedef bool (mi_cdecl mi_block_visit_fun)(const mi_heap_t* heap, const mi_heap_ mi_decl_export bool mi_heap_visit_blocks(const mi_heap_t* heap, bool visit_all_blocks, mi_block_visit_fun* visitor, void* arg); mi_decl_export bool mi_is_in_heap_region(const void* p) mi_attr_noexcept; -mi_decl_export int mi_reserve_huge_os_pages(size_t pages, double max_secs) mi_attr_noexcept; +mi_decl_export int mi_reserve_huge_os_pages(size_t pages, double max_secs, size_t* pages_reserved) mi_attr_noexcept; // ------------------------------------------------------ // Convenience diff --git a/src/init.c b/src/init.c index 6748e8f1..a0ed491a 100644 --- a/src/init.c +++ b/src/init.c @@ -429,7 +429,7 @@ static void mi_process_load(void) { if (mi_option_is_enabled(mi_option_reserve_huge_os_pages)) { size_t pages = mi_option_get(mi_option_reserve_huge_os_pages); double max_secs = (double)pages / 2.0; // 0.5s per page (1GiB) - mi_reserve_huge_os_pages(pages, max_secs); + mi_reserve_huge_os_pages(pages, max_secs, NULL); } } diff --git a/src/os.c b/src/os.c index f44b7fbe..2b7ae685 100644 --- a/src/os.c +++ b/src/os.c @@ -788,14 +788,17 @@ static void mi_os_free_huge_reserved() { */ #if !(MI_INTPTR_SIZE >= 8 && (defined(_WIN32) || defined(MI_OS_USE_MMAP))) -int mi_reserve_huge_os_pages(size_t pages, size_t max_secs) { - return -2; // cannot allocate +int mi_reserve_huge_os_pages(size_t pages, double max_secs, size_t* pages_reserved) mi_attr_noexcept { + UNUSED(pages); UNUSED(max_secs); + if (pages_reserved != NULL) *pages_reserved = 0; + return ENOMEM; // cannot allocate } #else -int mi_reserve_huge_os_pages( size_t pages, double max_secs ) mi_attr_noexcept +int mi_reserve_huge_os_pages( size_t pages, double max_secs, size_t* pages_reserved ) mi_attr_noexcept { - if (max_secs==0) return -1; // timeout - if (pages==0) return 0; // ok + if (pages_reserved != NULL) *pages_reserved = 0; + if (max_secs==0) return ETIMEDOUT; // timeout + if (pages==0) return 0; // ok if (!mi_atomic_cas_ptr_strong(&os_huge_reserved.start,(void*)1,NULL)) return -2; // already reserved // Allocate one page at the time but try to place them contiguously @@ -804,7 +807,7 @@ int mi_reserve_huge_os_pages( size_t pages, double max_secs ) mi_attr_noexcept uint8_t* start = (uint8_t*)((uintptr_t)16 << 40); // 16TiB virtual start address uint8_t* addr = start; // current top of the allocations for (size_t page = 0; page < pages; page++, addr += MI_HUGE_OS_PAGE_SIZE ) { - // allocate lorgu pages + // allocate a page void* p = NULL; #ifdef _WIN32 p = mi_win_virtual_alloc(addr, MI_HUGE_OS_PAGE_SIZE, 0, MEM_LARGE_PAGES | MEM_COMMIT | MEM_RESERVE, true); @@ -816,6 +819,7 @@ int mi_reserve_huge_os_pages( size_t pages, double max_secs ) mi_attr_noexcept // Did we succeed at a contiguous address? if (p != addr) { + // no success, issue a warning and return with an error if (p != NULL) { _mi_warning_message("could not allocate contiguous huge page %zu at 0x%p\n", page, addr); _mi_os_free(p, MI_HUGE_OS_PAGE_SIZE, &_mi_stats_main ); @@ -828,7 +832,7 @@ int mi_reserve_huge_os_pages( size_t pages, double max_secs ) mi_attr_noexcept #endif _mi_warning_message("could not allocate huge page %zu at 0x%p, error: %i\n", page, addr, err); } - return -2; + return ENOMEM; } // success, record it if (page==0) { @@ -840,7 +844,8 @@ int mi_reserve_huge_os_pages( size_t pages, double max_secs ) mi_attr_noexcept } _mi_stat_increase(&_mi_stats_main.committed, MI_HUGE_OS_PAGE_SIZE); _mi_stat_increase(&_mi_stats_main.reserved, MI_HUGE_OS_PAGE_SIZE); - + if (pages_reserved != NULL) { *pages_reserved = page + 1; }; + // check for timeout double elapsed = _mi_clock_end(start_t); if (elapsed > max_secs) return (-1); // timeout From f280f14e3115b0f5ff56fc60a959ed8e1295cc85 Mon Sep 17 00:00:00 2001 From: daan Date: Fri, 13 Sep 2019 12:16:40 -0700 Subject: [PATCH 4/9] roll back commit 3d8c331 and start region search from last idx per thread --- src/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/memory.c b/src/memory.c index 0fe3594c..4d3dfe9c 100644 --- a/src/memory.c +++ b/src/memory.c @@ -315,7 +315,7 @@ void* _mi_mem_alloc_aligned(size_t size, size_t alignment, bool commit, size_t* // find a range of free blocks void* p = NULL; size_t count = mi_atomic_read(®ions_count); - size_t idx = 0; // tld->region_idx; // start index is per-thread to reduce contention + size_t idx = tld->region_idx; // start index is per-thread to reduce contention for (size_t visited = 0; visited < count; visited++, idx++) { if (idx >= count) idx = 0; // wrap around if (!mi_region_try_alloc_blocks(idx, blocks, size, commit, &p, id, tld)) return NULL; // error From 7d018dc9e10e7d4a979f7b7199072d6ef30e05ff Mon Sep 17 00:00:00 2001 From: Daan Leijen Date: Fri, 11 Oct 2019 17:03:09 -0700 Subject: [PATCH 5/9] add delayed output buffer --- src/options.c | 45 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/src/options.c b/src/options.c index 09524cb4..5e631b8a 100644 --- a/src/options.c +++ b/src/options.c @@ -140,6 +140,46 @@ static void mi_out_stderr(const char* msg) { #endif } +// Since an output function can be registered earliest in the `main` +// function we also buffer output that happens earlier. When +// an output function is registered it is called immediately with +// the output up to that point. +#define MAX_OUT_BUF (8*1024) +static char out_buf[MAX_OUT_BUF+1]; +static _Atomic(uintptr_t) out_len; + +static void mi_out_buf(const char* msg) { + if (msg==NULL) return; + size_t n = strlen(msg); + if (n==0) return; + // claim + if (mi_atomic_read_relaxed(&out_len)>=MAX_OUT_BUF) return; + uintptr_t start = mi_atomic_addu(&out_len, n); + if (start >= MAX_OUT_BUF) return; + // check bound + if (start+n >= MAX_OUT_BUF) { + n = MAX_OUT_BUF-start-1; + } + memcpy(&out_buf[start], msg, n); +} + +static void mi_out_buf_contents(mi_output_fun* out) { + if (out==NULL) return; + // claim all + size_t count = mi_atomic_addu(&out_len, MAX_OUT_BUF); + // and output it + if (count>MAX_OUT_BUF) count = MAX_OUT_BUF; + out_buf[count] = 0; + out(out_buf); +} + +// The initial default output outputs to stderr and the delayed buffer. +static void mi_out_buf_stderr(const char* msg) { + mi_out_stderr(msg); + mi_out_buf(msg); +} + + // -------------------------------------------------------- // Default output handler // -------------------------------------------------------- @@ -151,11 +191,12 @@ static mi_output_fun* volatile mi_out_default; // = NULL static mi_output_fun* mi_out_get_default(void) { mi_output_fun* out = mi_out_default; - return (out == NULL ? &mi_out_stderr : out); + return (out == NULL ? &mi_out_buf_stderr : out); } void mi_register_output(mi_output_fun* out) mi_attr_noexcept { - mi_out_default = out; + mi_out_default = (out == NULL ? &mi_out_stderr : out); + if (out!=NULL) mi_out_buf_contents(out); } From 2affdbbd2e6c26fba92c380375d2e1f7c8578ffe Mon Sep 17 00:00:00 2001 From: daan Date: Fri, 18 Oct 2019 18:11:04 -0700 Subject: [PATCH 6/9] stronger secure mode when defining MI_SECURE=4: checks for double free, corrupted free list, and invalid pointer frees. Performance is impacted but not too much -- more perf testing is needed --- CMakeLists.txt | 2 +- ide/vs2019/mimalloc.vcxproj | 4 +-- include/mimalloc-internal.h | 45 ++++++++++++++++++++++++---------- include/mimalloc-types.h | 7 ++++-- src/alloc.c | 49 +++++++++++++++++++++++++++++++++++-- src/options.c | 8 ++++++ test/main-override-static.c | 20 +++++++++++++++ test/main-override.cpp | 6 +++++ 8 files changed, 121 insertions(+), 20 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 443476f0..81cc339a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -68,7 +68,7 @@ endif() if(MI_SECURE MATCHES "ON") message(STATUS "Set secure build (MI_SECURE=ON)") - list(APPEND mi_defines MI_SECURE=2) + list(APPEND mi_defines MI_SECURE=3) endif() if(MI_SEE_ASM MATCHES "ON") diff --git a/ide/vs2019/mimalloc.vcxproj b/ide/vs2019/mimalloc.vcxproj index 5658b536..28e96d71 100644 --- a/ide/vs2019/mimalloc.vcxproj +++ b/ide/vs2019/mimalloc.vcxproj @@ -111,12 +111,12 @@ - Level3 + Level2 Disabled true true ../../include - MI_DEBUG=3;%(PreprocessorDefinitions); + MI_DEBUG=1;%(PreprocessorDefinitions); CompileAsCpp false stdcpp17 diff --git a/include/mimalloc-internal.h b/include/mimalloc-internal.h index 4c47af94..7bffb6ac 100644 --- a/include/mimalloc-internal.h +++ b/include/mimalloc-internal.h @@ -20,6 +20,18 @@ terms of the MIT license. A copy of the license can be found in the file #define mi_trace_message(...) #endif +#if defined(_MSC_VER) +#define mi_decl_noinline __declspec(noinline) +#define mi_attr_noreturn +#elif defined(__GNUC__) || defined(__clang__) +#define mi_decl_noinline __attribute__((noinline)) +#define mi_attr_noreturn __attribute__((noreturn)) +#else +#define mi_decl_noinline +#define mi_attr_noreturn +#endif + + // "options.c" void _mi_fputs(mi_output_fun* out, const char* prefix, const char* message); void _mi_fprintf(mi_output_fun* out, const char* fmt, ...); @@ -28,6 +40,7 @@ void _mi_warning_message(const char* fmt, ...); void _mi_verbose_message(const char* fmt, ...); void _mi_trace_message(const char* fmt, ...); void _mi_options_init(void); +void _mi_fatal_error(const char* fmt, ...) mi_attr_noreturn; // "init.c" extern mi_stats_t _mi_stats_main; @@ -124,13 +137,6 @@ bool _mi_page_is_valid(mi_page_t* page); #define __has_builtin(x) 0 #endif -#if defined(_MSC_VER) -#define mi_decl_noinline __declspec(noinline) -#elif defined(__GNUC__) || defined(__clang__) -#define mi_decl_noinline __attribute__((noinline)) -#else -#define mi_decl_noinline -#endif /* ----------------------------------------------------------- @@ -365,8 +371,12 @@ static inline void mi_page_set_has_aligned(mi_page_t* page, bool has_aligned) { // Encoding/Decoding the free list next pointers // ------------------------------------------------------------------- -static inline mi_block_t* mi_block_nextx( uintptr_t cookie, mi_block_t* block ) { - #if MI_SECURE +static inline bool mi_is_in_same_segment(const void* p, const void* q) { + return (_mi_ptr_segment(p) == _mi_ptr_segment(q)); +} + +static inline mi_block_t* mi_block_nextx( uintptr_t cookie, const mi_block_t* block ) { + #if MI_SECURE return (mi_block_t*)(block->next ^ cookie); #else UNUSED(cookie); @@ -374,7 +384,7 @@ static inline mi_block_t* mi_block_nextx( uintptr_t cookie, mi_block_t* block ) #endif } -static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, mi_block_t* next) { +static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, const mi_block_t* next) { #if MI_SECURE block->next = (mi_encoded_t)next ^ cookie; #else @@ -383,16 +393,25 @@ static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, mi_bl #endif } -static inline mi_block_t* mi_block_next(mi_page_t* page, mi_block_t* block) { +static inline mi_block_t* mi_block_next(const mi_page_t* page, const mi_block_t* block) { #if MI_SECURE - return mi_block_nextx(page->cookie,block); + mi_block_t* next = mi_block_nextx(page->cookie,block); + #if MI_SECURE >= 4 + // check if next is at least in our segment range + // TODO: it is better to check if it is actually inside our page but that is more expensive + // to calculate. Perhaps with a relative free list this becomes feasible? + if (next!=NULL && !mi_is_in_same_segment(block, next)) { + _mi_fatal_error("corrupted free list entry at %p: %zx\n", block, (uintptr_t)next); + } + #endif + return next; #else UNUSED(page); return mi_block_nextx(0, block); #endif } -static inline void mi_block_set_next(mi_page_t* page, mi_block_t* block, mi_block_t* next) { +static inline void mi_block_set_next(const mi_page_t* page, mi_block_t* block, const mi_block_t* next) { #if MI_SECURE mi_block_set_nextx(page->cookie,block,next); #else diff --git a/include/mimalloc-types.h b/include/mimalloc-types.h index eea76a25..00a83839 100644 --- a/include/mimalloc-types.h +++ b/include/mimalloc-types.h @@ -22,8 +22,11 @@ terms of the MIT license. A copy of the license can be found in the file // Define MI_STAT as 1 to maintain statistics; set it to 2 to have detailed statistics (but costs some performance). // #define MI_STAT 1 -// Define MI_SECURE as 1 to encode free lists -// #define MI_SECURE 1 +// Define MI_SECURE to enable security mitigations +// #define MI_SECURE 1 // guard page around metadata +// #define MI_SECURE 2 // guard page around each mimalloc page +// #define MI_SECURE 3 // encode free lists +// #define MI_SECURE 4 // all security enabled (checks for double free, corrupted free list and invalid pointer free) #if !defined(MI_SECURE) #define MI_SECURE 0 diff --git a/src/alloc.c b/src/alloc.c index 0c399671..f5208a0a 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -124,10 +124,50 @@ mi_decl_allocator void* mi_zalloc(size_t size) mi_attr_noexcept { } +// ------------------------------------------------------ +// Check for double free in secure mode +// ------------------------------------------------------ + +#if MI_SECURE>=4 +static bool mi_list_contains(const mi_page_t* page, const mi_block_t* list, const mi_block_t* elem) { + while (list != NULL) { + if (elem==list) return true; + list = mi_block_next(page, list); + } + return false; +} + +static mi_decl_noinline void mi_free_check_blockx(const mi_page_t* page, const mi_block_t* block, const mi_block_t* n) { + size_t psize; + uint8_t* pstart = _mi_page_start(_mi_page_segment(page), page, &psize); + if ((uint8_t*)n >= pstart && (uint8_t*)n < (pstart + psize)) { + // Suspicious: the decoded value is in the same page. + // Walk the free lists to see if it is already freed + if (mi_list_contains(page, page->free, n) || + mi_list_contains(page, page->local_free, n) || + mi_list_contains(page, (const mi_block_t*)mi_atomic_read_ptr_relaxed(mi_atomic_cast(void*,&page->thread_free)), n)) + { + _mi_fatal_error("double free detected of block %p with size %zu\n", block, page->block_size); + } + } +} + +static inline void mi_free_check_block(const mi_page_t* page, const mi_block_t* block) { + mi_block_t* n = (mi_block_t*)(block->next ^ page->cookie); + if (n!=NULL && mi_is_in_same_segment(block, n)) { // quick check + // Suspicous: decoded value in block is in the same segment, maybe a double free? + mi_free_check_blockx(page, block, n); + } + return; +} +#endif + + // ------------------------------------------------------ // Free // ------------------------------------------------------ + // multi-threaded free static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* block) { @@ -251,14 +291,16 @@ void mi_free(void* p) mi_attr_noexcept #if (MI_DEBUG>0) if (mi_unlikely(!mi_is_in_heap_region(p))) { - _mi_warning_message("possibly trying to mi_free a pointer that does not point to a valid heap region: 0x%p\n" + _mi_warning_message("possibly trying to free a pointer that does not point to a valid heap region: 0x%p\n" "(this may still be a valid very large allocation (over 64MiB))\n", p); if (mi_likely(_mi_ptr_cookie(segment) == segment->cookie)) { _mi_warning_message("(yes, the previous pointer 0x%p was valid after all)\n", p); } } +#endif +#if (MI_DEBUG>0 || MI_SECURE>=4) if (mi_unlikely(_mi_ptr_cookie(segment) != segment->cookie)) { - _mi_error_message("trying to mi_free a pointer that does not point to a valid heap space: %p\n", p); + _mi_error_message("trying to free a pointer that does not point to a valid heap space: %p\n", p); return; } #endif @@ -278,6 +320,9 @@ void mi_free(void* p) mi_attr_noexcept if (mi_likely(tid == segment->thread_id && page->flags.full_aligned == 0)) { // the thread id matches and it is not a full page, nor has aligned blocks // local, and not full or aligned mi_block_t* block = (mi_block_t*)p; + #if MI_SECURE>=4 + mi_free_check_block(page,block); + #endif mi_block_set_next(page, block, page->local_free); page->local_free = block; page->used--; diff --git a/src/options.c b/src/options.c index 4e2bdeaa..e74d9eb5 100644 --- a/src/options.c +++ b/src/options.c @@ -285,6 +285,14 @@ void _mi_assert_fail(const char* assertion, const char* fname, unsigned line, co } #endif +mi_attr_noreturn void _mi_fatal_error(const char* fmt, ...) { + va_list args; + va_start(args, fmt); + mi_vfprintf(NULL, "mimalloc: fatal: ", fmt, args); + va_end(args); + exit(99); +} + // -------------------------------------------------------- // Initialize options by checking the environment // -------------------------------------------------------- diff --git a/test/main-override-static.c b/test/main-override-static.c index 6ddf4f37..d8369389 100644 --- a/test/main-override-static.c +++ b/test/main-override-static.c @@ -2,12 +2,16 @@ #include #include #include +#include #include #include // redefines malloc etc. +static void double_free(); + int main() { mi_version(); + double_free(); void* p1 = malloc(78); void* p2 = malloc(24); free(p1); @@ -29,3 +33,19 @@ int main() { mi_stats_print(NULL); return 0; } + +static void double_free() { + void* p[256]; + uintptr_t buf[256]; + + p[0] = mi_malloc(622616); + p[1] = mi_malloc(655362); + p[2] = mi_malloc(786432); + mi_free(p[2]); + // [VULN] Double free + mi_free(p[2]); + p[3] = mi_malloc(786456); + // [BUG] Found overlap + // p[3]=0x429b2ea2000 (size=917504), p[1]=0x429b2e42000 (size=786432) + fprintf(stderr, "p3: %p-%p, p1: %p-%p, p2: %p\n", p[3], (uint8_t*)(p[3]) + 786456, p[1], (uint8_t*)(p[1]) + 655362, p[2]); +} diff --git a/test/main-override.cpp b/test/main-override.cpp index 4bc91ae8..ea940061 100644 --- a/test/main-override.cpp +++ b/test/main-override.cpp @@ -2,10 +2,13 @@ #include #include #include +#include #include #include +static void double_free(); + static void* p = malloc(8); void free_p() { @@ -24,6 +27,7 @@ public: int main() { //mi_stats_reset(); // ignore earlier allocations + double_free(); atexit(free_p); void* p1 = malloc(78); void* p2 = mi_malloc_aligned(16,24); @@ -66,3 +70,5 @@ public: }; static Static s = Static(); + + From 25246070aeb3dc5a8f602a6e34222284b18560b4 Mon Sep 17 00:00:00 2001 From: daan Date: Sat, 19 Oct 2019 08:34:18 -0700 Subject: [PATCH 7/9] fix double free check in secure = 4 mode; inline _mi_ptr_cookie --- include/mimalloc-internal.h | 5 ++++- src/alloc.c | 32 ++++++++++++++++++-------------- src/init.c | 4 ---- test/main-override-static.c | 26 +++++++++++++++++++++++--- 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/include/mimalloc-internal.h b/include/mimalloc-internal.h index 7bffb6ac..cf0252c6 100644 --- a/include/mimalloc-internal.h +++ b/include/mimalloc-internal.h @@ -46,7 +46,6 @@ void _mi_fatal_error(const char* fmt, ...) mi_attr_noreturn; extern mi_stats_t _mi_stats_main; extern const mi_page_t _mi_page_empty; bool _mi_is_main_thread(void); -uintptr_t _mi_ptr_cookie(const void* p); uintptr_t _mi_random_shuffle(uintptr_t x); uintptr_t _mi_random_init(uintptr_t seed /* can be zero */); bool _mi_preloading(); // true while the C runtime is not ready @@ -244,6 +243,10 @@ static inline bool mi_heap_is_initialized(mi_heap_t* heap) { return (heap != &_mi_heap_empty); } +static inline uintptr_t _mi_ptr_cookie(const void* p) { + return ((uintptr_t)p ^ _mi_heap_main.cookie); +} + /* ----------------------------------------------------------- Pages ----------------------------------------------------------- */ diff --git a/src/alloc.c b/src/alloc.c index f5208a0a..916b1f32 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -137,28 +137,32 @@ static bool mi_list_contains(const mi_page_t* page, const mi_block_t* list, cons return false; } -static mi_decl_noinline void mi_free_check_blockx(const mi_page_t* page, const mi_block_t* block, const mi_block_t* n) { +static mi_decl_noinline bool mi_check_double_freex(const mi_page_t* page, const mi_block_t* block, const mi_block_t* n) { size_t psize; uint8_t* pstart = _mi_page_start(_mi_page_segment(page), page, &psize); - if ((uint8_t*)n >= pstart && (uint8_t*)n < (pstart + psize)) { - // Suspicious: the decoded value is in the same page. + if (n == NULL || ((uint8_t*)n >= pstart && (uint8_t*)n < (pstart + psize))) { + // Suspicious: the decoded value is in the same page (or NULL). // Walk the free lists to see if it is already freed - if (mi_list_contains(page, page->free, n) || - mi_list_contains(page, page->local_free, n) || - mi_list_contains(page, (const mi_block_t*)mi_atomic_read_ptr_relaxed(mi_atomic_cast(void*,&page->thread_free)), n)) + if (mi_list_contains(page, page->free, block) || + mi_list_contains(page, page->local_free, block) || + mi_list_contains(page, (const mi_block_t*)mi_atomic_read_ptr_relaxed(mi_atomic_cast(void*,&page->thread_free)), block)) { _mi_fatal_error("double free detected of block %p with size %zu\n", block, page->block_size); + return true; } } + return false; } -static inline void mi_free_check_block(const mi_page_t* page, const mi_block_t* block) { +static inline bool mi_check_double_free(const mi_page_t* page, const mi_block_t* block) { mi_block_t* n = (mi_block_t*)(block->next ^ page->cookie); - if (n!=NULL && mi_is_in_same_segment(block, n)) { // quick check - // Suspicous: decoded value in block is in the same segment, maybe a double free? - mi_free_check_blockx(page, block, n); - } - return; + if (((uintptr_t)n & (MI_INTPTR_SIZE-1))==0 && // quick check + (n==NULL || mi_is_in_same_segment(block, n))) + { + // Suspicous: decoded value in block is in the same segment (or NULL) -- maybe a double free? + return mi_check_double_freex(page, block, n); + } + return false; } #endif @@ -320,8 +324,8 @@ void mi_free(void* p) mi_attr_noexcept if (mi_likely(tid == segment->thread_id && page->flags.full_aligned == 0)) { // the thread id matches and it is not a full page, nor has aligned blocks // local, and not full or aligned mi_block_t* block = (mi_block_t*)p; - #if MI_SECURE>=4 - mi_free_check_block(page,block); + #if MI_SECURE>=4 + if (mi_check_double_free(page,block)) return; #endif mi_block_set_next(page, block, page->local_free); page->local_free = block; diff --git a/src/init.c b/src/init.c index 75836aca..6514ce53 100644 --- a/src/init.c +++ b/src/init.c @@ -184,10 +184,6 @@ uintptr_t _mi_random_init(uintptr_t seed /* can be zero */) { return x; } -uintptr_t _mi_ptr_cookie(const void* p) { - return ((uintptr_t)p ^ _mi_heap_main.cookie); -} - /* ----------------------------------------------------------- Initialization and freeing of the thread local heaps ----------------------------------------------------------- */ diff --git a/test/main-override-static.c b/test/main-override-static.c index d8369389..ed5048e0 100644 --- a/test/main-override-static.c +++ b/test/main-override-static.c @@ -7,11 +7,13 @@ #include #include // redefines malloc etc. -static void double_free(); +static void double_free1(); +static void double_free2(); int main() { mi_version(); - double_free(); + //double_free1(); + //double_free2(); void* p1 = malloc(78); void* p2 = malloc(24); free(p1); @@ -34,7 +36,7 @@ int main() { return 0; } -static void double_free() { +static void double_free1() { void* p[256]; uintptr_t buf[256]; @@ -49,3 +51,21 @@ static void double_free() { // p[3]=0x429b2ea2000 (size=917504), p[1]=0x429b2e42000 (size=786432) fprintf(stderr, "p3: %p-%p, p1: %p-%p, p2: %p\n", p[3], (uint8_t*)(p[3]) + 786456, p[1], (uint8_t*)(p[1]) + 655362, p[2]); } + +static void double_free2() { + void* p[256]; + uintptr_t buf[256]; + // [INFO] Command buffer: 0x327b2000 + // [INFO] Input size: 182 + p[0] = malloc(712352); + p[1] = malloc(786432); + free(p[0]); + // [VULN] Double free + free(p[0]); + p[2] = malloc(786440); + p[3] = malloc(917504); + p[4] = malloc(786440); + // [BUG] Found overlap + // p[4]=0x433f1402000 (size=917504), p[1]=0x433f14c2000 (size=786432) + fprintf(stderr, "p1: %p-%p, p2: %p-%p\n", p[4], (uint8_t*)(p[4]) + 917504, p[1], (uint8_t*)(p[1]) + 786432); +} From 5dfdc092b50612abddadc97bf609222bef2ab00f Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 12:26:57 -0700 Subject: [PATCH 8/9] improve windows warning message --- src/os.c | 2 +- test/main-override.cpp | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/os.c b/src/os.c index ed938221..0cd3a1ab 100644 --- a/src/os.c +++ b/src/os.c @@ -283,7 +283,7 @@ static void* mi_win_virtual_alloc(void* addr, size_t size, size_t try_alignment, p = mi_win_virtual_allocx(addr, size, try_alignment, flags); } if (p == NULL) { - _mi_warning_message("unable to alloc mem error: err: %i size: 0x%x \n", GetLastError(), size); + _mi_warning_message("unable to allocate memory: error code: %i, addr: %p, size: 0x%x, large only: %d, allow_large: %d\n", GetLastError(), addr, size, large_only, allow_large); } return p; } diff --git a/test/main-override.cpp b/test/main-override.cpp index ea940061..92740c6e 100644 --- a/test/main-override.cpp +++ b/test/main-override.cpp @@ -7,8 +7,6 @@ #include #include -static void double_free(); - static void* p = malloc(8); void free_p() { From ff9f29660b38ad3dfb9b8a55d7b5fb1837a50c7f Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 12:27:32 -0700 Subject: [PATCH 9/9] remove double_free call --- test/main-override.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/main-override.cpp b/test/main-override.cpp index 92740c6e..e006ad27 100644 --- a/test/main-override.cpp +++ b/test/main-override.cpp @@ -25,7 +25,6 @@ public: int main() { //mi_stats_reset(); // ignore earlier allocations - double_free(); atexit(free_p); void* p1 = malloc(78); void* p2 = mi_malloc_aligned(16,24);