[Asan] Fix false leak report

[vitalybuka]

Summary

If user thread is in the allocator, the allocator
may have no pointer into future user's part of
the allocated block. AddrIsInside ignores such
pointers and lsan reports a false memory leak.

[vitalybuka]

re-word comment

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1120–1125

[vitalybuka]

comments

[vitalybuka]

thanks

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Removing this check seems to introduce some LSan false negatives. Ran into it while playing with libFuzzer:

$ bin/clang++ -fsanitize=address,fuzzer ../compiler-rt/test/fuzzer/LeakTest.cpp -m32 -g
$ rm -rf corpus && mkdir corpus
$ ./a.out -runs=100000 -detect_leaks=1 -entropic=1 -seed=1 corpus/

libFuzzer finds an input H that triggers a leak, but LSan doesn't detect it. Only happens after this patch, and only with -m32.

Still trying to figure out why this check is needed...

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Presumably we have some internal pointer to the ASanChunk header that is causing the allocation to be marked reachable.

[vitalybuka]

thanks

compiler-rt/lib/asan/asan_allocator.cpp

1121

I don't know what to do about this.
E.g. vector<> usually keeps end() pointer which can be the first byte of the next chunk. Before the patch is was not an issue.
But now it's going to hold next chunk as reachable.

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Shouldn't the acquire-load above be enough to guarantee that we've stored a pointer to the user allocation either on stack or in a register, since we store-release in Allocate()?

[vitalybuka]

compiler-rt/lib/asan/asan_allocator.cpp

1121

I think not, memory order only requires that we return same as user_beg calculated before atomic operation.

[morehouse]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Can we force the guarantee, for example by storing to a volatile stack variable before store-releasing?

If we can, then the LSan false positive this patch fixed shouldn't come back if we add the AddrIsInside check back.

[vitalybuka]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Actually I tried that after landing the patch, and I still see the same bug on that binary. Either my way to pin pointer is too week, or I misdiagnosed the bug. I am trying to add more logs to confirm either way.

However even if pinning pointer is possible we have case when we have small allocations without magic. There is a probability that the primary allocator returns CHUNK_ALLOCATED for uninitialized chunk. It's theoretical but I was able to catch that with CHECK(). I tried to solve that with that patch which always adds magic, the one which extends header from 16 to 24 D87359.
I have idea how to use size bits for additional validation magic, as we need magic only when we have small size and we don't use most of bits. But it's needed only we can have guaranteed user pointer.

[vitalybuka]

compiler-rt/lib/asan/asan_allocator.cpp

1121

Compiler puts pointer into FP registers.
I will undo this patch and fix how we read registers.

compiler-rt/lib/asan/asan_allocator.cpp

void UnlockAllocator() {
   __asan::get_allocator().ForceUnlock();
 }
 
 void GetAllocatorGlobalRange(uptr *begin, uptr *end) {
   *begin = (uptr)&__asan::get_allocator();
   *end = *begin + sizeof(__asan::get_allocator());
 }
 
-uptr PointsIntoChunk(void* p) {
+uptr PointsIntoChunk(void *p) {
   uptr addr = reinterpret_cast<uptr>(p);
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddrFastLocked(addr);
   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;
+  // AsanChunk presence means that we point into some block from underlying
+  // allocators. Don't check whether p points into user memory, since until
+  // the return from AsanAllocator::Allocator we may have no such
+  // pointer anywhere. But we must already have a pointer to GetBlockBegin().
+  return m->Beg();
 }
 
 uptr GetUserBegin(uptr chunk) {
   __asan::AsanChunk *m = __asan::instance.GetAsanChunkByAddrFastLocked(chunk);
   return m ? m->Beg() : 0;
 }
 
 LsanMetadata::LsanMetadata(uptr chunk) {
   metadata_ = chunk ? reinterpret_cast<void *>(chunk - __asan::kChunkHeaderSize)

compiler-rt/test/asan/TestCases/redzone_noleak.cpp

+// Test whether pointers into left redzone count memory are reachable.
+// If user thread is inside asan allocator code then we may have no
+// pointers into user part of memory yet. However we should have a pointer
+// into the allocated memory chunk.
+//
+// RUN: %clangxx_asan  %s -o %t
+// RUN: %run %t 2>&1
+
+#include <cstdlib>
+#include <stdio.h>
+#include <thread>
+
+void *pointers[1000];
+void **cur = pointers;
+
+void leak(int n, int offset) {
+  printf("%d %d\n", n, offset);
+  for (int i = 0; i < 3; ++i)
+    *(cur++) = (new int[n]) + offset;
+}
+
+int main(int argc, char **argv) {
+  for (int n = 1; n < 10000000; n = n * 2) {
+    leak(n, 0);
+    leak(n, -1);
+  }
+  return 0;
+}