[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: compiler-rt/test/gwp_asan/num_usable_slots.cpp:12
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=128 %run %t 128 2>&1
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=129 %run %t 129 2>&1
+
----------------
vlad.tsyrklevich wrote:
> Add a negative test or two (e.g. do we actually fail when invoked with 8 slots but we iterate 9 times? Do we fail when we have 8 slots but iterate 7 times?)
We test +1 case but not the -1 case.


================
Comment at: compiler-rt/test/gwp_asan/num_usable_slots.cpp:60
+  char *Ptr = reinterpret_cast<char*>(malloc(PageSize));
+  // The system allocator doesn't have use-after-free READ detection. If this
+  // was a sampled allocation, this would be caught.
----------------
I'm not sure what a better test is but this isn't always true (e.g. the allocator could actually take back that page if all the allocations on it are gone.) These tests could test stronger assertions and be simpler if there was a __gwp_asan_pointer_is_mine() or something like that.


================
Comment at: compiler-rt/test/gwp_asan/thread_contention_with_release.cpp:32
+    volatile char *Ptr = reinterpret_cast<volatile char *>(malloc(pageSize()));
+    // Do any other threads have access to this page?
+    if (*Ptr != 0)
----------------
*Ptr may not be 0 if we fall through to regular malloc() (either here or on the realloc call below), perhaps we should risk running this test on normal malloc()/realloc() sometimes and have NumAllocsTotal fully deplete the free list instead?


================
Comment at: compiler-rt/test/gwp_asan/thread_contention_with_release.cpp:72
+  int NumGuardedSlots = atoi(argv[1]);
+  // Ensure that we have enough slots for the STL allocations, primarily the
+  // std::set.
----------------
I'm not sure I understand this logic: 1) what std::set? (the std::vector<std::thread>?) 2) we should be able to consume more than the total number of guarded allocations if we wanted right since it would just fall back to normal malloc?


================
Comment at: llvm/docs/GwpAsan.rst:152
+
+  GWP_ASAN_OPTIONS="NumUsableGuardedPages=16:SampleRate=5000" ./a.out
+
----------------
Replace NumUsableGuardedPages


================
Comment at: llvm/docs/GwpAsan.rst:159
+  extern "C" const char *__gwp_asan_default_options() {
+    return "NumUsableGuardedPages=16:SampleRate=5000";
+  }
----------------
Ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593