[compiler-rt][asan] Add support for desallocation of unhandled pointers

[etienneb]

Summary

On windows 10, the ucrt DLL is performing allocations before the function hooking and there are multiple allocations not handled by Asan. When a free occur at the end of the process, asan is reporting desallocations not malloc-ed.

[rnk]

lib/asan/asan_allocator.cc

560

IMO it's a bug if we get here and !PointerIsMine. The primary allocator check is pretty fast, but checking the secondary allocator is a linear search I think.

We can probably move this check to the HeapFree interceptor in asan_malloc_win.cc if we need it, though. That's probably only called by the CRT, which isn't usually hot. It also keeps Windows heap interception concerns out of the main allocator logic.

[etienneb]

lib/asan/asan_allocator.cc

560

Do we consider full coverage (catching every allocations) or not.

If not, then this make sense and we may need to figure out efficient way to run this test.

If we assume full coverage, then we should at least add a DCHECK here.
This is really error prone.

On windows, it's pretty hard to guarantee full coverage. Dynamic loading of DLL and the operating system may play in your back often.
I''m not convince which way is the best.

At least, we should make some benchmark on this.

[kcc]

Yep. Whatever the actual problem is this patch is unlikely to be the right fix.

[etienneb]

bug: https://reviews.llvm.org/D25946

[etienneb]

bad link! my bad.

BUG: https://llvm.org/bugs/show_bug.cgi?id=30903

[etienneb]

better validation, windows only

[etienneb]

rnk@, is that what we talked about?

[rnk]

lib/asan/asan_allocator.cc

557

Reworded a little better:

// On Windows, uninstrumented DLLs may allocate memory before ASan hooks malloc. Don't report an invalid free in this case.

lib/asan/asan_internal.h

67

There are many ways an address can be "valid". Maybe IsSystemHeapAddress would be a better name?

lib/asan/asan_win.cc

282

This is too broad. This will suppress invalid free checks on any pointer from VirtualAlloc. Why not use HeapValidate? It appears to support checking a single block efficiently: https://msdn.microsoft.com/en-us/library/windows/desktop/aa366708(v=vs.85).aspx

test/asan/TestCases/Windows/virtual_memory.cc

1 ↗

