[asan] Fix internal CHECK failure on double free in recovery mode.

[m.ostapenko]

Summary

As reported in https://github.com/google/sanitizers/issues/639, currently ASan fails with internal CHECK in allocator on double free in recovery mode. Although there isn't reasonable expected behavior on double free (recent Glibc aborts in this case), we still want to proceed execution in hope to find more bugs (of course, without corrupting ASan internal invariants).

This patch tries to overcome the issue by adding return after ReportInvalidFree to avoid CHECK failure and pushing freed chunk into quarantine only if has CHUNK_ALLOCATED status.

[ygribov]

I'm not sure upstream people will take this but probably still makes sense to publish FTR.

[m.ostapenko]

I'm not sure upstream people will take this but probably still makes sense to publish FTR.

Yeah, perhaps this looks dirty. But still, failing with CHECK error looks a bit scary for users, perhaps we could just exit after ReportInvalidFree call if continuing execution doesn't look reasonable?

[ygribov]

But still, failing with CHECK error looks a bit scary for users

In normal mode the CHECK will never happen. More user-friendly error handling is needed only by recovery mode which is kind of a second class citizen.

[kcc]

I would prefer a force exit in ReportInvalidFree, just because this is simpler.
In either case, test please.

[ygribov]

Max, what about test?

[m.ostapenko]

Added testcase for the bug. I didn't updated the patch itself because we still want continue execution after double free was detected. Can this approach be acceptable for recovery mode?

[m.ostapenko]

Reminder.

[kcc]

LGTM with a nit

lib/asan/asan_allocator.cc

461 ↗

(On Diff #45986)

s/ones/chunks

compiler-rt/trunk/lib/asan/asan_allocator.cc

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);
     ASAN_MALLOC_HOOK(res, size);
     return res;
   }
 
-  void AtomicallySetQuarantineFlag(AsanChunk *m, void *ptr,
+  // 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;
     // Flip the chunk_state atomically to avoid race on double-free.
-    if (!atomic_compare_exchange_strong((atomic_uint8_t*)m, &old_chunk_state,
-                                        CHUNK_QUARANTINE, memory_order_acquire))
+    if (!atomic_compare_exchange_strong((atomic_uint8_t *)m, &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);
+    return true;
   }
 
   // Expects the chunk to already be marked as quarantined by using
-  // AtomicallySetQuarantineFlag.
+  // AtomicallySetQuarantineFlagIfAllocated.
   void QuarantineChunk(AsanChunk *m, void *ptr, BufferedStackTrace *stack,
                        AllocType alloc_type) {
     CHECK_EQ(m->chunk_state, CHUNK_QUARANTINE);
 
     if (m->alloc_type != alloc_type) {
       if (atomic_load(&alloc_dealloc_mismatch, memory_order_acquire)) {
         ReportAllocTypeMismatch((uptr)ptr, stack, (AllocType)m->alloc_type,
                                 (AllocType)alloc_type);
void Deallocate(void *ptr, uptr delete_size, BufferedStackTrace *stack,
     uptr chunk_beg = p - kChunkHeaderSize;
     AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);
     if (delete_size && flags()->new_delete_type_mismatch &&
         delete_size != m->UsedSize()) {
       ReportNewDeleteSizeMismatch(p, delete_size, stack);
     }
     ASAN_FREE_HOOK(ptr);
     // Must mark the chunk as quarantined before any changes to its metadata.
-    AtomicallySetQuarantineFlag(m, ptr, stack);
+    // Do not quarantine given chunk if we failed to set CHUNK_QUARANTINE flag.
+    if (!AtomicallySetQuarantineFlagIfAllocated(m, ptr, stack)) return;
     QuarantineChunk(m, ptr, stack, alloc_type);
   }
 
   void *Reallocate(void *old_ptr, uptr new_size, BufferedStackTrace *stack) {
     CHECK(old_ptr && new_size);
     uptr p = reinterpret_cast<uptr>(old_ptr);
     uptr chunk_beg = p - kChunkHeaderSize;
     AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);

compiler-rt/trunk/test/asan/TestCases/double-free.cc

 // RUN: %clangxx_asan -O0 %s -o %t 2>&1
 // RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=MALLOC-CTX
 
 // Also works if no malloc context is available.
 // RUN: %env_asan_opts=malloc_context_size=0:fast_unwind_on_malloc=0 not %run %t 2>&1 | FileCheck %s
 // RUN: %env_asan_opts=malloc_context_size=0:fast_unwind_on_malloc=1 not %run %t 2>&1 | FileCheck %s
+
+// RUN: %clangxx_asan -O0 -fsanitize-recover=address %s -o %t 2>&1
+// RUN: %env_asan_opts=halt_on_error=false %run %t 2>&1 | FileCheck %s --check-prefix CHECK-RECOVER
+
 // XFAIL: arm-linux-gnueabi
 // XFAIL: armv7l-unknown-linux-gnueabihf
 
 #include <stdlib.h>
 #include <string.h>
 int main(int argc, char **argv) {
   char *x = (char*)malloc(10 * sizeof(char));
   memset(x, 0, 10);
   int res = x[argc];
   free(x);
   free(x + argc - 1);  // BOOM
   // CHECK: AddressSanitizer: attempting double-free{{.*}}in thread T0
   // CHECK: #0 0x{{.*}} in {{.*}}free
   // CHECK: #1 0x{{.*}} in main {{.*}}double-free.cc:[[@LINE-3]]
   // CHECK: freed by thread T0 here:
   // MALLOC-CTX: #0 0x{{.*}} in {{.*}}free
   // MALLOC-CTX: #1 0x{{.*}} in main {{.*}}double-free.cc:[[@LINE-7]]
   // CHECK: allocated by thread T0 here:
   // MALLOC-CTX: double-free.cc:[[@LINE-12]]
+  // CHECK-RECOVER: AddressSanitizer: attempting double-free{{.*}}in thread T0
+  // CHECK-RECOVER-NOT: AddressSanitizer CHECK failed:
   return res;
 }