[Asan] Cleanup atomic usage in allocator

[vitalybuka]

Summary

There are no know bugs related to this, still it may fix some latent ones.
Main concerns with preexisting code:

1. Inconsistent atomic/non-atomic access to the same field.
2. Assumption that bitfield chunk_state is always the first byte without even taking into account endianness.

[jfb]

Is this really NFC?

[vitalybuka]

In D86917#2249793, @jfb wrote:

Is this really NFC?

I guess so, do you have particular concern?
I don't know particular cases where it was broken, but reading values some times as atomic and sometimes as not, plus unclear order of bit fields looks suspicions.

[jfb]

In D86917#2250709, @vitalybuka wrote:

In D86917#2249793, @jfb wrote:

Is this really NFC?

I guess so, do you have particular concern?
I don't know particular cases where it was broken, but reading values some times as atomic and sometimes as not, plus unclear order of bit fields looks suspicions.

It looks like a bunch of latent bugs being fixed, which is by definition changing functionality from "might be broken" to "correct" :-)
I'd remove the NFC tag.

[vitalybuka]

It looks like a bunch of latent bugs being fixed, which is by definition changing functionality from "might be broken" to "correct" :-)
I'd remove the NFC tag.

Done

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1083–1084

Remove this extra check.

[vitalybuka]

remove duplicate check

[filcab]

Hi Vitaly,

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

Thank you
Filipe

[mstorsjo]

In D86917#2255845, @filcab wrote:

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

I'm also seeing that the build for windows is broken now, due to the COMPILER_CHECK(kChunkHeaderSize == 16); which is failing.

This can be reproduced in a smaller scale with this test snippet:

struct ChunkHeader {
  unsigned char chunk_state;
  unsigned int alloc_tid : 24;
};      

static_assert(sizeof(ChunkHeader) == 4, "unexpected size");

This fails when targeting Windows, as the Microsoft struct layout aligns the bitfield alloc_tid to a 4 byte boundary, following the preceding non-bitfield member.

[thakis]

Reverted this (and some follow-ups that caused conflicts during the revert) in rGdbf04aaade235a0d76c6ad549c091c9fd0ada0e8 to unbreak the Windows build.

compiler-rt/lib/asan/asan_allocator.cpp

73–74

This breaks Windows -- in the MS abi, only variables of the same type can be packed, and so the static_assert about the size fails.

...oh, mstorsjo alread reported this. Reverting, then :)

[vitalybuka]

In D86917#2255845, @filcab wrote:

Hi Vitaly,

Did you make ChunkHeader bigger on purpose? Can you at least change the
comments, as it’s no longer the same size?

Sorry, it was not intentional.

[vitalybuka]

fix Windows

