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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/asan/asan_allocator.cc:557
 
+    // On windows, some DLL may allocate memory before HeapAlloc is hooked.
+    if (SANITIZER_WINDOWS &&
----------------
Reworded a little better:
  // On Windows, uninstrumented DLLs may allocate memory before ASan hooks malloc. Don't report an invalid free in this case.


================
Comment at: lib/asan/asan_internal.h:67
 void InitializePlatformExceptionHandlers();
+bool PlatformIsValidAddress(uptr addr);
 
----------------
There are many ways an address can be "valid". Maybe `IsSystemHeapAddress` would be a better name?


================
Comment at: lib/asan/asan_win.cc:282
+  MEMORY_BASIC_INFORMATION mem_info = {};
+  if (::VirtualQuery((LPVOID)addr, &mem_info, sizeof(mem_info)) == 0 ||
+      mem_info.State == MEM_FREE) {
----------------
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


================
Comment at: test/asan/TestCases/Windows/virtual_memory.cc:1
+// RUN: %clang_cl_asan -O0 %s -Fe%t
+// RUN: not %run %t 2>&1 | FileCheck %s
----------------
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
...
```


https://reviews.llvm.org/D25946