Confusion Regarding ASAN

[Niteip]

Hello, I enabled the mimalloc ASAN feature and noticed a minor issue: insufficient accuracy.

For example, in code like this:

char* ptr = (char*)mi_malloc(20);  
ptr[20] = 1; // Deliberate out-of-bounds access here. ASAN should report an error.  

No error was triggered. However, when I tried a larger out-of-bounds access:

char* ptr = (char*)mi_malloc(20);  
ptr[24] = 1; // Deliberate larger out-of-bounds access here.  

It finally reported an error: "use of poisoned memory."

This suggests that mimalloc ASAN's detection is not precise enough.

My development environment: Windows, Visual Studio 2022, mimalloc 2.1.7, using the mimalloc static library with MI_TRACK_ASAN=ON and MI_OVERRIDE=OFF.

I look forward to your assistance.

[daanx]

Thanks! Does this also happen in Debug mode or only in Release builds?

[Niteip]

Thanks! Does this also happen in Debug mode or only in Release builds?

Thank you for your reply. Through testing, I've observed that this behavior occurs in static libraries compiled under both "Debug" and "RelWithDebInfo" configurations.

[Morakito]

@daanx @Niteip
I guess the reason may be as follows:

1. 20 bytes is a small object. Therefore, _mi_heap_get_free_small_page is used to get a page with the appropriate block_size directly from heap->pages_free_direct.
2. MI_PADDING is enabled in Debug mode. Mimalloc inserts some padding bytes and a mi_padding_t struct at the end of each block.
3. heap->pages_free_direct is 8-byte aligned (64-bit). Therefore, 20 bytes are requested, and together with the size of the mi_padding_t struct (MI_PADDING_SIZE, 8 bytes), we end up with a page block size of 32 bytes. The size of padding bytes is 4 bytes.

4. Mimalloc marks memory POISON or UNPOISON using a size that includes some padding bytes (page->block_size - MI_PADDING_SIZE), in this case is 4 bytes.

5. Therefore, ptr[20] through ptr[23] are actually padding bytes, and their in-memory value is MI_DEBUG_PADDING(0xDE). And they are now marked as UNPOISON, so no errors are reported. However, since ptr[24], we have reached the memory area of the mi_padding_t struct , which is in the POISON state and therefore starts reporting errors.

The above is my naive understanding. I don't know if this is the real reason that leads to the imprecise asan of mimalloc in this issue. Maybe we just need to mark UNPOISON for the specific data instead of the padding bytes and mi_padding_t struct?

Looking forward to your reply!

[Niteip]

@daanx @Niteip I guess the reason may be as follows:

1. 20 bytes is a small object. Therefore, _mi_heap_get_free_small_page is used to get a page with the appropriate block_size directly from heap->pages_free_direct.
2. MI_PADDING is enabled in Debug mode. Mimalloc inserts some padding bytes and a mi_padding_t struct at the end of each block.
3. heap->pages_free_direct is 8-byte aligned (64-bit). Therefore, 20 bytes are requested, and together with the size of the mi_padding_t struct (MI_PADDING_SIZE, 8 bytes), we end up with a page block size of 32 bytes. The size of padding bytes is 4 bytes.

4. Mimalloc marks memory POISON or UNPOISON using a size that includes some padding bytes (page->block_size - MI_PADDING_SIZE), in this case is 4 bytes.

5. Therefore, ptr[20] through ptr[23] are actually padding bytes, and their in-memory value is MI_DEBUG_PADDING(0xDE). And they are now marked as UNPOISON, so no errors are reported. However, since ptr[24], we have reached the memory area of the mi_padding_t struct , which is in the POISON state and therefore starts reporting errors.

The above is my naive understanding. I don't know if this is the real reason that leads to the imprecise asan of mimalloc in this issue. Maybe we just need to mark UNPOISON for the specific data instead of the padding bytes and mi_padding_t struct?

Looking forward to your reply!

Thank you for your reply. I think what you said makes a lot of sense.
MI_PADDING is always turned on with ASAN/Valgrind.

Moreover, I conducted a test where I specifically modified the code (in types.h), changing
#define MI_PADDING 1
to
#define MI_PADDING 0

After compiling it into a static library, my test code was successfully detected as "poisoned" by ASAN.

[Niteip]

@daanx @Niteip I guess the reason may be as follows:

