[PATCH] Call __asan_free_hook() before marking the chunk quarantinned

[Alexey Samsonov]

  More comments by timurrrr.

Hi samsonov, timurrrr, kcc,

http://llvm-reviews.chandlerc.com/D913

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D913?vs=2256&id=2257#toc

BRANCH
  svn

ARCANIST PROJECT
  compiler-rt

Files:
  lib/asan/tests/asan_test.cc
  lib/asan/asan_allocator2.cc
  lib/asan/lit_tests/malloc_hook.cc
  lib/asan/lit_tests/free_hook_realloc.cc

Index: lib/asan/tests/asan_test.cc
===================================================================
--- lib/asan/tests/asan_test.cc
+++ lib/asan/tests/asan_test.cc
@@ -227,16 +227,26 @@
   delete Ident(x);
 }
 
+static size_t kOOMAllocationSize =
+  SANITIZER_WORDSIZE == 64 ? (size_t)(1ULL << 48) : (0xf0000000);
+
 TEST(AddressSanitizer, OutOfMemoryTest) {
-  size_t size = SANITIZER_WORDSIZE == 64 ? (size_t)(1ULL << 48) : (0xf0000000);
-  EXPECT_EQ(0, realloc(0, size));
+  EXPECT_EQ(0, realloc(0, kOOMAllocationSize));
   EXPECT_EQ(0, realloc(0, ~Ident(0)));
-  EXPECT_EQ(0, malloc(size));
+  EXPECT_EQ(0, malloc(kOOMAllocationSize));
   EXPECT_EQ(0, malloc(~Ident(0)));
-  EXPECT_EQ(0, calloc(1, size));
+  EXPECT_EQ(0, calloc(1, kOOMAllocationSize));
   EXPECT_EQ(0, calloc(1, ~Ident(0)));
 }
 
+TEST(AddressSanitizer, BadReallocTest) {
+  int *a = (int*)Ident(malloc(100));
+  a[0] = 42;
+  EXPECT_EQ(0, realloc(a, kOOMAllocationSize));
+  EXPECT_EQ(42, a[0]);
+  free(a);
+}
+
 #if ASAN_NEEDS_SEGV
 namespace {
 
Index: lib/asan/asan_allocator2.cc
===================================================================
--- lib/asan/asan_allocator2.cc
+++ lib/asan/asan_allocator2.cc
@@ -452,12 +452,6 @@
                             StackTrace *stack, AllocType alloc_type) {
   CHECK_EQ(m->chunk_state, CHUNK_QUARANTINE);
 
-  // FIXME: if the free hook produces an ASan report (e.g. due to a bug),
-  // printing the report may crash as the AsanChunk free-related fields have not
-  // been updated yet. We might need to introduce yet another chunk state to
-  // handle this correctly, but don't want to yet.
-  ASAN_FREE_HOOK(ptr);
-
   if (m->alloc_type != alloc_type && flags()->alloc_dealloc_mismatch)
     ReportAllocTypeMismatch((uptr)ptr, stack,
                             (AllocType)m->alloc_type, (AllocType)alloc_type);
@@ -502,6 +496,7 @@
 
   uptr chunk_beg = p - kChunkHeaderSize;
   AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);
+  ASAN_FREE_HOOK(ptr);
   // Must mark the chunk as quarantined before any changes to its metadata.
   AtomicallySetQuarantineFlag(m, ptr, stack);
   QuarantineChunk(m, ptr, stack, alloc_type);
@@ -517,17 +512,14 @@
   thread_stats.reallocs++;
   thread_stats.realloced += new_size;
 
-  // Must mark the chunk as quarantined before any changes to its metadata.
-  // This also ensures that other threads can't deallocate it in the meantime.
-  AtomicallySetQuarantineFlag(m, old_ptr, stack);
-
-  uptr old_size = m->UsedSize();
-  uptr memcpy_size = Min(new_size, old_size);
   void *new_ptr = Allocate(new_size, 8, stack, FROM_MALLOC, true);
   if (new_ptr) {
     CHECK_NE(REAL(memcpy), (void*)0);
+    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);
-    QuarantineChunk(m, old_ptr, stack, FROM_MALLOC);
+    Deallocate(old_ptr, stack, FROM_MALLOC);
   }
   return new_ptr;
 }
Index: lib/asan/lit_tests/malloc_hook.cc
===================================================================
--- lib/asan/lit_tests/malloc_hook.cc
+++ lib/asan/lit_tests/malloc_hook.cc
@@ -4,19 +4,31 @@
 #include <unistd.h>
 
 extern "C" {
+bool __asan_get_ownership(const void *p);
+
+void *global_ptr;
+
 // Note: avoid calling functions that allocate memory in malloc/free
 // to avoid infinite recursion.
 void __asan_malloc_hook(void *ptr, size_t sz) {
-  write(1, "MallocHook\n", sizeof("MallocHook\n"));
+  if (__asan_get_ownership(ptr)) {
+    write(1, "MallocHook\n", sizeof("MallocHook\n"));
+    global_ptr = ptr;
+  }
 }
 void __asan_free_hook(void *ptr) {
-  write(1, "FreeHook\n", sizeof("FreeHook\n"));
+  if (__asan_get_ownership(ptr) && ptr == global_ptr)
+    write(1, "FreeHook\n", sizeof("FreeHook\n"));
 }
 }  // extern "C"
 
 int main() {
   volatile int *x = new int;
   // CHECK: MallocHook
+  // Check that malloc hook was called with correct argument.
+  if (global_ptr != (void*)x) {
+    _exit(1);
+  }
   *x = 0;
   delete x;
   // CHECK: FreeHook
Index: lib/asan/lit_tests/free_hook_realloc.cc
===================================================================
--- /dev/null
+++ lib/asan/lit_tests/free_hook_realloc.cc
@@ -0,0 +1,18 @@
+// Check that free hook doesn't conflict with Realloc.
+// RUN: %clangxx_asan -O2 %s -o %t && %t
+#include <assert.h>
+#include <stdlib.h>
+
+extern "C" {
+void __asan_free_hook(void *ptr) {
+  *(int*)ptr = 0;
+}
+}
+
+int main() {
+  int *x = (int*)malloc(100);
+  x[0] = 42;
+  int *y = (int*)realloc(x, 200);
+  assert(y[0] == 42);
+  return 0;
+}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D913.3.patch
Type: text/x-patch
Size: 4658 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130604/3ea499fa/attachment.bin>