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

[Matthew G McGovern via Phabricator via llvm-commits]

mcgov marked 4 inline comments as done.
mcgov added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:53

----------------
vitalybuka wrote:
> can you please upload the patch with "arc" tool, otherwise context is not visible
will do


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:209
+  if (flags()->windows_hook_rtl_allocators)
+  {
+    if (UNLIKELY(!asan_inited || (!__sanitizer_get_ownership(lpMem) &&
----------------
vitalybuka wrote:
> Not sure I understand this "if".
> Can we get here when windows_hook_rtl_allocators==false?
> If true, I don't see much difference after asan_inited set to true.
> 
These if's are handling some edge cases when you hook the RTL allocators. There are some allocations which will occur before interception happens, so you have to check where the allocation came from.

I agree these look pretty gross and aren't super clear. I'll see if I can clean it up or write some comments to make them obvious.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:211
+    if (UNLIKELY(!asan_inited || (!__sanitizer_get_ownership(lpMem) &&
+                                  HeapValidate(hHeap, 0, lpMem)))) {
+      return REAL(HeapSize)(hHeap, dwFlags, lpMem);
----------------
vitalybuka wrote:
> can you please clang-format the patch?
I have my editor set to format on save/paste but something must have gone wrong. Will make sure the new sections are formatted properly


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:218
+    CHECK(dwFlags == 0 && "unsupported heap flags");
+  }
+  GET_CURRENT_PC_BP_SP;
----------------
vitalybuka wrote:
> I'd expect this check is needed unconditionally, we can get to asan_malloc_usable_size even "if-then" about was executed.
Oh good point, I'll check assertions again file-wide.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cc:462
+  void *p;
+  // Reading MSDN suggests that the *entire* usable allocation is zeroed out.
+  // Otherwise it is difficult to HeapReAlloc with HEAP_ZERO_MEMORY.
----------------
vitalybuka wrote:
> Isn't better to use REAL in cases above instead of CHECK(Flags...) ?
I need more context on what you mean here, where would it be better?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D62927