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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
