Container annotations (-fstanitize=address) crash when used with sizeof(*buffer) < shadow granularity

[mlippautz]

When using container annotations (__sanitizer_annotate_contiguous_container) with sizeof(*buffer) < SHADOW_GRANULARITY ASAN leaves behind non-0 shadow memory state in certain scenarios (e.g. when used with uint32_t (half word)).

The problem boils down to the question whether

 __sanitizer_annotate_contiguous_container(
    /* begin = */ buffer, 
    /* end = */ buffer + capacity,
    /* old mid = */ buffer + capacity,
    /* new mid = */ buffer + capacity);

is allowed to leave behind non-0 shadow memory state that must be cleaned up by the allocator in case buffer is freed and later reused with a different capacity. From the API description I would have assumed that this is the valid start/end state that is fully cleaned up.

Note that upwards aligning end doesn't change anything as begin and end are merely used for containments checks but not for computing the poisoned ranges.

This show up as issue in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1292392

Self-contained repro:

// clang++ -o asan_container -std=c++11 -fsanitize=address asan_container.cc
#include <cstddef>
#include <sanitizer/common_interface_defs.h>

// Simple malloc emulation satisfying 8 byte aligned memory.
alignas(void*) static uint32_t global_buffer[5];
void* my_malloc() { return global_buffer; }
void my_free(void*) { }

int main(int argc, char** argv) {
   size_t capacity = 1;
   // Get a new buffer.
   uint32_t* buffer = static_cast<uint32_t*>(my_malloc());
   if (buffer) {
    // 1. Allow whole range.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + capacity);
    // 2. Allow only up till size.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 0);
  }
  // push_back()
  if (buffer) {
    // 3. Increase size.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 1);
  }
  buffer[0] = 'c';
  // Delete buffer.
  if (buffer) {
    // 4. Allow whole range.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + capacity);
  }
  my_free(buffer);

  // Get a new buffer with increased capacity.
  capacity = 2;
  // my_malloc() returns the same block by coincidence.
  buffer = static_cast<uint32_t*>(my_malloc());
  if (buffer) {
    // 5. Allow whole range.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + capacity);
    // 6. Allow only up till size.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 0);
  }
  // push_back()
  if (buffer) {
    // 7. Increase size.
    // BOOM:
    // Step 4. actually left behind non-0 shadow memory.
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 1);
  }
  if (buffer) {}
  return 0;
}

Crash:

AddressSanitizer: CHECK failed: asan_poisoning.cpp:377 "((*(u8*)MemToShadow(a))) == ((0))" (0x4, 0x0) (tid=795799)
    #0 0x4a3541 in __asan::CheckUnwind() asan_rtl.cpp.o
    #1 0x4b8964 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) (/usr/local/google/home/mlippautz/workspace/temp/test+0x4b8964)
    #2 0x49ecf7 in __sanitizer_annotate_contiguous_container (/usr/local/google/home/mlippautz/workspace/temp/test+0x49ecf7)
    #3 0x4d02e1 in main (/usr/local/google/home/mlippautz/workspace/temp/test+0x4d02e1)
    #4 0x7f630203a7ec in __libc_start_main csu/../csu/libc-start.c:332:16
    #5 0x41f289 in _start (/usr/local/google/home/mlippautz/workspace/temp/test+0x41f289)

[AdvenamTacet]

Hey, thank you for the example! It looks like __sanitizer_annotate_contiguous_container is used incorrectly in the self-contained repro you provided.
If you look at the definition of __sanitizer_annotate_contiguous_container, you can see that arguments are:

/// \param beg Beginning of memory region.
/// \param end End of memory region.
/// \param old_mid Old middle of memory region.
/// \param new_mid New middle of memory region.
void __sanitizer_annotate_contiguous_container(const void *beg,
                                               const void *end,
                                               const void *old_mid,
                                               const void *new_mid);

The old_mid pointer should always point to the end of the old content of the contiguous container. This is because the __sanitizer_annotate_contiguous_container function only modifies the annotations between old_mid and new_mid. If old_mid is not pointing to the end of the old content, then the annotations may be incorrect.

Explanation of the issue:

   if (buffer) {
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + capacity);
   // ^ That's correct, but does not change anything, at the beginning the whole buffer is accessible

    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 0);
    // ^ That's correct, `old_mid == buffer + capacity`, the mid is moved to the very beginning of the buffer (everything is posioned)
  }

  if (buffer) {
    __sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + 1);
   // ^ That's NOT correct, `old_mid` should be the very beginning of buffer and here the argument is `buffer + capacity`.
   // The correct line is `__sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer, buffer + 1);`
  }

Fixed code is working without the error: https://godbolt.org/z/cjxY5odPW (I had to fix more than just that one line, but all fixes are same.)
I don't see the problem with __sanitizer_annotate_contiguous_container in Chromium, but I mostly looked at your example. I'm closing it, but if I'm wrong and the issue exists, just not in that snippet, let me know and I will fix it.

Also, this code won't generate this error since LLVM 17 thanks to 0b2c0dc63faa, but it's still incorrect. Checks were removed to allow unpoisoning containers memory without crashing the program (in that case, false negatives can happen, but not false positives). Providing incorrect old_mid may (and will) result in false positives (errors).

Edit:
I focused on the code and missed the question, sorry. The answer is: container should unpoison memory during destruction, but when using the default allocator (or malloc/free), everything works regardless (it poisons whole memory during deallocation and later unpoisons during next allocation, without touching the memory). However, when using a custom allocator, you are responsible for taking care of annotations.

Unpoisoning the memory by container is important even when you are using a simple wrapper to the default allocator which cleans the memory (set every byte to zero) before freeing it. Memory cannot be poisoned when touched by allocator (unless you explicitly turn off instrumentation).

Container unpoisoning memory is a good practice (and required by many allocators), but with default allocators it works without it. __sanitizer_annotate_contiguous_container just simplifies working with containers, but it's same as just poisoning/unpoisoning memory between old_mid and new_mid.
You cannot write

__sanitizer_annotate_contiguous_container(buffer, buffer + capacity, buffer + capacity, buffer + capacity);

when the container is [partially] poisoned, old_mid has to be equal to the beginning of poisoned memory.

TL;DR:
If you are using the default allocator, it poisons/unpoisons the whole buffer regardless of the state in which container left it, but if you want the same behavior in a custom allocator, you have to implement it (probably with __asan_poison_memory_region and __asan_unpoison_memory_region.