From 635cf7af6acae8d03767e5c158b79b94690813fa Mon Sep 17 00:00:00 2001 From: daanx Date: Tue, 20 Aug 2024 09:55:57 -0700 Subject: [PATCH] fix multi-threaded free to unprotect guarded blocks --- include/mimalloc/types.h | 2 +- src/alloc.c | 26 +++++++++++++++++++------- src/free.c | 33 ++++++++++++++++++++++----------- src/segment.c | 1 + test/test-api-fill.c | 2 +- 5 files changed, 44 insertions(+), 20 deletions(-) diff --git a/include/mimalloc/types.h b/include/mimalloc/types.h index ffe095a5..bf75f8c7 100644 --- a/include/mimalloc/types.h +++ b/include/mimalloc/types.h @@ -74,7 +74,7 @@ terms of the MIT license. A copy of the license can be found in the file // Use guard pages behind objects of a certain size #define MI_DEBUG_GUARDED 1 -#if defined(MI_DEBUG_GUARDED) || defined(MI_DEBUG_GUARDEDX) +#if defined(MI_DEBUG_GUARDED) #define MI_PADDING 0 #endif diff --git a/src/alloc.c b/src/alloc.c index ec008318..8f47e541 100644 --- a/src/alloc.c +++ b/src/alloc.c @@ -55,8 +55,10 @@ extern inline void* _mi_page_malloc_zero(mi_heap_t* heap, mi_page_t* page, size_ // zero the block? note: we need to zero the full block size (issue #63) if mi_unlikely(zero) { mi_assert_internal(page->block_size != 0); // do not call with zero'ing for huge blocks (see _mi_malloc_generic) - mi_assert_internal(page->block_size >= MI_PADDING_SIZE); mi_assert_internal(!mi_page_is_huge(page)); + #if MI_PADDING_SIZE > 0 + mi_assert_internal(page->block_size >= MI_PADDING_SIZE); + #endif if (page->free_is_zero) { block->next = 0; mi_track_mem_defined(block, page->block_size - MI_PADDING_SIZE); @@ -131,7 +133,7 @@ static inline mi_decl_restrict void* mi_heap_malloc_small_zero(mi_heap_t* heap, if (size == 0) { size = sizeof(void*); } #endif #if MI_DEBUG_GUARDED - if (size >= _mi_option_get_fast(mi_option_debug_guarded_min) && size <= _mi_option_get_fast(mi_option_debug_guarded_max)) { + if (size <= (size_t)_mi_option_get_fast(mi_option_debug_guarded_max) && size >= (size_t)_mi_option_get_fast(mi_option_debug_guarded_min)) { return mi_heap_malloc_guarded(heap, size, zero, 0); } #endif @@ -171,8 +173,9 @@ extern inline void* _mi_heap_malloc_zero_ex(mi_heap_t* heap, size_t size, bool z } #if MI_DEBUG_GUARDED else if ( huge_alignment == 0 && // guarded pages do not work with huge aligments at the moment - ((size >= _mi_option_get_fast(mi_option_debug_guarded_min) && size <= _mi_option_get_fast(mi_option_debug_guarded_max)) - || ((size & (_mi_os_page_size()-1)) == 0)) ) // page-size multiple are always guarded so we can have a correct `mi_usable_size`. + _mi_option_get_fast(mi_option_debug_guarded_max) > 0 && // guarded must be enabled + ((size >= (size_t)_mi_option_get_fast(mi_option_debug_guarded_min) && size <= (size_t)_mi_option_get_fast(mi_option_debug_guarded_max)) + || ((mi_good_size(size) & (_mi_os_page_size()-1)) == 0)) ) // page-size multiple are always guarded so we can have a correct `mi_usable_size`. { return mi_heap_malloc_guarded(heap, size, zero, 0); } @@ -611,15 +614,24 @@ static mi_decl_restrict void* mi_heap_malloc_guarded(mi_heap_t* heap, size_t siz if (base==NULL) return NULL; mi_page_t* page = _mi_ptr_page(base); mi_segment_t* segment = _mi_page_segment(page); - + const size_t fullsize = mi_page_block_size(page); // must use `block_size` to match `mi_free_local` void* const gpage = (uint8_t*)base + (fullsize - psize); mi_assert_internal(_mi_is_aligned(gpage, psize)); - void* const p = (uint8_t*)base + (fullsize - psize - bsize); + + // place block in front of the guard page + size_t offset = fullsize - psize - bsize; + if (offset > MI_BLOCK_ALIGNMENT_MAX) { + // give up to place it right in front of the guard page if the offset is too large for unalignment + offset = MI_BLOCK_ALIGNMENT_MAX; + } + void* const p = (uint8_t*)base + offset; mi_assert_internal(p >= base); // set page flags - if (p > base) { mi_page_set_has_aligned(page, true); } + if (offset > 0) { + mi_page_set_has_aligned(page, true); + } // set guard page if (segment->allow_decommit) { diff --git a/src/free.c b/src/free.c index 07c06899..2137f7c9 100644 --- a/src/free.c +++ b/src/free.c @@ -26,6 +26,25 @@ static void mi_stat_free(const mi_page_t* page, const mi_block_t* block); // forward declaration of multi-threaded free (`_mt`) (or free in huge block if compiled with MI_HUGE_PAGE_ABANDON) static mi_decl_noinline void mi_free_block_mt(mi_page_t* page, mi_segment_t* segment, mi_block_t* block); +#if !MI_DEBUG_GUARDED +static void mi_block_unguard(mi_page_t* page, mi_block_t* block) { + MI_UNUSED(page); + MI_UNUSED(block); +} +#else +static void mi_block_unguard(mi_page_t* page, mi_block_t* block) { + if (mi_page_has_guarded(page)) { + const size_t bsize = mi_page_block_size(page); + const size_t psize = _mi_os_page_size(); + mi_assert_internal(bsize > psize); + mi_assert_internal(_mi_page_segment(page)->allow_decommit); + void* gpage = (uint8_t*)block + (bsize - psize); + mi_assert_internal(_mi_is_aligned(gpage, psize)); + _mi_os_unprotect(gpage, psize); + } +} +#endif + // regular free of a (thread local) block pointer // fast path written carefully to prevent spilling on the stack static inline void mi_free_block_local(mi_page_t* page, mi_block_t* block, bool track_stats, bool check_full) @@ -33,17 +52,7 @@ static inline void mi_free_block_local(mi_page_t* page, mi_block_t* block, bool // checks if mi_unlikely(mi_check_is_double_free(page, block)) return; mi_check_padding(page, block); - if (track_stats) { mi_stat_free(page, block); } - #if MI_DEBUG_GUARDED - if (mi_page_has_guarded(page)) { - const size_t bsize = mi_page_block_size(page); - const size_t psize = _mi_os_page_size(); - mi_assert_internal(bsize > psize); - mi_assert_internal(_mi_page_segment(page)->allow_decommit); - void* gpage = (uint8_t*)block + (bsize - psize); - _mi_os_unprotect(gpage, psize); - } - #endif + if (track_stats) { mi_stat_free(page, block); } #if (MI_DEBUG>0) && !MI_TRACK_ENABLED && !MI_TSAN && !MI_DEBUG_GUARDED memset(block, MI_DEBUG_FREED, mi_page_block_size(page)); #endif @@ -83,12 +92,14 @@ mi_block_t* _mi_page_ptr_unalign(const mi_page_t* page, const void* p) { static void mi_decl_noinline mi_free_generic_local(mi_page_t* page, mi_segment_t* segment, void* p) mi_attr_noexcept { MI_UNUSED(segment); mi_block_t* const block = (mi_page_has_aligned(page) ? _mi_page_ptr_unalign(page, p) : (mi_block_t*)p); + mi_block_unguard(page,block); mi_free_block_local(page, block, true /* track stats */, true /* check for a full page */); } // free a pointer owned by another thread (page parameter comes first for better codegen) static void mi_decl_noinline mi_free_generic_mt(mi_page_t* page, mi_segment_t* segment, void* p) mi_attr_noexcept { mi_block_t* const block = _mi_page_ptr_unalign(page, p); // don't check `has_aligned` flag to avoid a race (issue #865) + mi_block_unguard(page, block); mi_free_block_mt(page, segment, block); } diff --git a/src/segment.c b/src/segment.c index ead8bb98..b03c7e85 100644 --- a/src/segment.c +++ b/src/segment.c @@ -745,6 +745,7 @@ static void mi_segment_page_clear(mi_segment_t* segment, mi_page_t* page, mi_seg void _mi_segment_page_free(mi_page_t* page, bool force, mi_segments_tld_t* tld) { mi_assert(page != NULL); + mi_assert_internal(!mi_page_has_guarded(page)); mi_segment_t* segment = _mi_page_segment(page); mi_assert_expensive(mi_segment_is_valid(segment,tld)); mi_pages_try_purge(false /*force?*/, tld); diff --git a/test/test-api-fill.c b/test/test-api-fill.c index 3fca3b9d..3baee83d 100644 --- a/test/test-api-fill.c +++ b/test/test-api-fill.c @@ -271,7 +271,7 @@ int main(void) { mi_free(p); }; - #if !(MI_TRACK_VALGRIND || MI_TRACK_ASAN) + #if !(MI_TRACK_VALGRIND || MI_TRACK_ASAN || MI_DEBUG_GUARDED) CHECK_BODY("fill-freed-small") { size_t malloc_size = MI_SMALL_SIZE_MAX / 2; uint8_t* p = (uint8_t*)mi_malloc(malloc_size);