From 28d4ec4c5ad39bc9459cd432c34f8d9234b51431 Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 13:14:14 -0700 Subject: [PATCH 1/6] fix statistics accounting of huge pages --- src/alloc.c | 10 +++++++++- src/page.c | 3 ++- test/test-stress.c | 7 ++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/alloc.c b/src/alloc.c index 916b1f32..f8dab24d 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -192,7 +192,15 @@ static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* bloc page->free = block; page->used--; page->is_zero = false; - _mi_segment_page_free(page,true,&heap->tld->segments); + mi_assert(page->used == 0); + mi_tld_t* tld = heap->tld; + if (page->block_size > MI_HUGE_OBJ_SIZE_MAX) { + _mi_stat_decrease(&tld->stats.giant, page->block_size); + } + else { + _mi_stat_decrease(&tld->stats.huge, page->block_size); + } + _mi_segment_page_free(page,true,&tld->segments); } return; } diff --git a/src/page.c b/src/page.c index 77d98f11..b71be522 100644 --- a/src/page.c +++ b/src/page.c @@ -370,6 +370,7 @@ void _mi_page_free(mi_page_t* page, mi_page_queue_t* pq, bool force) { mi_page_set_has_aligned(page, false); // account for huge pages here + // (note: no longer necessary as huge pages are always abandoned) if (page->block_size > MI_LARGE_OBJ_SIZE_MAX) { if (page->block_size > MI_HUGE_OBJ_SIZE_MAX) { _mi_stat_decrease(&page->heap->tld->stats.giant, page->block_size); @@ -378,7 +379,7 @@ void _mi_page_free(mi_page_t* page, mi_page_queue_t* pq, bool force) { _mi_stat_decrease(&page->heap->tld->stats.huge, page->block_size); } } - + // remove from the page list // (no need to do _mi_heap_delayed_free first as all blocks are already free) mi_segments_tld_t* segments_tld = &page->heap->tld->segments; diff --git a/test/test-stress.c b/test/test-stress.c index 354e2b07..bb428072 100644 --- a/test/test-stress.c +++ b/test/test-stress.c @@ -158,6 +158,7 @@ int main(int argc, char** argv) { //printf("(reserve huge: %i\n)", res); //bench_start_program(); + mi_stats_reset(); memset((void*)transfer, 0, TRANSFERS*sizeof(void*)); run_os_threads(THREADS); for (int i = 0; i < TRANSFERS; i++) { @@ -165,7 +166,6 @@ int main(int argc, char** argv) { } #ifndef NDEBUG mi_collect(false); - mi_collect(true); #endif mi_stats_print(NULL); //bench_end_program(); @@ -191,6 +191,11 @@ static void run_os_threads(size_t nthreads) { for (size_t i = 0; i < nthreads; i++) { WaitForSingleObject(thandles[i], INFINITE); } + for (size_t i = 0; i < nthreads; i++) { + CloseHandle(thandles[i]); + } + free(tids); + free(thandles); } static void* atomic_exchange_ptr(volatile void** p, void* newval) { From 081e2d1eb6d517f1949b3245d37f758e8a27ac3f Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 13:43:42 -0700 Subject: [PATCH 2/6] fix statistics display --- include/mimalloc-types.h | 6 +++--- src/init.c | 4 ++-- src/os.c | 4 ++-- src/page.c | 2 +- src/stats.c | 36 ++++++++++++++++++++++++------------ 5 files changed, 32 insertions(+), 20 deletions(-) diff --git a/include/mimalloc-types.h b/include/mimalloc-types.h index 00a83839..2e5d481b 100644 --- a/include/mimalloc-types.h +++ b/include/mimalloc-types.h @@ -340,14 +340,14 @@ typedef struct mi_stats_s { mi_stat_count_t page_committed; mi_stat_count_t segments_abandoned; mi_stat_count_t pages_abandoned; - mi_stat_count_t pages_extended; - mi_stat_count_t mmap_calls; - mi_stat_count_t commit_calls; mi_stat_count_t threads; mi_stat_count_t huge; mi_stat_count_t giant; mi_stat_count_t malloc; mi_stat_count_t segments_cache; + mi_stat_counter_t pages_extended; + mi_stat_counter_t mmap_calls; + mi_stat_counter_t commit_calls; mi_stat_counter_t page_no_retire; mi_stat_counter_t searches; mi_stat_counter_t huge_count; diff --git a/src/init.c b/src/init.c index 6514ce53..d361de3a 100644 --- a/src/init.c +++ b/src/init.c @@ -64,8 +64,8 @@ const mi_page_t _mi_page_empty = { MI_STAT_COUNT_NULL(), MI_STAT_COUNT_NULL(), \ MI_STAT_COUNT_NULL(), MI_STAT_COUNT_NULL(), \ MI_STAT_COUNT_NULL(), MI_STAT_COUNT_NULL(), \ - MI_STAT_COUNT_NULL(), MI_STAT_COUNT_NULL(), \ - MI_STAT_COUNT_NULL(), MI_STAT_COUNT_NULL(), \ + MI_STAT_COUNT_NULL(), \ + { 0, 0 }, { 0, 0 }, { 0, 0 }, \ { 0, 0 }, { 0, 0 }, { 0, 0 }, { 0, 0 } \ MI_STAT_COUNT_END_NULL() diff --git a/src/os.c b/src/os.c index 0cd3a1ab..8f5afc5b 100644 --- a/src/os.c +++ b/src/os.c @@ -477,7 +477,7 @@ static void* mi_os_mem_alloc(size_t size, size_t try_alignment, bool commit, boo int protect_flags = (commit ? (PROT_WRITE | PROT_READ) : PROT_NONE); p = mi_unix_mmap(NULL, size, try_alignment, protect_flags, false, allow_large, is_large); #endif - _mi_stat_increase(&stats->mmap_calls, 1); + mi_stat_counter_increase(stats->mmap_calls, 1); if (p != NULL) { _mi_stat_increase(&stats->reserved, size); if (commit) { _mi_stat_increase(&stats->committed, size); } @@ -632,7 +632,7 @@ static bool mi_os_commitx(void* addr, size_t size, bool commit, bool conservativ int err = 0; if (commit) { _mi_stat_increase(&stats->committed, csize); - _mi_stat_increase(&stats->commit_calls, 1); + _mi_stat_counter_increase(&stats->commit_calls, 1); } else { _mi_stat_decrease(&stats->committed, csize); diff --git a/src/page.c b/src/page.c index b71be522..2a48c64b 100644 --- a/src/page.c +++ b/src/page.c @@ -531,7 +531,7 @@ static void mi_page_extend_free(mi_heap_t* heap, mi_page_t* page, mi_stats_t* st size_t page_size; _mi_page_start(_mi_page_segment(page), page, &page_size); - mi_stat_increase(stats->pages_extended, 1); + mi_stat_counter_increase(stats->pages_extended, 1); // calculate the extend count size_t extend = page->reserved - page->capacity; diff --git a/src/stats.c b/src/stats.c index 37a7bde4..50bd029d 100644 --- a/src/stats.c +++ b/src/stats.c @@ -95,15 +95,17 @@ static void mi_stats_add(mi_stats_t* stats, const mi_stats_t* src) { mi_stat_add(&stats->pages_abandoned, &src->pages_abandoned, 1); mi_stat_add(&stats->segments_abandoned, &src->segments_abandoned, 1); - mi_stat_add(&stats->mmap_calls, &src->mmap_calls, 1); - mi_stat_add(&stats->commit_calls, &src->commit_calls, 1); mi_stat_add(&stats->threads, &src->threads, 1); - mi_stat_add(&stats->pages_extended, &src->pages_extended, 1); mi_stat_add(&stats->malloc, &src->malloc, 1); mi_stat_add(&stats->segments_cache, &src->segments_cache, 1); mi_stat_add(&stats->huge, &src->huge, 1); mi_stat_add(&stats->giant, &src->giant, 1); + + mi_stat_counter_add(&stats->pages_extended, &src->pages_extended, 1); + mi_stat_counter_add(&stats->mmap_calls, &src->mmap_calls, 1); + mi_stat_counter_add(&stats->commit_calls, &src->commit_calls, 1); + mi_stat_counter_add(&stats->page_no_retire, &src->page_no_retire, 1); mi_stat_counter_add(&stats->searches, &src->searches, 1); mi_stat_counter_add(&stats->huge_count, &src->huge_count, 1); @@ -121,6 +123,9 @@ static void mi_stats_add(mi_stats_t* stats, const mi_stats_t* src) { Display statistics ----------------------------------------------------------- */ +// unit > 0 : size in binary bytes +// unit == 0: count as decimal +// unit < 0 : count in binary static void mi_printf_amount(int64_t n, int64_t unit, mi_output_fun* out, const char* fmt) { char buf[32]; int len = 32; @@ -165,17 +170,24 @@ static void mi_stat_print(const mi_stat_count_t* stat, const char* msg, int64_t _mi_fprintf(out, " ok\n"); } else if (unit<0) { - mi_print_amount(stat->peak, 1, out); - mi_print_amount(stat->allocated, 1, out); - mi_print_amount(stat->freed, 1, out); - mi_print_amount(-unit, 1, out); - mi_print_count((stat->allocated / -unit), 0, out); + mi_print_amount(stat->peak, -1, out); + mi_print_amount(stat->allocated, -1, out); + mi_print_amount(stat->freed, -1, out); + if (unit==-1) { + _mi_fprintf(out, "%22s", ""); + } + else { + mi_print_amount(-unit, 1, out); + mi_print_count((stat->allocated / -unit), 0, out); + } if (stat->allocated > stat->freed) _mi_fprintf(out, " not all freed!\n"); else _mi_fprintf(out, " ok\n"); } else { + mi_print_amount(stat->peak, 1, out); + mi_print_amount(stat->allocated, 1, out); _mi_fprintf(out, "\n"); } } @@ -247,11 +259,11 @@ static void _mi_stats_print(mi_stats_t* stats, double secs, mi_output_fun* out) mi_stat_print(&stats->segments_cache, "-cached", -1, out); mi_stat_print(&stats->pages, "pages", -1, out); mi_stat_print(&stats->pages_abandoned, "-abandoned", -1, out); - mi_stat_print(&stats->pages_extended, "-extended", 0, out); + mi_stat_counter_print(&stats->pages_extended, "-extended", out); mi_stat_counter_print(&stats->page_no_retire, "-noretire", out); - mi_stat_print(&stats->mmap_calls, "mmaps", 0, out); - mi_stat_print(&stats->commit_calls, "commits", 0, out); - mi_stat_print(&stats->threads, "threads", 0, out); + mi_stat_counter_print(&stats->mmap_calls, "mmaps", out); + mi_stat_counter_print(&stats->commit_calls, "commits", out); + mi_stat_print(&stats->threads, "threads", -1, out); mi_stat_counter_print_avg(&stats->searches, "searches", out); if (secs >= 0.0) _mi_fprintf(out, "%10s: %9.3f s\n", "elapsed", secs); From 87bdfbb9b6b0a869f9ce8e76fd4fa580bb816840 Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 14:12:06 -0700 Subject: [PATCH 3/6] use more conservative retire strategy --- src/page.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/page.c b/src/page.c index 2a48c64b..5a186727 100644 --- a/src/page.c +++ b/src/page.c @@ -407,16 +407,18 @@ void _mi_page_retire(mi_page_t* page) { // (or we end up retiring and re-allocating most of the time) // NOTE: refine this more: we should not retire if this // is the only page left with free blocks. It is not clear - // how to check this efficiently though... for now we just check - // if its neighbours are almost fully used. + // how to check this efficiently though... + // for now, we don't retire if it is the only page left of this size class. + mi_page_queue_t* pq = mi_page_queue_of(page); if (mi_likely(page->block_size <= (MI_SMALL_SIZE_MAX/4))) { - if (mi_page_mostly_used(page->prev) && mi_page_mostly_used(page->next)) { + // if (mi_page_mostly_used(page->prev) && mi_page_mostly_used(page->next)) { + if (pq->last==page && pq->first==page) { mi_stat_counter_increase(_mi_stats_main.page_no_retire,1); return; // dont't retire after all } } - _mi_page_free(page, mi_page_queue_of(page), false); + _mi_page_free(page, pq, false); } From b052d3b73129fee02e145b7c1b8b2153dd39af0d Mon Sep 17 00:00:00 2001 From: daan Date: Mon, 28 Oct 2019 15:54:33 -0700 Subject: [PATCH 4/6] enable double free and heap corruption detection in debug mode --- include/mimalloc-internal.h | 23 +++++++++--------- include/mimalloc-types.h | 27 ++++++++++++++------- src/alloc.c | 47 +++++++++++++++++++++---------------- src/init.c | 4 ++-- src/options.c | 4 +++- src/page.c | 6 ++--- test/main-override-static.c | 42 +++++++++++++++++++++++++++++++-- 7 files changed, 104 insertions(+), 49 deletions(-) diff --git a/include/mimalloc-internal.h b/include/mimalloc-internal.h index cf0252c6..ccf12a06 100644 --- a/include/mimalloc-internal.h +++ b/include/mimalloc-internal.h @@ -379,7 +379,7 @@ static inline bool mi_is_in_same_segment(const void* p, const void* q) { } static inline mi_block_t* mi_block_nextx( uintptr_t cookie, const mi_block_t* block ) { - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST return (mi_block_t*)(block->next ^ cookie); #else UNUSED(cookie); @@ -388,7 +388,7 @@ static inline mi_block_t* mi_block_nextx( uintptr_t cookie, const mi_block_t* bl } static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, const mi_block_t* next) { - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST block->next = (mi_encoded_t)next ^ cookie; #else UNUSED(cookie); @@ -397,16 +397,15 @@ static inline void mi_block_set_nextx(uintptr_t cookie, mi_block_t* block, const } static inline mi_block_t* mi_block_next(const mi_page_t* page, const mi_block_t* block) { - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST 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 + // check for free list corruption: is `next` 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 of size %zub at %p: value 0x%zx\n", page->block_size, block, (uintptr_t)next); + next = NULL; + } return next; #else UNUSED(page); @@ -415,7 +414,7 @@ static inline mi_block_t* mi_block_next(const mi_page_t* page, const mi_block_t* } static inline void mi_block_set_next(const mi_page_t* page, mi_block_t* block, const mi_block_t* next) { - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST mi_block_set_nextx(page->cookie,block,next); #else UNUSED(page); diff --git a/include/mimalloc-types.h b/include/mimalloc-types.h index 2e5d481b..99b6b22b 100644 --- a/include/mimalloc-types.h +++ b/include/mimalloc-types.h @@ -25,24 +25,30 @@ terms of the MIT license. A copy of the license can be found in the file // 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) +// #define MI_SECURE 3 // encode free lists (detect corrupted free list (buffer overflow), and invalid pointer free) +// #define MI_SECURE 4 // experimental, may be more expensive: checks for double free. #if !defined(MI_SECURE) #define MI_SECURE 0 #endif -// Define MI_DEBUG as 1 for basic assert checks and statistics -// set it to 2 to do internal asserts, -// and to 3 to do extensive invariant checking. +// Define MI_DEBUG for debug mode +// #define MI_DEBUG 1 // basic assertion checks and statistics, check double free, corrupted free list, and invalid pointer free. +// #define MI_DEBUG 2 // + internal assertion checks +// #define MI_DEBUG 3 // + extensive internal invariant checking #if !defined(MI_DEBUG) #if !defined(NDEBUG) || defined(_DEBUG) -#define MI_DEBUG 1 +#define MI_DEBUG 2 #else #define MI_DEBUG 0 #endif #endif +// Encoded free lists allow detection of corrupted free lists +// and can detect buffer overflows and double `free`s. +#if (MI_SECURE>=3 || MI_DEBUG>=1) +#define MI_ENCODE_FREELIST 1 +#endif // ------------------------------------------------------ // Platform specific values @@ -117,6 +123,8 @@ terms of the MIT license. A copy of the license can be found in the file #error "define more bins" #endif +// The free lists use encoded next fields +// (Only actually encodes when MI_ENCODED_FREELIST is defined.) typedef uintptr_t mi_encoded_t; // free lists contain blocks @@ -125,6 +133,7 @@ typedef struct mi_block_s { } mi_block_t; +// The delayed flags are used for efficient multi-threaded free-ing typedef enum mi_delayed_e { MI_NO_DELAYED_FREE = 0, MI_USE_DELAYED_FREE = 1, @@ -180,7 +189,7 @@ typedef struct mi_page_s { bool is_zero; // `true` if the blocks in the free list are zero initialized mi_block_t* free; // list of available free blocks (`malloc` allocates from this list) - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST uintptr_t cookie; // random cookie to encode the free lists #endif size_t used; // number of blocks in use (including blocks in `local_free` and `thread_free`) @@ -197,8 +206,8 @@ typedef struct mi_page_s { // improve page index calculation // without padding: 10 words on 64-bit, 11 on 32-bit. Secure adds one word - #if (MI_INTPTR_SIZE==8 && MI_SECURE>0) || (MI_INTPTR_SIZE==4 && MI_SECURE==0) - void* padding[1]; // 12 words on 64-bit in secure mode, 12 words on 32-bit plain + #if (MI_INTPTR_SIZE==8 && defined(MI_ENCODE_FREELIST)) || (MI_INTPTR_SIZE==4 && !defined(MI_ENCODE_FREELIST)) + void* padding[1]; // 12 words on 64-bit with cookie, 12 words on 32-bit plain #endif } mi_page_t; diff --git a/src/alloc.c b/src/alloc.c index f8dab24d..d2319f82 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -32,10 +32,10 @@ extern inline void* _mi_page_malloc(mi_heap_t* heap, mi_page_t* page, size_t siz page->free = mi_block_next(page,block); page->used++; mi_assert_internal(page->free == NULL || _mi_ptr_page(page->free) == page); -#if (MI_DEBUG) +#if (MI_DEBUG!=0) if (!page->is_zero) { memset(block, MI_DEBUG_UNINIT, size); } -#elif (MI_SECURE) - block->next = 0; +#elif (MI_SECURE!=0) + block->next = 0; // don't leak internal data #endif #if (MI_STAT>1) if(size <= MI_LARGE_OBJ_SIZE_MAX) { @@ -125,10 +125,12 @@ mi_decl_allocator void* mi_zalloc(size_t size) mi_attr_noexcept { // ------------------------------------------------------ -// Check for double free in secure mode +// Check for double free in secure and debug mode +// This is somewhat expensive so only enabled for secure mode 4 // ------------------------------------------------------ -#if MI_SECURE>=4 +#if (MI_ENCODE_FREELIST && (MI_SECURE>=4 || MI_DEBUG!=0)) +// linear check if the free list contains a specific element 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; @@ -137,15 +139,15 @@ static bool mi_list_contains(const mi_page_t* page, const mi_block_t* list, cons return false; } -static mi_decl_noinline bool mi_check_double_freex(const mi_page_t* page, const mi_block_t* block, const mi_block_t* n) { +static mi_decl_noinline bool mi_check_is_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 (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 + // Walk the free lists to verify positively if it is already freed 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_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; @@ -154,16 +156,23 @@ static mi_decl_noinline bool mi_check_double_freex(const mi_page_t* page, const return false; } -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 (((uintptr_t)n & (MI_INTPTR_SIZE-1))==0 && // quick check - (n==NULL || mi_is_in_same_segment(block, n))) +static inline bool mi_check_is_double_free(const mi_page_t* page, const mi_block_t* block) { + mi_block_t* n = mi_block_nextx(page->cookie, block); // pretend it is freed, and get the decoded first field + if (((uintptr_t)n & (MI_INTPTR_SIZE-1))==0 && // quick check: aligned pointer? + (n==NULL || mi_is_in_same_segment(block, n))) // quick check: in same segment or NULL? { // Suspicous: decoded value in block is in the same segment (or NULL) -- maybe a double free? - return mi_check_double_freex(page, block, n); + // (continue in separate function to improve code generation) + return mi_check_is_double_freex(page, block, n); } return false; } +#else +static inline bool mi_check_is_double_free(const mi_page_t* page, const mi_block_t* block) { + UNUSED(page); + UNUSED(block); + return false; +} #endif @@ -171,7 +180,6 @@ static inline bool mi_check_double_free(const mi_page_t* page, const mi_block_t* // Free // ------------------------------------------------------ - // multi-threaded free static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* block) { @@ -258,6 +266,7 @@ static inline void _mi_free_block(mi_page_t* page, bool local, mi_block_t* block // and push it on the free list if (mi_likely(local)) { // owning thread can free a block directly + if (mi_check_is_double_free(page, block)) return; mi_block_set_next(page, block, page->local_free); page->local_free = block; page->used--; @@ -301,7 +310,7 @@ void mi_free(void* p) mi_attr_noexcept const mi_segment_t* const segment = _mi_ptr_segment(p); if (mi_unlikely(segment == NULL)) return; // checks for (p==NULL) -#if (MI_DEBUG>0) +#if (MI_DEBUG!=0) if (mi_unlikely(!mi_is_in_heap_region(p))) { _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); @@ -310,7 +319,7 @@ void mi_free(void* p) mi_attr_noexcept } } #endif -#if (MI_DEBUG>0 || MI_SECURE>=4) +#if (MI_DEBUG!=0 || MI_SECURE>=4) if (mi_unlikely(_mi_ptr_cookie(segment) != segment->cookie)) { _mi_error_message("trying to free a pointer that does not point to a valid heap space: %p\n", p); return; @@ -332,9 +341,7 @@ 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 - if (mi_check_double_free(page,block)) return; - #endif + if (mi_check_is_double_free(page,block)) return; mi_block_set_next(page, block, page->local_free); page->local_free = block; page->used--; diff --git a/src/init.c b/src/init.c index d361de3a..e15d82eb 100644 --- a/src/init.c +++ b/src/init.c @@ -15,14 +15,14 @@ const mi_page_t _mi_page_empty = { 0, false, false, false, false, 0, 0, { 0 }, false, NULL, // free - #if MI_SECURE + #if MI_ENCODE_FREELIST 0, #endif 0, // used NULL, ATOMIC_VAR_INIT(0), ATOMIC_VAR_INIT(0), 0, NULL, NULL, NULL - #if (MI_INTPTR_SIZE==8 && MI_SECURE>0) || (MI_INTPTR_SIZE==4 && MI_SECURE==0) + #if (MI_INTPTR_SIZE==8 && defined(MI_ENCODE_FREELIST)) || (MI_INTPTR_SIZE==4 && !defined(MI_ENCODE_FREELIST)) , { NULL } // padding #endif }; diff --git a/src/options.c b/src/options.c index e74d9eb5..d71e5d1c 100644 --- a/src/options.c +++ b/src/options.c @@ -290,7 +290,9 @@ mi_attr_noreturn void _mi_fatal_error(const char* fmt, ...) { va_start(args, fmt); mi_vfprintf(NULL, "mimalloc: fatal: ", fmt, args); va_end(args); - exit(99); + #if (MI_SECURE>=0) + abort(); + #endif } // -------------------------------------------------------- diff --git a/src/page.c b/src/page.c index 5a186727..c1d29d46 100644 --- a/src/page.c +++ b/src/page.c @@ -512,7 +512,7 @@ static mi_decl_noinline void mi_page_free_list_extend( mi_page_t* page, size_t e ----------------------------------------------------------- */ #define MI_MAX_EXTEND_SIZE (4*1024) // heuristic, one OS page seems to work well. -#if MI_SECURE +#if (MI_SECURE>0) #define MI_MIN_EXTEND (8*MI_SECURE) // extend at least by this many #else #define MI_MIN_EXTEND (1) @@ -579,7 +579,7 @@ static void mi_page_init(mi_heap_t* heap, mi_page_t* page, size_t block_size, mi page->block_size = block_size; mi_assert_internal(page_size / block_size < (1L<<16)); page->reserved = (uint16_t)(page_size / block_size); - #if MI_SECURE + #ifdef MI_ENCODE_FREELIST page->cookie = _mi_heap_random(heap) | 1; #endif page->is_zero = page->is_zero_init; @@ -592,7 +592,7 @@ static void mi_page_init(mi_heap_t* heap, mi_page_t* page, size_t block_size, mi mi_assert_internal(page->next == NULL); mi_assert_internal(page->prev == NULL); mi_assert_internal(!mi_page_has_aligned(page)); - #if MI_SECURE + #if (MI_ENCODE_FREELIST) mi_assert_internal(page->cookie != 0); #endif mi_assert_expensive(mi_page_is_valid_init(page)); diff --git a/test/main-override-static.c b/test/main-override-static.c index ed5048e0..19712411 100644 --- a/test/main-override-static.c +++ b/test/main-override-static.c @@ -9,11 +9,16 @@ static void double_free1(); static void double_free2(); +static void corrupt_free(); int main() { mi_version(); + + // detect double frees and heap corruption //double_free1(); //double_free2(); + //corrupt_free(); + void* p1 = malloc(78); void* p2 = malloc(24); free(p1); @@ -36,9 +41,13 @@ int main() { return 0; } + +// The double free samples come ArcHeap [1] by Insu Yun (issue #161) +// [1]: https://arxiv.org/pdf/1903.00503.pdf + static void double_free1() { void* p[256]; - uintptr_t buf[256]; + //uintptr_t buf[256]; p[0] = mi_malloc(622616); p[1] = mi_malloc(655362); @@ -54,7 +63,7 @@ static void double_free1() { static void double_free2() { void* p[256]; - uintptr_t buf[256]; + //uintptr_t buf[256]; // [INFO] Command buffer: 0x327b2000 // [INFO] Input size: 182 p[0] = malloc(712352); @@ -69,3 +78,32 @@ static void double_free2() { // 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); } + + +// Try to corrupt the heap through buffer overflow +#define N 256 +#define SZ 64 + +static void corrupt_free() { + void* p[N]; + // allocate + for (int i = 0; i < N; i++) { + p[i] = malloc(SZ); + } + // free some + for (int i = 0; i < N; i += (N/10)) { + free(p[i]); + p[i] = NULL; + } + // try to corrupt the free list + for (int i = 0; i < N; i++) { + if (p[i] != NULL) { + memset(p[i], 0, SZ+8); + } + } + // allocate more.. trying to trigger an allocation from a corrupted entry + // this may need many allocations to get there (if at all) + for (int i = 0; i < 4096; i++) { + malloc(SZ); + } +} \ No newline at end of file From 6cf16b1201fdfa47e18020b2166d49c5b97d2097 Mon Sep 17 00:00:00 2001 From: daan Date: Wed, 30 Oct 2019 14:32:28 -0700 Subject: [PATCH 5/6] fix reset error on windows when disabling eager commit option --- src/memory.c | 10 +++++++--- src/options.c | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/memory.c b/src/memory.c index f9c53782..dd03cf95 100644 --- a/src/memory.c +++ b/src/memory.c @@ -461,10 +461,14 @@ void _mi_mem_free(void* p, size_t size, size_t id, mi_stats_t* stats) { // reset: 10x slowdown on malloc-large, decommit: 17x slowdown on malloc-large if (!is_large) { if (mi_option_is_enabled(mi_option_segment_reset)) { - _mi_os_reset(p, size, stats); // - // _mi_os_decommit(p,size,stats); // if !is_eager_committed (and clear dirty bits) + if (!is_eager_committed && // cannot reset large pages + (mi_option_is_enabled(mi_option_eager_commit) || // cannot reset halfway committed segments, use `option_page_reset` instead + mi_option_is_enabled(mi_option_reset_decommits))) // but we can decommit halfway committed segments + { + _mi_os_reset(p, size, stats); + //_mi_os_decommit(p, size, stats); // todo: and clear dirty bits? + } } - // else { _mi_os_reset(p,size,stats); } } if (!is_eager_committed) { // adjust commit statistics as we commit again when re-using the same slot diff --git a/src/options.c b/src/options.c index d71e5d1c..a49c46ed 100644 --- a/src/options.c +++ b/src/options.c @@ -65,7 +65,7 @@ static mi_option_desc_t options[_mi_option_last] = { 0, UNINIT, MI_OPTION(cache_reset) }, { 0, UNINIT, MI_OPTION(reset_decommits) }, // note: cannot enable this if secure is on { 0, UNINIT, MI_OPTION(eager_commit_delay) }, // the first N segments per thread are not eagerly committed - { 0, UNINIT, MI_OPTION(segment_reset) }, // reset segment memory on free + { 0, UNINIT, MI_OPTION(segment_reset) }, // reset segment memory on free (needs eager commit) { 100, UNINIT, MI_OPTION(os_tag) } // only apple specific for now but might serve more or less related purpose }; From 4a4d74927ccd7c07e68d28ca372c6c30408ad92f Mon Sep 17 00:00:00 2001 From: daan Date: Wed, 30 Oct 2019 14:53:21 -0700 Subject: [PATCH 6/6] protect against double-free in multi-threaded free list --- src/page.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/page.c b/src/page.c index c1d29d46..ab271309 100644 --- a/src/page.c +++ b/src/page.c @@ -161,14 +161,21 @@ static void _mi_page_thread_free_collect(mi_page_t* page) // return if the list is empty if (head == NULL) return; - // find the tail + // find the tail -- also to get a proper count (without data races) + uintptr_t max_count = page->capacity; // cannot collect more than capacity uintptr_t count = 1; mi_block_t* tail = head; mi_block_t* next; - while ((next = mi_block_next(page,tail)) != NULL) { + while ((next = mi_block_next(page,tail)) != NULL && count <= max_count) { count++; tail = next; } + // if `count > max_count` there was a memory corruption (possibly infinite list due to double multi-threaded free) + if (count > max_count) { + _mi_fatal_error("corrupted thread-free list\n"); + return; // the thread-free items cannot be freed + } + // and append the current local free list mi_block_set_next(page,tail, page->local_free); page->local_free = head;