[PATCH] D72011: [sanitizers][windows] Global/LocalAlloc interception and tests

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cpp:57
+
+_declspec(dllimport) HGLOBAL WINAPI GlobalAlloc(UINT uFlags, SIZE_T dwBytes);
+_declspec(dllimport) HGLOBAL WINAPI GlobalFree(HGLOBAL hMem);
----------------
I guess I prefer the spelling `__declspec`, since it's in the implementer's namespace.


================
Comment at: compiler-rt/lib/asan/asan_malloc_win.cpp:95
 ALLOCATION_FUNCTION_ATTRIBUTE
-size_t _msize_base(void *ptr) {
-  return _msize(ptr);
-}
+size_t _msize_base(void *ptr) { return _msize(ptr); }
 
----------------
Most of the changes in this file are from clang-format. I don't mind a few, but this is too many. Can you revert the non-functional whitespace changes far from the code you are editing? You can use `git checkout -p` to speed this up.

I also encourage you to run run `check-sanitizer` on Linux. The ASan codebase has its own linter script that doesn't run on Windows, and sometimes disagrees with clang-format.


================
Comment at: compiler-rt/lib/interception/interception_win.cpp:524
     case 0x588948:  // 48 89 58 XX : mov QWORD PTR[rax + XX], rbx
+    case 0x245489:  // 89 54 24 XX : mov DWORD PTR[rsp + XX], edx
       return 4;
----------------
So far this seems like the only functional change in this file.


================
Comment at: compiler-rt/test/asan/TestCases/Windows/globalalloc_flags_fallback.cpp:4
+// RUN: %env_asan_opts=windows_hook_rtl_allocators=true %run %t 2>&1 | FileCheck %s
+// UNSUPPORTED: asan-64-bits
+#include <assert.h>
----------------
What is the main blocker for making these tests work for 64-bit? Fixing the hotpatching interceptors?

Chromium stopped using 32-bit ASan because the address space limitations were prohibitive. I think the sanitizer-windows bot only builds x64 these days, so if we can port these, that would help make sure they are covered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72011