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

[Julian Lettner via Phabricator via cfe-commits]

yln added inline comments.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2453
+    CGF.Builder.CreateCall(F, cookie.getRawPointer(CGF));
+  }
 
----------------
This code is very similar to what we have in `ItaniumCXXABI::InitializeArrayCookie()`.  Can we try to extract a local `handleASanArrayCookiePoisoning()` helper?


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2485
+      CGM.CreateRuntimeFunction(FTy, "__asan_load_cxx_array_cookie");
+  return CGF.Builder.CreateCall(F, numElementsPtr.getRawPointer(CGF));
 }
----------------
Same here: extract common helper for the ASan stuff to be shared by ARM and ItaniumCXXABI.


================
Comment at: compiler-rt/lib/asan/asan_poisoning.cpp:262-265
+  // The ARM64 cookie has a second "elementSize" entry so poison it as well
+  #if SANITIZER_ARM64
+    *(reinterpret_cast<u8*>(s)-1) = kAsanArrayCookieMagic;
+  #endif
----------------
Nitpicking extreme:
* I think the code inside `#if` shouldn't have extra indent (because preprocessor sections don't establish code).  If in doubt, let `clang-format` do it for you.
* Let's move the comment inside the #if, just above before the line of code.  If you ever read the pre-processed source-code, then the comment "lives and dies" with the line of code it relates too, i.e, on x86, currently there would be a comment without the code.


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:3
 // RUN: %clangxx_asan -O3 %s -o %t
-// RUN:                                    not %run %t 2>&1  | FileCheck %s
+// RUN:                                      not %run %t 2>&1  | FileCheck %s
 // RUN: %env_asan_opts=poison_array_cookie=1 not %run %t 2>&1  | FileCheck %s
----------------
❤  The boy scouts rule:
> Always leave the campground cleaner than you found it.


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_test.cpp:19
   C *buffer = new C[argc];
-  buffer[-2].x = 10;
+  buffer[-1].x = 10;
 // CHECK: AddressSanitizer: heap-buffer-overflow
----------------
Can we try to simulate a 1-byte buffer-underflow (and leave the definition of `struct C` as-is)?  This would show that ASan still reports the smallest possible infraction.


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_uaf_test.cpp:20
 __attribute__((no_sanitize_address)) void Write42ToCookie(C *c) {
-  long *p = reinterpret_cast<long*>(c);
+  size_t *p = reinterpret_cast<size_t*>(c);
   p[-1] = 42;
----------------
Can we try to simulate a 1-byte buffer-underflow (and leave the definition of struct C as-is)? This would show that ASan still reports the smallest possible infraction.


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:5
+// Here we test that:
+//   1) w/o sanitizer, the array cookie location IS writable
+//   2) w/sanitizer, the array cookie location IS writeable by default
----------------
sanitizer -> ASan


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:12
 //
-// XFAIL: arm
-
-// UNSUPPORTED: ios
+// RUN: %clangxx                                                       %s -o %t &&     %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
+// RUN: %clangxx_asan                                                  %s -o %t &&     %run %t 2>&1 | FileCheck %s --check-prefix=SANITIZE_NOT
----------------
Avoid `NOT` in FileCheck prefix names, because `-NOT` is the directive to do a "negative match", that is, "make sure this text does not appear".

How about:
`--check-prefix=COOKIE` for the cookie case and just skip `--check-prefix` for the "no cookie" (it's the default after all)


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:35
-  fprintf(stderr, "foo  : %p\n", foo);
-  fprintf(stderr, "alloc: %p\n", Foo::allocated);
-  assert(reinterpret_cast<uintptr_t>(foo) ==
----------------
Why remove this debug output?  It might be useful...


================
Comment at: compiler-rt/test/asan/TestCases/Posix/new_array_cookie_with_new_from_class.cpp:43-47
+// SANITIZE_YES: AddressSanitizer: heap-buffer-overflow
+// SANITIZE_YES: in main {{.*}}new_array_cookie_with_new_from_class.cpp:[[@LINE-2]]
+// SANITIZE_YES: is located 0 bytes inside of {{18|26}}-byte region
+  printf("Unsurprisingly, we were able to write to the array cookie\n");
+// SANITIZE_NOT: Unsurprisingly, we were able to write to the array cookie
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125195