[PATCH] D125195: [asan][ARMCXXABI] Added missing asan poison array cookie hooks.

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:263
+#if SANITIZER_ARM64
+  // The ARM64 cookie has a second "size_t" entry so poison it as well
+  *(reinterpret_cast<u8*>(s)-1) = kAsanArrayCookieMagic;
----------------



================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
 int main(int argc, char **argv) {
-  C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  C *buffer = new C[1];
+  reinterpret_cast<char*>(buffer)[-1] = 42;
----------------
`C *buffer = new C[argc];`

Although this looks weird, I think this is to prevent the compiler from reasoning about the array size.
I know that we had tests in the past that were "silently passing", because the compiler optimized away parts of the test.

I am not sure if it applies here, but let's keep it just in case ...


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:20
+  C *buffer = new C[1];
+  reinterpret_cast<char*>(buffer)[-1] = 42;
 // CHECK: AddressSanitizer: heap-buffer-overflow
----------------



================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:4-15
+// Here we test that:
+//   1) w/o ASan, the array cookie location IS writable
+//   2) w/ASan, the array cookie location IS writeable by default
+//   3) w/ASan, the flag "-fno-sanitize-address-poison-custom-array-cookie"
+//      is a NOP (because this is the default) and finally,
+//   4) w/ASan AND "-fsanitize-address-poison-custom-array-cookie", the
+//      array cookie location is NOT writable (ASAN successfully stopped it)
----------------
Thanks for improving the test like this!  This is now better than "just a test", it's essentially "executable documentation"! :)


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:36-38
-  assert(reinterpret_cast<uintptr_t>(foo) ==
-         reinterpret_cast<uintptr_t>(Foo::allocated) + sizeof(void*));
-  *reinterpret_cast<uintptr_t*>(Foo::allocated) = 42;
----------------
Is the reason that we had to remove this assert (and change how we overwrite the cookie) that on arm the cookie is 2 words?

Can we do the following?
```
int cooke_words = <preprocessor for "is ARM?"> : 2 : 1;
assert(reinterpret_cast<uintptr_t>(foo) ==
         reinterpret_cast<uintptr_t>(Foo::allocated) + cookie_words * sizeof(void*));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195