fix various false positives in test-stress from valgrind

include/mimalloc-track.h

@@ -13,9 +13,6 @@ terms of the MIT license. A copy of the license can be found in the file
 // or other memory checkers.
 // ------------------------------------------------------
 
-
-#define MI_VALGRIND 1
-
 #if MI_VALGRIND
 
 #define MI_TRACK_ENABLED 1
@@ -23,9 +20,9 @@ terms of the MIT license. A copy of the license can be found in the file
 #include <valgrind/valgrind.h>
 #include <valgrind/memcheck.h>
 
-#define mi_track_malloc(p,size,zero)        VALGRIND_MALLOCLIKE_BLOCK(p,size,0 /*red zone*/,zero)
+#define mi_track_malloc(p,size,zero)        VALGRIND_MALLOCLIKE_BLOCK(p,size,MI_PADDING_SIZE /*red zone*/,(zero?1:0))
-#define mi_track_resize(p,oldsize,newsize)  VALGRIND_RESIZEINPLACE_BLOCK(p,oldsize,newsize,0 /*red zone*/)  
+#define mi_track_resize(p,oldsize,newsize)  VALGRIND_RESIZEINPLACE_BLOCK(p,oldsize,newsize,MI_PADDING_SIZE /*red zone*/)  
-#define mi_track_free(p)                    VALGRIND_FREELIKE_BLOCK(p,0 /*red zone*/)
+#define mi_track_free(p)                    VALGRIND_FREELIKE_BLOCK(p,MI_PADDING_SIZE /*red zone*/)
 #define mi_track_mem_defined(p,size)        VALGRIND_MAKE_MEM_DEFINED(p,size)
 #define mi_track_mem_undefined(p,size)      VALGRIND_MAKE_MEM_UNDEFINED(p,size)
 #define mi_track_mem_noaccess(p,size)       VALGRIND_MAKE_MEM_NOACCESS(p,size)

include/mimalloc-types.h

@@ -29,6 +29,9 @@ terms of the MIT license. A copy of the license can be found in the file
 // Define NDEBUG in the release version to disable assertions.
 // #define NDEBUG
 
+// Define MI_VALGRIND to enable valgrind support
+#define MI_VALGRIND 1
+
 // Define MI_STAT as 1 to maintain statistics; set it to 2 to have detailed statistics (but costs some performance).
 // #define MI_STAT 1
 
@@ -56,7 +59,7 @@ terms of the MIT license. A copy of the license can be found in the file
 
 // Reserve extra padding at the end of each block to be more resilient against heap block overflows.
 // The padding can detect byte-precise buffer overflow on free.
-#if !defined(MI_PADDING) && (MI_DEBUG>=1)
+#if !defined(MI_PADDING) && (MI_DEBUG>=1 || MI_VALGRIND)
 #define MI_PADDING  1
 #endif
 

src/alloc.c

