[asan] Check if the memory is readable before using the AsanChunk in free() and realloc()

[filcab]

Summary

This allows us to better diagnose free() and realloc() with a bad
pointer, instead of SIGSEGV.

[samsonov]

I'm somewhat worried about the performance implications of this. IsAccessibleMemoryRange is a syscall after all, not sure it would be nice to call it on every malloc/free. We do run non-trivial operations on these calls though, like collecting stack traces.

[kcc]

Thanks for doing this!
One comment for the test.

test/asan/TestCases/Posix/bad-free-no-segv.cc

15

I'd prefer the test to use a separate mprotec-ted chunk, not the shadow gap.

[kcc]

lib/asan/asan_allocator.cc

527

Oh, yes, I've totally missed this. For some reason I thought this is on the error-reporting path.
Thanks Alexey.
Yes, on the main path we can't do this.

[filcab]

That is totally true. I didn't think of the pipe()+write().
Thank you.

lib/asan/asan_allocator.cc

 
 #include "asan_allocator.h"
 #include "asan_mapping.h"
 #include "asan_poisoning.h"
 #include "asan_report.h"
 #include "asan_stack.h"
 #include "asan_thread.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
+#include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_internal_defs.h"
 #include "sanitizer_common/sanitizer_list.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
 #include "sanitizer_common/sanitizer_quarantine.h"
 #include "lsan/lsan_common.h"
 
 namespace __asan {
#endif
 
   void Deallocate(void *ptr, uptr delete_size, BufferedStackTrace *stack,
                   AllocType alloc_type) {
     uptr p = reinterpret_cast<uptr>(ptr);
     if (p == 0) return;
 
     uptr chunk_beg = p - kChunkHeaderSize;
     AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);
+    if (!IsAccessibleMemoryRange((uptr)m, sizeof(AsanChunk)))
+      ReportFreeNotMalloced((uptr)ptr, stack);
     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.
     // 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);
+    if (!IsAccessibleMemoryRange((uptr)m, sizeof(AsanChunk)))
+      ReportFreeNotMalloced((uptr)old_ptr, stack);
 
     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;

test/asan/TestCases/Posix/bad-free-no-segv.cc

+// RUN: %clangxx_asan -O0 %s -o %t
+// RUN: not %run %t f 2>&1 | FileCheck %s
+// RUN: not %run %t r 2>&1 | FileCheck %s
+#include <assert.h>
+#include <stdlib.h>
+#include <sanitizer/asan_interface.h>
+
+// CHECK-NOT: DEADLYSIGNAL
+// CHECK: AddressSanitizer: attempting free on address which was not malloc()-ed
+int main(int argc, char **argv) {
+  // The shadow gap is mprotect()ed, so we get an address inside it and pass it
+  // along to free. reading from it should be a problem and ASan should notice
+  // it before trying to read.
+  size_t shadow_scale, shadow_offset;
+  __asan_get_shadow_mapping(&shadow_scale, &shadow_offset);
+  // We add 0x100 because asan_free subtracts sizeof(AsanChunk) to our ptr.
+  // Which means we need to make sure that the free()ed/realloc()ed pointer is
+  // also in non-addressable memory.
+  void *p = (void *)(shadow_offset + 0x1000 + (shadow_offset >> shadow_scale));
+  assert(argc == 2);
+  switch (argv[1][0]) {
+  case 'f':
+    free(p);
+    break;
+  case 'r':
+    realloc(p, 42);
+    break;
+  default:
+    assert(false);
+  }
+}