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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich requested changes to this revision.
vlad.tsyrklevich added a comment.
This revision now requires changes to proceed.

Found some bugs, requesting changes to track that I'd like to review the fixes and tests for that behavior.



================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:168
+  if (NumUsedSlots < NumGuardedSlots) {
+    ReservedIndex = NumUsedSlots++;
+  } else {
----------------
This algorithm doesn't work, imagine NumGuardedSlots=4. Then you first start off with 0 1 2 3 + (The plus donates FreeSlotsLength.) Then after an allocation you have 3 1 2 + 3, after another allocation you have 3 2 + 2 3. Now your ReservedIndex reads past the end of FreeSlotsLength which is wrong. If you free the first pointer you get 3 2 0 + 3. Another allocation 3 2 + 0 3, then 3 + 2 0 2. Now you've hit NumUsedSlots == NumGuardedSlots and you'll allocate slot 3 (again.)

This highlights the importance of tests like AllocDeallocAllPages, ThreadedAllocCount, and ThreadedHighContention in https://github.com/chromium/chromium/blob/master/components/gwp_asan/client/guarded_page_allocator_unittest.cc

We need to add tests ensuring that in both single- and multi-threaded contexts we are never returning slots twice.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:246
+              "GuardedPoolAllocator must be trivially destructible.");
+}; // namespace gwp_asan
+
----------------
nit: unnecessary semi-colon


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:42
+private:
+  Mutex *Mutex;
+};
----------------
gcc doesn't like this being named identically.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:24
+GWP_ASAN_OPTION(
+    int, NumUsableGuardedSlots, 16,
+    "Number of usable guarded slots in the allocation pool. Defaults to 16.")
----------------
The documentation and tests use NumUsableGuardedPages but it's not directly tested anywhere so it's not caught that this is named differently here and doesn't work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593