@@ -40,7 +40,10 @@ extern inline void* _mi_page_malloc(mi_heap_t* heap, mi_page_t* page, size_t siz
 
   // allow use of the block internally 
   // todo: can we optimize this call away for non-zero'd release mode?
-  mi_track_mem_undefined(block,mi_page_block_size(page));
+  #if MI_TRACK_ENABLED
+  const size_t track_bsize = mi_page_block_size(page);
+  if (zero) mi_track_mem_defined(block,track_bsize); else mi_track_mem_undefined(block,track_bsize);
+  #endif
 
   // zero the block? note: we need to zero the full block size (issue #63)
   if mi_unlikely(zero) {
@@ -70,7 +73,10 @@ extern inline void* _mi_page_malloc(mi_heap_t* heap, mi_page_t* page, size_t siz
 #if (MI_PADDING > 0) && defined(MI_ENCODE_FREELIST)
   mi_padding_t* const padding = (mi_padding_t*)((uint8_t*)block + mi_page_usable_block_size(page));
   ptrdiff_t delta = ((uint8_t*)padding - (uint8_t*)block - (size - MI_PADDING_SIZE));
+  #if (MI_DEBUG>1)
   mi_assert_internal(delta >= 0 && mi_page_usable_block_size(page) >= (size - MI_PADDING_SIZE + delta));
+  mi_track_mem_defined(padding,sizeof(mi_padding_t));  // note: re-enable since mi_page_usable_block_size may set noaccess
+  #endif
   padding->canary = (uint32_t)(mi_ptr_encode(page,block,page->keys));
   padding->delta  = (uint32_t)(delta);
   uint8_t* fill = (uint8_t*)padding - delta;
@@ -79,7 +85,7 @@ extern inline void* _mi_page_malloc(mi_heap_t* heap, mi_page_t* page, size_t siz
 #endif
 
   // mark as no-access again
-  mi_track_mem_noaccess(block,mi_page_block_size(page));
+  mi_track_mem_noaccess(block,track_bsize);
   return block;
 }
 
@@ -215,10 +221,14 @@ static inline bool mi_check_is_double_free(const mi_page_t* page, const mi_block
 static bool mi_page_decode_padding(const mi_page_t* page, const mi_block_t* block, size_t* delta, size_t* bsize) {
   *bsize = mi_page_usable_block_size(page);
   const mi_padding_t* const padding = (mi_padding_t*)((uint8_t*)block + *bsize);
-  mi_track_mem_defined(padding,sizeof(*padding));
+  mi_track_mem_defined(padding,sizeof(mi_padding_t));
   *delta = padding->delta;
-  bool ok = ((uint32_t)mi_ptr_encode(page,block,page->keys) == padding->canary && *delta <= *bsize);
+  uint32_t canary = padding->canary;
-  mi_track_mem_noaccess(padding,sizeof(*padding));
+  uintptr_t keys[2]; 
+  keys[0] = page->keys[0];
+  keys[1] = page->keys[1]; 
+  bool ok = ((uint32_t)mi_ptr_encode(page,block,keys) == canary && *delta <= *bsize);
+  mi_track_mem_noaccess(padding,sizeof(mi_padding_t));
   return ok;
 }
 
@@ -349,9 +359,9 @@ static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* bloc
   // The padding check may access the non-thread-owned page for the key values.
   // that is safe as these are constant and the page won't be freed (as the block is not freed yet).
   mi_check_padding(page, block);
-  mi_padding_shrink(page, block, sizeof(mi_block_t)); // for small size, ensure we can fit the delayed thread pointers without triggering overflow detection
+  mi_track_mem_defined(block, mi_page_block_size(page));    // ensure the padding can be accessed
-  #if (MI_DEBUG!=0)
+  mi_padding_shrink(page, block, sizeof(mi_block_t));       // for small size, ensure we can fit the delayed thread pointers without triggering overflow detection
-  mi_track_mem_undefined(block, mi_page_block_size(page));  // note: check padding may set parts to noaccess
+  #if (MI_DEBUG!=0) && !MI_TRACK_ENABLED                    // note: when tracking, cannot use mi_usable_size with multi-threading
   memset(block, MI_DEBUG_FREED, mi_usable_size(block));
   #endif
 
@@ -406,6 +416,7 @@ static mi_decl_noinline void _mi_free_block_mt(mi_page_t* page, mi_block_t* bloc
 static inline void _mi_free_block(mi_page_t* page, bool local, mi_block_t* block)
 {
   // and push it on the free list
+  //const size_t bsize = mi_page_block_size(page);
   if mi_likely(local) {
     // owning thread can free a block directly
     if mi_unlikely(mi_check_is_double_free(page, block)) return;
@@ -442,8 +453,13 @@ mi_block_t* _mi_page_ptr_unalign(const mi_segment_t* segment, const mi_page_t* p
 static void mi_decl_noinline mi_free_generic(const mi_segment_t* segment, bool local, void* p) mi_attr_noexcept {
   mi_page_t* const page = _mi_segment_page_of(segment, p);
   mi_block_t* const block = (mi_page_has_aligned(page) ? _mi_page_ptr_unalign(segment, page, p) : (mi_block_t*)p);
-  mi_stat_free(page, block);
+  //#if MI_TRACK_ENABLED
+  //const size_t track_bsize = mi_page_block_size(page);
+  //#endif
+  //mi_track_mem_defined(block,track_bsize);    
+  mi_stat_free(page, block);                 // stat_free may access the padding
   _mi_free_block(page, local, block);
+  //mi_track_mem_noaccess(block,track_bsize);  // use track_bsize as the page may have been deallocated by now
 }
 
 // Get the segment data belonging to a pointer
@@ -485,27 +501,22 @@ void mi_free(void* p) mi_attr_noexcept
 {
   mi_segment_t* const segment = mi_checked_ptr_segment(p,"mi_free");
   if mi_unlikely(segment == NULL) return; 
-  mi_track_free(p);
 
   mi_threadid_t tid = _mi_thread_id();
   mi_page_t* const page = _mi_segment_page_of(segment, p);
   mi_block_t* const block = (mi_block_t*)p;
 
-  #if MI_TRACK_ENABLED
-  const size_t track_bsize = mi_page_block_size(page);
-  #endif
-  
   if mi_likely(tid == mi_atomic_load_relaxed(&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
     if mi_unlikely(mi_check_is_double_free(page,block)) return;      
     mi_check_padding(page, block);
     mi_stat_free(page, block);
-    #if (MI_DEBUG!=0)
+    #if (MI_DEBUG!=0) && !MI_TRACK_ENABLED
-    mi_track_mem_undefined(block,track_bsize);  // note: check padding may set parts to noaccess
     memset(block, MI_DEBUG_FREED, mi_page_block_size(page));
     #endif
     mi_block_set_next(page, block, page->local_free);
     page->local_free = block;
+    // mi_track_mem_noaccess(block,mi_page_block_size(page));
     if mi_unlikely(--page->used == 0) {   // using this expression generates better code than: page->used--; if (mi_page_all_free(page))    
       _mi_page_retire(page);
     }
@@ -513,10 +524,9 @@ void mi_free(void* p) mi_attr_noexcept
   else {
     // non-local, aligned blocks, or a full page; use the more generic path
     // note: recalc page in generic to improve code generation
-    mi_track_mem_undefined(block,track_bsize);
     mi_free_generic(segment, tid == segment->thread_id, p);
   }  
-  mi_track_mem_noaccess(block,track_bsize);  // cannot use mi_page_block_size as the segment might be deallocated by now
+  mi_track_free(p);
 }
 
 bool _mi_free_delayed_block(mi_block_t* block) {

src/region.c

@@ -330,7 +330,7 @@ static void* mi_region_try_alloc(size_t blocks, bool* commit, bool* large, bool*
   }
   mi_assert_internal(!_mi_bitmap_is_any_claimed(&region->reset, 1, blocks, bit_idx));
   
-  #if (MI_DEBUG>=2)
+  #if (MI_DEBUG>=2) && !MI_TRACK_ENABLED
   if (*commit) { ((uint8_t*)p)[0] = 0; }
   #endif
   
@@ -376,9 +376,9 @@ void* _mi_mem_alloc_aligned(size_t size, size_t alignment, bool* commit, bool* l
 
   if (p != NULL) {
     mi_assert_internal((uintptr_t)p % alignment == 0);
-#if (MI_DEBUG>=2)
+    #if (MI_DEBUG>=2) && !MI_TRACK_ENABLED
     if (*commit) { ((uint8_t*)p)[0] = 0; } // ensure the memory is committed
-#endif
+    #endif
   }
   return p;
 }

src/segment.c

@@ -531,6 +531,7 @@ static mi_segment_t* mi_segment_init(mi_segment_t* segment, size_t required, mi_
   // Try to get it from our thread local cache first
   if (segment != NULL) {
     // came from cache
+    mi_track_mem_defined(segment,info_size);  
     mi_assert_internal(segment->segment_size == segment_size);
     if (page_kind <= MI_PAGE_MEDIUM && segment->page_kind == page_kind && segment->segment_size == segment_size) {
       pages_still_good = true;
@@ -584,6 +585,7 @@ static mi_segment_t* mi_segment_init(mi_segment_t* segment, size_t required, mi_
         return NULL;  
       }
     }
+    mi_track_mem_undefined(segment,info_size);
     segment->memid = memid;
     segment->mem_is_pinned = (mem_large || is_pinned);
     segment->mem_is_committed = commit;    
@@ -591,7 +593,6 @@ static mi_segment_t* mi_segment_init(mi_segment_t* segment, size_t required, mi_
   }  
   mi_assert_internal(segment != NULL && (uintptr_t)segment % MI_SEGMENT_SIZE == 0);
   mi_assert_internal(segment->mem_is_pinned ? segment->mem_is_committed : true);    
-  mi_track_mem_defined(segment,info_size);
     
   mi_atomic_store_ptr_release(mi_segment_t, &segment->abandoned_next, NULL);  // tsan
   if (!pages_still_good) {

test/test-wrong.c

@@ -16,17 +16,20 @@ int main(int argc, char** argv) {
   mi_free(r);
 
   // undefined access
-  // printf("undefined: %d\n", *q);
+  printf("undefined: %d\n", *q);
   
   *q = 42;
 
   // buffer overflow
-  // q[1] = 43;
+  q[1] = 43;
+
+  // buffer underflow
+  q[-1] = 44;
   
   mi(free)(q);
 
   // double free
-  // mi(free)(q);
+  mi(free)(q);
 
   // use after free
   printf("use-after-free: %d\n", *q);