compiler-rt/lib/asan/asan_allocator.cpp

 // ChunkBase in the next uptr.
 // M B L L L L L L L L L  H H U U U U U U
 //   |                    ^
 //   ---------------------|
 //   M -- magic value kAllocBegMagic
 //   B -- address of ChunkHeader pointing to the first 'H'
 static const uptr kAllocBegMagic = 0xCC6E96B9;
 
 struct ChunkHeader {
-  u8 chunk_state;  // Must be first.
-  u8 padding[3];
+  atomic_uint32_t chunk_state;
   u32 alloc_tid : 24;
   u32 from_memalign : 1;
   u32 alloc_type : 2;
   u32 rz_log : 3;
   u32 lsan_tag : 2;
   // 2-nd 8 bytes
   // This field is used for small sizes. For large sizes it is equal to
   // SizeClassMap::kMaxSize and the actual size is stored in the
   // SecondaryAllocator's metadata.
   u32 user_requested_size : 29;
   // align < 8 -> 0
   // else      -> log2(min(align, 512)) - 2
   u32 user_requested_alignment_log : 3;
-  u32 alloc_context_id;
+  atomic_uint32_t alloc_context_id;
 };
 
 struct ChunkBase : ChunkHeader {
   // Header2, intersects with user memory.
   u32 free_context_id;
   u32 free_tid;
 };
 

 
 struct QuarantineCallback {
   QuarantineCallback(AllocatorCache *cache, BufferedStackTrace *stack)
       : cache_(cache),
         stack_(stack) {
   }
 
   void Recycle(AsanChunk *m) {
-    CHECK_EQ(m->chunk_state, CHUNK_QUARANTINE);
-    atomic_store((atomic_uint8_t *)m, CHUNK_INVALID, memory_order_relaxed);
+    u32 old_chunk_state = CHUNK_QUARANTINE;
+    if (!atomic_compare_exchange_strong(&m->chunk_state, &old_chunk_state,
+                                        CHUNK_INVALID, memory_order_acquire)) {
+      CHECK_EQ(old_chunk_state, CHUNK_QUARANTINE);
+    }
+
     CHECK_NE(m->alloc_tid, kInvalidTid);
     CHECK_NE(m->free_tid, kInvalidTid);
     PoisonShadow(m->Beg(),
                  RoundUpTo(m->UsedSize(), SHADOW_GRANULARITY),
                  kAsanHeapLeftRedzoneMagic);
     void *p = reinterpret_cast<void *>(m->AllocBeg());
     if (p != m) {
       uptr *alloc_magic = reinterpret_cast<uptr *>(p);
void SetRssLimitExceeded(bool limit_exceeded) {
     atomic_store(&rss_limit_exceeded, limit_exceeded, memory_order_relaxed);
   }
 
   void RePoisonChunk(uptr chunk) {
     // This could be a user-facing chunk (with redzones), or some internal
     // housekeeping chunk, like TransferBatch. Start by assuming the former.
     AsanChunk *ac = GetAsanChunk((void *)chunk);
     uptr allocated_size = allocator.GetActuallyAllocatedSize((void *)ac);
+    if (atomic_load(&ac->chunk_state, memory_order_acquire) ==
+        CHUNK_ALLOCATED) {
       uptr beg = ac->Beg();
       uptr end = ac->Beg() + ac->UsedSize(true);
       uptr chunk_end = chunk + allocated_size;
-    if (chunk < beg && beg < end && end <= chunk_end &&
-        ac->chunk_state == CHUNK_ALLOCATED) {
+      if (chunk < beg && beg < end && end <= chunk_end) {
         // Looks like a valid AsanChunk in use, poison redzones only.
         PoisonShadow(chunk, beg - chunk, kAsanHeapLeftRedzoneMagic);
         uptr end_aligned_down = RoundDownTo(end, SHADOW_GRANULARITY);
         FastPoisonShadowPartialRightRedzone(
             end_aligned_down, end - end_aligned_down,
             chunk_end - end_aligned_down, kAsanHeapLeftRedzoneMagic);
-    } else {
-      // This is either not an AsanChunk or freed or quarantined AsanChunk.
-      // In either case, poison everything.
-      PoisonShadow(chunk, allocated_size, kAsanHeapLeftRedzoneMagic);
-    }
-  }
+        return;
+      }
+    }
+
 
   void ReInitialize(const AllocatorOptions &options) {
     SetAllocatorMayReturnNull(options.may_return_null);
     allocator.SetReleaseToOSIntervalMs(options.release_to_os_interval_ms);
     SharedInitCode(options);
 
     // Poison all existing allocation's redzones.
     if (CanPoisonMemory()) {
static uptr ComputeUserAlignment(uptr user_requested_alignment_log) {
     return 1LL << (user_requested_alignment_log + 2);
   }
 
   // We have an address between two chunks, and we want to report just one.
   AsanChunk *ChooseChunk(uptr addr, AsanChunk *left_chunk,
                          AsanChunk *right_chunk) {
     // Prefer an allocated chunk over freed chunk and freed chunk
     // over available chunk.
-    if (left_chunk->chunk_state != right_chunk->chunk_state) {
-      if (left_chunk->chunk_state == CHUNK_ALLOCATED)
+    u32 left_state =
+        atomic_load(&left_chunk->chunk_state, memory_order_relaxed);
+    u32 right_state =
+        atomic_load(&right_chunk->chunk_state, memory_order_relaxed);
+    if (left_state != right_state) {
+      if (left_state == CHUNK_ALLOCATED)
         return left_chunk;
-      if (right_chunk->chunk_state == CHUNK_ALLOCATED)
+      if (right_state == CHUNK_ALLOCATED)
         return right_chunk;
-      if (left_chunk->chunk_state == CHUNK_QUARANTINE)
+      if (left_state == CHUNK_QUARANTINE)
         return left_chunk;
-      if (right_chunk->chunk_state == CHUNK_QUARANTINE)
+      if (right_state == CHUNK_QUARANTINE)
         return right_chunk;
     }
     // Same chunk_state: choose based on offset.
     sptr l_offset = 0, r_offset = 0;
     CHECK(AsanChunkView(left_chunk).AddrIsAtRight(addr, 1, &l_offset));
     CHECK(AsanChunkView(right_chunk).AddrIsAtLeft(addr, 1, &r_offset));
     if (l_offset < r_offset)
       return left_chunk;
     return right_chunk;
   }
 
   bool UpdateAllocationStack(uptr addr, BufferedStackTrace *stack) {
     AsanChunk *m = GetAsanChunkByAddr(addr);
     if (!m) return false;
-    if (m->chunk_state != CHUNK_ALLOCATED) return false;
+    if (atomic_load(&m->chunk_state, memory_order_acquire) != CHUNK_ALLOCATED)
+      return false;
     if (m->Beg() != addr) return false;
-    atomic_store((atomic_uint32_t *)&m->alloc_context_id, StackDepotPut(*stack),
+    atomic_store(&m->alloc_context_id, StackDepotPut(*stack),
                  memory_order_relaxed);
     return true;
   }
 
   // -------------------- Allocation/Deallocation routines ---------------
   void *Allocate(uptr size, uptr alignment, BufferedStackTrace *stack,
                  AllocType alloc_type, bool can_fill) {
     if (UNLIKELY(!asan_inited))
if (using_primary_allocator) {
       CHECK(!allocator.FromPrimary(allocated));
       m->user_requested_size = SizeClassMap::kMaxSize;
       uptr *meta = reinterpret_cast<uptr *>(allocator.GetMetaData(allocated));
       meta[0] = size;
       meta[1] = chunk_beg;
     }
     m->user_requested_alignment_log = user_requested_alignment_log;
 
-    m->alloc_context_id = StackDepotPut(*stack);
+    atomic_store(&m->alloc_context_id, StackDepotPut(*stack),
+                 memory_order_relaxed);
 
     uptr size_rounded_down_to_granularity =
         RoundDownTo(size, SHADOW_GRANULARITY);
     // Unpoison the bulk of the memory region.
     if (size_rounded_down_to_granularity)
       PoisonShadow(user_beg, size_rounded_down_to_granularity, 0);
     // Deal with the end of the region if size is not aligned to granularity.
     if (size != size_rounded_down_to_granularity && CanPoisonMemory()) {
if (can_fill && fl.max_malloc_fill_size) {
       uptr fill_size = Min(size, (uptr)fl.max_malloc_fill_size);
       REAL(memset)(res, fl.malloc_fill_byte, fill_size);
     }
 #if CAN_SANITIZE_LEAKS
     m->lsan_tag = __lsan::DisabledInThisThread() ? __lsan::kIgnored
                                                  : __lsan::kDirectlyLeaked;
 #endif
     // Must be the last mutation of metadata in this function.
-    atomic_store((atomic_uint8_t *)m, CHUNK_ALLOCATED, memory_order_release);
+    atomic_store(&m->chunk_state, CHUNK_ALLOCATED, memory_order_release);
     ASAN_MALLOC_HOOK(res, size);
     return res;
   }
 
   // Set quarantine flag if chunk is allocated, issue ASan error report on
   // available and quarantined chunks. Return true on success, false otherwise.
   bool AtomicallySetQuarantineFlagIfAllocated(AsanChunk *m, void *ptr,
                                               BufferedStackTrace *stack) {
-    u8 old_chunk_state = CHUNK_ALLOCATED;
+    u32 old_chunk_state = CHUNK_ALLOCATED;
     // Flip the chunk_state atomically to avoid race on double-free.
-    if (!atomic_compare_exchange_strong((atomic_uint8_t *)m, &old_chunk_state,
+    if (!atomic_compare_exchange_strong(&m->chunk_state, &old_chunk_state,
                                         CHUNK_QUARANTINE,
                                         memory_order_acquire)) {
       ReportInvalidFree(ptr, old_chunk_state, stack);
       // It's not safe to push a chunk in quarantine on invalid free.
       return false;
     }
     CHECK_EQ(CHUNK_ALLOCATED, old_chunk_state);
     // It was a user data.
     m->free_tid = kInvalidTid;
     m->free_context_id = 0;
     return true;
   }
 
   // Expects the chunk to already be marked as quarantined by using
   // AtomicallySetQuarantineFlagIfAllocated.
   void QuarantineChunk(AsanChunk *m, void *ptr, BufferedStackTrace *stack) {
-    CHECK_EQ(m->chunk_state, CHUNK_QUARANTINE);
+    CHECK_EQ(atomic_load(&m->chunk_state, memory_order_relaxed),
+             CHUNK_QUARANTINE);
     AsanThread *t = GetCurrentThread();
     m->free_tid = t ? t->tid() : 0;
     m->free_context_id = StackDepotPut(*stack);
 
     Flags &fl = *flags();
     if (fl.max_free_fill_size > 0) {
       // We have to skip the chunk header, it contains free_context_id.
       uptr scribble_start = (uptr)m + kChunkHeaderSize + kChunkHeader2Size;
void *Reallocate(void *old_ptr, uptr new_size, BufferedStackTrace *stack) {
     AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);
 
     AsanStats &thread_stats = GetCurrentThreadStats();
     thread_stats.reallocs++;
     thread_stats.realloced += new_size;
 
     void *new_ptr = Allocate(new_size, 8, stack, FROM_MALLOC, true);
     if (new_ptr) {
-      u8 chunk_state = m->chunk_state;
+      u32 chunk_state = atomic_load(&m->chunk_state, memory_order_acquire);
       if (chunk_state != CHUNK_ALLOCATED)
         ReportInvalidFree(old_ptr, chunk_state, stack);
       CHECK_NE(REAL(memcpy), nullptr);
       uptr memcpy_size = Min(new_size, m->UsedSize());
       // If realloc() races with free(), we may start copying freed memory.
       // However, we will report racy double-free later anyway.
       REAL(memcpy)(new_ptr, old_ptr, memcpy_size);
       Deallocate(old_ptr, 0, 0, stack, FROM_MALLOC);
void *Calloc(uptr nmemb, uptr size, BufferedStackTrace *stack) {
     void *ptr = Allocate(nmemb * size, 8, stack, FROM_MALLOC, false);
     // If the memory comes from the secondary allocator no need to clear it
     // as it comes directly from mmap.
     if (ptr && allocator.FromPrimary(ptr))
       REAL(memset)(ptr, 0, nmemb * size);
     return ptr;
   }
 
-  void ReportInvalidFree(void *ptr, u8 chunk_state, BufferedStackTrace *stack) {
+  void ReportInvalidFree(void *ptr, u32 chunk_state,
+                         BufferedStackTrace *stack) {
     if (chunk_state == CHUNK_QUARANTINE)
       ReportDoubleFree((uptr)ptr, stack);
     else
       ReportFreeNotMalloced((uptr)ptr, stack);
   }
 
   void CommitBack(AsanThreadLocalMallocStorage *ms, BufferedStackTrace *stack) {
     AllocatorCache *ac = GetAllocatorCache(ms);
AsanChunk *GetAsanChunkByAddrFastLockedDebug(uptr p) {
     Printf("GetAsanChunkByAddrFastLockedDebug p %p alloc_beg %p\n", p,
            alloc_beg);
     return GetAsanChunkDebug(alloc_beg);
   }
 
   uptr AllocationSize(uptr p) {
     AsanChunk *m = GetAsanChunkByAddr(p);
     if (!m) return 0;
-    if (m->chunk_state != CHUNK_ALLOCATED) return 0;
+    if (atomic_load(&m->chunk_state, memory_order_acquire) != CHUNK_ALLOCATED)
+      return 0;
     if (m->Beg() != p) return 0;
     return m->UsedSize();
   }
 
   AsanChunkView FindHeapChunkByAddress(uptr addr) {
     AsanChunk *m1 = GetAsanChunkByAddr(addr);
     if (!m1) return AsanChunkView(m1);
     sptr offset = 0;

 
 static Allocator instance(LINKER_INITIALIZED);
 
 static AsanAllocator &get_allocator() {
   return instance.allocator;
 }
 
 bool AsanChunkView::IsValid() const {
-  return chunk_ && chunk_->chunk_state != CHUNK_INVALID;
+  return chunk_ && atomic_load(&chunk_->chunk_state, memory_order_relaxed) !=
+                       CHUNK_INVALID;
 }
 bool AsanChunkView::IsAllocated() const {
-  return chunk_ && chunk_->chunk_state == CHUNK_ALLOCATED;
+  return chunk_ && atomic_load(&chunk_->chunk_state, memory_order_relaxed) ==
+                       CHUNK_ALLOCATED;
 }
 bool AsanChunkView::IsQuarantined() const {
-  return chunk_ && chunk_->chunk_state == CHUNK_QUARANTINE;
+  return chunk_ && atomic_load(&chunk_->chunk_state, memory_order_relaxed) ==
+                       CHUNK_QUARANTINE;
 }
 uptr AsanChunkView::Beg() const { return chunk_->Beg(); }
 uptr AsanChunkView::End() const { return Beg() + UsedSize(); }
 uptr AsanChunkView::UsedSize() const { return chunk_->UsedSize(); }
 u32 AsanChunkView::UserRequestedAlignment() const {
   return Allocator::ComputeUserAlignment(chunk_->user_requested_alignment_log);
 }
 uptr AsanChunkView::AllocTid() const { return chunk_->alloc_tid; }
 uptr AsanChunkView::FreeTid() const {
   return IsQuarantined() ? chunk_->free_tid : kInvalidTid;
 }
 AllocType AsanChunkView::GetAllocType() const {
   return (AllocType)chunk_->alloc_type;
 }
 
 static StackTrace GetStackTraceFromId(u32 id) {
   CHECK(id);
   StackTrace res = StackDepotGet(id);
   CHECK(res.trace);
   return res;
 }
 
-u32 AsanChunkView::GetAllocStackId() const { return chunk_->alloc_context_id; }
+u32 AsanChunkView::GetAllocStackId() const {
+  return atomic_load(&chunk_->alloc_context_id, memory_order_relaxed);
+}
 u32 AsanChunkView::GetFreeStackId() const {
   return IsQuarantined() ? chunk_->free_context_id : 0;
 }
 
 StackTrace AsanChunkView::GetAllocStack() const {
   return GetStackTraceFromId(GetAllocStackId());
 }
 

 
 void GetAllocatorGlobalRange(uptr *begin, uptr *end) {
   *begin = (uptr)&__asan::get_allocator();
   *end = *begin + sizeof(__asan::get_allocator());
 }
 
 uptr PointsIntoChunk(void* p) {
   uptr addr = reinterpret_cast<uptr>(p);
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddrFastLocked(addr);
-  if (!m) return 0;
-  uptr chunk = m->Beg();
-  if (m->chunk_state != __asan::CHUNK_ALLOCATED)
+  if (!m || atomic_load(&m->chunk_state, memory_order_acquire) !=
+                __asan::CHUNK_ALLOCATED)
     return 0;
+  uptr chunk = m->Beg();
   if (m->AddrIsInside(addr, /*locked_version=*/true))
     return chunk;
   if (IsSpecialCaseOfOperatorNew0(chunk, m->UsedSize(/*locked_version*/ true),
                                   addr))
     return chunk;
   return 0;
 }
 

 }
 
 LsanMetadata::LsanMetadata(uptr chunk) {
   metadata_ = reinterpret_cast<void *>(chunk - __asan::kChunkHeaderSize);
 }
 
 bool LsanMetadata::allocated() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
-  return m->chunk_state == __asan::CHUNK_ALLOCATED;
+  return atomic_load(&m->chunk_state, memory_order_relaxed) ==
+         __asan::CHUNK_ALLOCATED;
 }
 
 ChunkTag LsanMetadata::tag() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   return static_cast<ChunkTag>(m->lsan_tag);
 }
 
 void LsanMetadata::set_tag(ChunkTag value) {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   m->lsan_tag = value;
 }
 
 uptr LsanMetadata::requested_size() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
   return m->UsedSize(/*locked_version=*/true);
 }
 
 u32 LsanMetadata::stack_trace_id() const {
   __asan::AsanChunk *m = reinterpret_cast<__asan::AsanChunk *>(metadata_);
-  return m->alloc_context_id;
+  return atomic_load(&m->alloc_context_id, memory_order_relaxed);
 }
 
 void ForEachChunk(ForEachChunkCallback callback, void *arg) {
   __asan::get_allocator().ForEachChunk(callback, arg);
 }
 
 IgnoreObjectResult IgnoreObjectLocked(const void *p) {
   uptr addr = reinterpret_cast<uptr>(p);
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddr(addr);
   if (!m) return kIgnoreObjectInvalid;
-  if ((m->chunk_state == __asan::CHUNK_ALLOCATED) && m->AddrIsInside(addr)) {
+  if ((atomic_load(&m->chunk_state, memory_order_acquire) ==
+       __asan::CHUNK_ALLOCATED) &&
+      m->AddrIsInside(addr)) {
     if (m->lsan_tag == kIgnored)
       return kIgnoreObjectAlreadyIgnored;
     m->lsan_tag = __lsan::kIgnored;
     return kIgnoreObjectSuccess;
   }
   return kIgnoreObjectInvalid;
 }
 }  // namespace __lsan