(On Diff #88007)

Use a test that looks more like this, it more closely matches the actual dbghelp.dll scenario where a DLL initializes before ASan and uses ucrtbase.dll.

// foo.cpp:
#include <memory>
std::unique_ptr<int> __declspec(dllexport) myglobal(new int(42));

// t.cpp:
#include <memory>
extern std::unique_ptr<int> __declspec(dllimport) myglobal;
int main() { printf("*myglobal: %d\n", *myglobal); }

$ clang-cl -EHsc foo.cpp -MD -LD -nologo
foo.cpp
   Creating library foo.lib and object foo.exp

$ clang-cl -fsanitize=address t.cpp foo.lib -MT
   Creating library t.lib and object t.exp

$ ./t.exe
...

[etienneb]

lib/asan/asan_win.cc

282

Are we assuming?

1. no custom memory management (direct access to virtual-alloc/free)?
2. no other heap (only the default process heap

And I'm not sure |lpMem| can be any arbitrary pointer within the memory chunk. I suspect it must be the begining of the block.

[etienneb]

For this given snippet:

#include <windows.h>
#include <iostream>

int main(int argc, char* argv[]) {
  char *p1 = (char*)HeapAlloc(GetProcessHeap(), 0, 12);
  char *p2 = p1 + 7;

  BOOL b1 = HeapValidate(GetProcessHeap(), 0, NULL);
  BOOL b2 = HeapValidate(GetProcessHeap(), 0, p1);
  BOOL b3 = HeapValidate(GetProcessHeap(), 0, p2);
  std::cout << "result: " << b1 << " " << b2 << " " << b3 << std::endl;
}

The result is:

result: 1 1 0

HeapValidate is triggering a breakpoint with an invalid address:

_RtlpBreakPointHeap@4:
77CA06F9  mov         edi,edi  
77CA06FB  push        ebp  
77CA06FC  mov         ebp,esp  
77CA06FE  mov         eax,dword ptr fs:[00000018h]  
77CA0704  mov         eax,dword ptr [eax+30h]  
77CA0707  cmp         byte ptr [eax+2],0  
77CA070B  je          _RtlpBreakPointHeap@4+2Bh (77CA0724h)  
77CA070D  mov         eax,dword ptr [ebp+8]  
77CA0710  mov         byte ptr ds:[77CD92A5h],1  
77CA0717  mov         dword ptr ds:[77CD92A0h],eax  
77CA071C  int         3                                  <<---- debugger is breaking here
77CA071D  mov         byte ptr ds:[77CD92A5h],0  
77CA0724  pop         ebp  
77CA0725  ret         4

[rnk]

lib/asan/asan_win.cc

282

We only come here when someone calls free in the CRT. That means they must be using the system heap. ASan assumes it's the only code hotpatching the system heap. We only want to suppress invalid free reports on pointers to blocks of memory allocated on the system heap from before ASan initialized. Any other pointer should generate an invalid free report.

[rnk]

In D25946#676423, @etienneb wrote:

For this given snippet:
... snip

Looks right to me.

HeapValidate is triggering a breakpoint with an invalid address:
.. snip

That's annoying, but it only happens when you attach a debugger, and I still think this is the right thing to do. The universal CRT actually uses HeapValidate internally in some builds to select between different CRT heaps. Take a look at select_heap in WindowsSDK\10\Source\10.0.14393\ucrt\heap\free_base.cpp.

[etienneb]

lib/asan/asan_win.cc

282

You're right, this is for Free(...).
I'm gonna add a comment to |IsSystemHeapAddress |.

[etienneb]

rnk comments

[etienneb]

PTAL.

Without the fix, the unittest is failing with:

% "D:\src\llvm\ninja\projects\compiler-rt\test\asan\I386WindowsConfig\TestCases\Windows\Output\dll_heap_allocation.cc.tmp"
myglobal: 42
=================================================================
==17584==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x00add6f8 in thread T0
    #0 0x39e7e4 in __asan_wrap_HeapFree d:\src\llvm\llvm\projects\compiler-rt\lib\asan\asan_malloc_win.cc:181
    #1 0xf5e99fe in _stdio_common_vswprintf+0x12e (C:\Windows\system32\ucrtbase.DLL+0x100299fe)
    #2 0x77c094c4 in RtlIsCurrentThreadAttachExempt+0x5e (C:\Windows\SysWOW64\ntdll.dll+0x7dea94c4)
    #3 0x77c28ebd in LdrShutdownProcess+0x96 (C:\Windows\SysWOW64\ntdll.dll+0x7dec8ebd)
    #4 0x77c28e09 in RtlExitUserProcess+0x73 (C:\Windows\SysWOW64\ntdll.dll+0x7dec8e09)
    #5 0x775879c4 in ExitProcess+0x14 (C:\Windows\syswow64\kernel32.dll+0x7dd779c4)
    #6 0x3d28e8 in exit_or_terminate_process d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:129
    #7 0x3d287e in common_exit d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:269
    #8 0x3d2a63 in exit d:\th\minkernel\crts\ucrt\src\appcrt\startup\exit.cpp:282
    #9 0x3b1159 in __scrt_common_main_seh f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:260
    #10 0x77583369 in BaseThreadInitThunk+0x11 (C:\Windows\syswow64\kernel32.dll+0x7dd73369)
    #11 0x77c09901 in RtlInitializeExceptionChain+0x62 (C:\Windows\SysWOW64\ntdll.dll+0x7dea9901)
    #12 0x77c098d4 in RtlInitializeExceptionChain+0x35 (C:\Windows\SysWOW64\ntdll.dll+0x7dea98d4)

[rnk]

lgtm

@kcc, does this look like what we discussed?

[kcc]

LGTM with a comment nit.

lib/asan/asan_internal.h

67

Returns whether 'addr' is a valid allocated system heap block.
'addr' must point to the beginning of the block.

[etienneb]

fix kcc comments

lib/asan/asan_allocator.cc

#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);
 
+    // On Windows, uninstrumented DLLs may allocate memory before ASan hooks
+    // malloc. Don't report an invalid free in this case.
+    if (SANITIZER_WINDOWS &&
+        !get_allocator().PointerIsMine(ptr)) {
+      if (!IsSystemHeapAddress(p))
+        ReportFreeNotMalloced(p, stack);
+      return;
+    }
+
     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;
 
     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);

lib/asan/asan_internal.h

 
 class AsanThread;
 using __sanitizer::StackTrace;
 
 void AsanInitFromRtl();
 
 // asan_win.cc
 void InitializePlatformExceptionHandlers();
+// Returns whether an address is a valid allocated system heap block. It must
+// points to the beginning of the block.
+bool IsSystemHeapAddress(uptr addr);
 
 // asan_rtl.cc
 void NORETURN ShowStatsAndAbort();
 
 // asan_malloc_linux.cc / asan_malloc_mac.cc
 void ReplaceSystemMalloc();
 
 // asan_linux.cc / asan_mac.cc / asan_win.cc

lib/asan/asan_linux.cc

 SANITIZER_INTERFACE_ATTRIBUTE
 asan_rt_version_t  __asan_rt_version;
 }
 
 namespace __asan {
 
 void InitializePlatformInterceptors() {}
 void InitializePlatformExceptionHandlers() {}
+bool IsSystemHeapAddress (uptr addr) { return false; }
 
 void *AsanDoesNotSupportStaticLinkage() {
   // This will fail to link with -static.
   return &_DYNAMIC;  // defined in link.h
 }
 
 void AsanApplyToGlobals(globals_op_fptr op, const void *needle) {
   UNIMPLEMENTED();

lib/asan/asan_mac.cc

extern "C" {
   extern char ***_NSGetArgv(void);
   extern char ***_NSGetEnviron(void);
 }
 
 namespace __asan {
 
 void InitializePlatformInterceptors() {}
 void InitializePlatformExceptionHandlers() {}
+bool IsSystemHeapAddress (uptr addr) { return false; }
 
 // No-op. Mac does not support static linkage anyway.
 void *AsanDoesNotSupportStaticLinkage() {
   return 0;
 }
 
 // No-op. Mac does not support static linkage anyway.
 void AsanCheckDynamicRTPrereqs() {}

lib/asan/asan_win.cc

 void InitializePlatformExceptionHandlers() {
 #if SANITIZER_WINDOWS64
   // On Win64, we map memory on demand with access violation handler.
   // Install our exception handler.
   CHECK(AddVectoredExceptionHandler(TRUE, &ShadowExceptionHandler));
 #endif
 }
 
+bool IsSystemHeapAddress(uptr addr) {
+  return ::HeapValidate(GetProcessHeap(), 0, (void*)addr) != FALSE;
+}
+
 // We want to install our own exception handler (EH) to print helpful reports
 // on access violations and whatnot.  Unfortunately, the CRT initializers assume
 // they are run before any user code and drop any previously-installed EHs on
 // the floor, so we can't install our handler inside __asan_init.
 // (See crt0dat.c in the CRT sources for the details)
 //
 // Things get even more complicated with the dynamic runtime, as it finishes its
 // initialization before the .exe module CRT begins to initialize.

test/asan/TestCases/Windows/dll_heap_allocation.cc

+
+// RUN: %clang_cl -LD %s -Fe%t.dll -DHEAP_LIBRARY -MD
+// RUN: %clang_cl %s %t.lib -Fe%t -fsanitize=address -MT
+// RUN: %run %t 2>&1 | FileCheck %s
+
+// Check that ASan does not fail when releasing allocations that occurred within
+// an uninstrumented DLL.
+
+#ifdef HEAP_LIBRARY
+#include <memory>
+#include <windows.h>
+
+std::unique_ptr<int> __declspec(dllexport) myglobal(new int(42));
+BOOL WINAPI DllMain(PVOID h, DWORD reason, PVOID reserved) {
+  return TRUE;
+}
+
+#else
+
+#include <memory>
+extern std::unique_ptr<int> __declspec(dllimport) myglobal;
+int main(int argc, char **argv) {
+  printf("myglobal: %d\n", *myglobal);
+  return 0;
+}
+
+#endif
+
+// CHECK: myglobal: 42
+// CHECK-NOT: ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed

test/asan/lit.cfg

 config.substitutions.append( ("%clangxx_asan ", build_invocation(clang_asan_cxxflags)) )
 config.substitutions.append( ("%shared_libasan", "libclang_rt.asan-%s.so" % target_arch))
 if config.asan_dynamic:
   config.substitutions.append( ("%clang_asan_static ", build_invocation(clang_asan_static_cflags)) )
   config.substitutions.append( ("%clangxx_asan_static ", build_invocation(clang_asan_static_cxxflags)) )
 
 # Windows-specific tests might also use the clang-cl.exe driver.
 if platform.system() == 'Windows':
-  clang_cl_asan_cxxflags = ["-fsanitize=address",
-                            "-Wno-deprecated-declarations",
-                            "-WX",
-                            "-D_HAS_EXCEPTIONS=0",
-                            "-Zi"] + target_cflags
+  clang_cl_cxxflags = ["-Wno-deprecated-declarations",
+  clang_cl_asan_cxxflags = ["-fsanitize=address"] + clang_cl_cxxflags
   if config.asan_dynamic:
     clang_cl_asan_cxxflags.append("-MD")
-  clang_invocation = build_invocation(clang_cl_asan_cxxflags)
-  clang_cl_invocation = clang_invocation.replace("clang.exe","clang-cl.exe")
-  config.substitutions.append( ("%clang_cl_asan ", clang_cl_invocation) )
+
+  clang_cl_invocation = build_invocation(clang_cl_cxxflags)
+  clang_cl_invocation = clang_cl_invocation.replace("clang.exe","clang-cl.exe")
+  config.substitutions.append( ("%clang_cl ", clang_cl_invocation) )
+
+  clang_cl_asan_invocation = build_invocation(clang_cl_asan_cxxflags)
+  clang_cl_asan_invocation = clang_cl_asan_invocation.replace("clang.exe","clang-cl.exe")
+  config.substitutions.append( ("%clang_cl_asan ", clang_cl_asan_invocation) )
+
   base_lib = os.path.join(config.compiler_rt_libdir, "clang_rt.asan%%s-%s.lib" % config.target_arch)
   config.substitutions.append( ("%asan_lib", base_lib % "") )
   config.substitutions.append( ("%asan_cxx_lib", base_lib % "_cxx") )
   config.substitutions.append( ("%asan_dll_thunk", base_lib % "_dll_thunk") )
 
 if platform.system() == 'Windows':
   # Don't use -std=c++11 on Windows, as the driver will detect the appropriate
   # default needed to use with the STL.