1. 20 bytes is a small object. Therefore, _mi_heap_get_free_small_page is used to get a page with the appropriate block_size directly from heap->pages_free_direct.
2. MI_PADDING is enabled in Debug mode. Mimalloc inserts some padding bytes and a mi_padding_t struct at the end of each block.
3. heap->pages_free_direct is 8-byte aligned (64-bit). Therefore, 20 bytes are requested, and together with the size of the mi_padding_t struct (MI_PADDING_SIZE, 8 bytes), we end up with a page block size of 32 bytes. The size of padding bytes is 4 bytes.

4. Mimalloc marks memory POISON or UNPOISON using a size that includes some padding bytes (page->block_size - MI_PADDING_SIZE), in this case is 4 bytes.

5. Therefore, ptr[20] through ptr[23] are actually padding bytes, and their in-memory value is MI_DEBUG_PADDING(0xDE). And they are now marked as UNPOISON, so no errors are reported. However, since ptr[24], we have reached the memory area of the mi_padding_t struct , which is in the POISON state and therefore starts reporting errors.

The above is my naive understanding. I don't know if this is the real reason that leads to the imprecise asan of mimalloc in this issue. Maybe we just need to mark UNPOISON for the specific data instead of the padding bytes and mi_padding_t struct?

Looking forward to your reply!

Sorry, what I said before was not rigorous enough.

When #define MI_PADDING 0,

test code:

size_t count = 8;
char* ptr = (char*)mi_malloc(count);  
ptr[count] = 1; // Deliberate out-of-bounds access here. ASAN should report an error.

After multiple tests, I found that different values of count yield different results.

count = 8; // ASAN reports an error as expected
count = 16; // ASAN reports an error as expected
count = 20; // Unexpected, ASAN does not report an error
count = 24; // Unexpected, ASAN does not report an error
count = 32; // ASAN reports an error as expected

[Morakito]

@Niteip
Thank you for sharing the additional test cases. In the course of the discussions with you, I have gained a deeper understanding of the part of mimalloc that deals with ASAN.

In mimalloc, different types of pages (small, medium, large, or huge) are allocated depending on how much memory is requested. There are some differences in zero-initialization and other aspects between huge page and normal page (small, medium, large).

1. Padding is performed at the end of the _mi_page_malloc_zero function(in alloc.c) (if MI_PADDING is enabled) after memory is requested. But the padding bytes and mi_padding_t struct do not appear to be marked POISON, which we can do here use mi_track_mem_noaccess. However, this only solves part of the problem and does not provide ASAN support for huge pages.
2. When a huge page is not initialized with zero, it is initialized with the _mi_page_malloc_zero function just like normal pages. When initializing with zero, we first fetch a huge page using _mi_page_malloc (actually _mi_page_malloc_zero(heap,page,size,false)) and then zero it manually.

The problem here is that we cannot POISON part of the bytes in the huge page in the end of the _mi_page_malloc_zero function, otherwise we would not be able to zeroing the huge page manually. So we can mark the bytes POISON after we get the page. It looks like this：

  // and try again, this time succeeding! (i.e. this should never recurse through _mi_page_malloc)
  void* p;
  if mi_unlikely(zero && mi_page_is_huge(page)) {
    // note: we cannot call _mi_page_malloc with zeroing for huge blocks; we zero it afterwards in that case.
    p = _mi_page_malloc(heap, page, size);
    mi_assert_internal(p != NULL);
    _mi_memzero_aligned(p, mi_page_usable_block_size(page));
  }
  else {
    p = _mi_page_malloc_zero(heap, page, size, zero);
    mi_assert_internal(p != NULL);
  }

// Here is the new code that marks 
// the padding bytes and mi_padding_t struct as POISON
#if MI_PADDING
  mi_padding_t* const padding = (mi_padding_t*)((uint8_t*)p + mi_page_usable_block_size(page));
  ptrdiff_t delta = ((uint8_t*)padding - (uint8_t*)p - (size - MI_PADDING_SIZE));

  uint8_t* fill = (uint8_t*)padding - delta;
  const size_t maxpad = (delta > MI_MAX_ALIGN_SIZE ? MI_MAX_ALIGN_SIZE : delta);
  mi_track_mem_noaccess(fill, maxpad + sizeof(mi_padding_t));
#endif

  // move singleton pages to the full queue
  if (page->reserved == page->used) {
    mi_page_to_full(page, mi_page_queue_of(page));
  }
  return p;

I did a simple test of the above code and it passed all 42 tests in test-api.c. It works well for both huge page and normal page, including the case you provided.

I'm not particularly sure this is foolproof, it hasn't been heavily tested and could be potentially buggy. Is there a better and more elegant implementation?

I look forward to your reply and any discussion on this topic.