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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added a comment.

In D60593#1483183 <https://reviews.llvm.org/D60593#1483183>, @hctim wrote:

> - Added a thread contention test. Creates allocations using multiple threads and checks to see that a guarded slot is never allocated twice. Also tests against the random slot selection.


There's no way to access the allocator directly in the tests? I ask because you could test stronger conditions if you had things like PointerIsMine in the tests.



================
Comment at: compiler-rt/lib/gwp_asan/options.inc:28
+GWP_ASAN_OPTION(
+    unsigned long, SampleRate, 5000,
+    "The probability (1 / SampleRate) that an allocation is selected for "
----------------
morehouse wrote:
>  I think this needs to be `uint64` or `unsigned long long`.
Feels like this should still be uint64/ULL (2^63 comment is otherwise not right on 32-bit platforms.)


================
Comment at: compiler-rt/test/gwp_asan/alignment_power_of_two.cpp:38
+    // left/right alignment, we test each allocation multiple times. We have a
+    // 0.195% chance of all allocations being left or right aligned when we do
+    // 10 tests.
----------------
Lets make this 2**-40 so it can never flake.


================
Comment at: compiler-rt/test/gwp_asan/alignment_power_of_two.cpp:50
+      // Normally causes buffer overflow, but alignment should prevent this.
+      volatile char x = *(Ptr + AllocSizeToAlignment[i + 1] - 1);
+      free(Ptr);
----------------
Seems redundant to the check above (if you assume that page size is always a power of two.)


================
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
+
----------------
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?)


================
Comment at: compiler-rt/test/gwp_asan/num_usable_slots.cpp:63
+  // Clean up allocations.
+  for (unsigned i = 0; SampledAllocs[i] != nullptr; ++i) {
+    free(SampledAllocs[i]);
----------------
There's UB in the 129 case (SampleAllocs OOB access), replace with i<NumSlots and maybe just make SampledAllocs a stack-allocated VLA?


================
Comment at: compiler-rt/test/gwp_asan/page_size.h:11
+
+#if defined (_WIN32)
+# define WIN32_LEAN_AND_MEAN
----------------
Delete, currently unused.


================
Comment at: llvm/docs/GwpAsan.rst:177
++------------------------+--------------------+---------------------------------------------------------------------------------+
+| NumUsableGuardedSlots  |  16                |  Number of usable guarded slots in the allocation pool.                         |
++------------------------+--------------------+---------------------------------------------------------------------------------+
----------------
s/NumUsableGuardedSlots/MaxSimultaneousAllocations/ ? It would also make the description a little clearer to explain it that way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593