[PATCH] D62927: [sanitizers][windows] Rtl-Heap Interception and tests

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

I reviewed the code and mostly came up with surface formatting issues. Functionally I think it looks good and the only way to really find out if it works is to ship it. :) So, I think we should fix the surface issues and try to move forward with that.



================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:275
+      return reallocFunc(hHeap, dwFlags, lpMem, dwBytes);
+    }
+    bool only_asan_supported_flags =
----------------
Please capitalize the type name of the enum, so AllocationOwnership.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:286
+        if (dwFlags & HEAP_ZERO_MEMORY)
+          replacement_alloc = asan_calloc(1, dwBytes, &stack);
+        else
----------------
This is used in one place, I would sink it down to the use.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:290
+        if (replacement_alloc) {
+          size_t old_size = heapSizeFunc(hHeap, dwFlags, lpMem);
+          if (old_size == ((SIZE_T)0) - 1) {
----------------
This is only used if RTL allocator hooking is enabled, I'd sink it into the if.


================
Comment at: compiler-rt/lib/asan/asan_win.cc:233
 void AsanCheckDynamicRTPrereqs() {}

 void AsanCheckIncompatibleRT() {}
 void ReadContextStack(void *context, uptr *stack, uptr *ssize) {
----------------
I think it would be simpler to drop this check and always do the `IsTlsInitialized()` check. I understand that adding the new heap interceptors is what causes us to run code prior to TLS initialization, but in the future ASan could change again in some other way that causes us to clal ASanTSDGet early and removing an `if` and defending against that possibility seems good.

Also, this isn't indented correctly, please fix that before committing.


================
Comment at: compiler-rt/lib/asan/asan_win.cc:299
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 int __asan_set_seh_filter() {
   // We should only store the previous handler if it's not our own handler in
----------------
There should be a space between the `){`. I'd recommend using `git-clang-format` or some other tool to run clang-format to fix these minor issues.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62927/new/

https://reviews.llvm.org/D62927