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

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich accepted this revision.
vlad.tsyrklevich added inline comments.
This revision is now accepted and ready to land.


================
Comment at: compiler-rt/test/gwp_asan/no_reuse_ahead_of_time.cpp:35
+
+    // CAREFUL: We always use a sampled allocation to initialise GWP-ASan
+    // in the interface helpers. For this case, we already have called malloc
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > Why does anything need to care about initializing the GPA ahead of time for anything other than allocate? The underlying methods should return the correct values. And even for allocate() we can get around this edge case by doing allocate(PageSize+1).
> These helper funcs call through the singleton ptr, which is `nullptr` until `init()`. Calling `pointerIsMine()` (for example) would access `(nullptr)->GuardedPagePool`.
> 
> I like the `maximumAllocSize() + 1`. At the moment, `GPA::maximumAllocSize()`doesn't rely on any data members, but is still UB to call with a `nullptr` instance. I think I'll do this but with an arbritrarily large allocation and leave a note for me to update it when we support multipage slots.
But that's not true, all of the __gwp_asan_* functions fail gracefully if initialization hasn't occurred (except for deallocate which is never valid to call when uninitialized), e.g. pointer_is_mine would just return false. We do need initialization for the allocate() case so it works as expected, and I'm fine with wrapping all of the gwp_asan_* functions for consistency with allocate(), but I think we should only call maybeInit in the allocate case to make it explicit that that is the only one that needs it (and if that changes we can get rid of it and use the underlying methods directly.)


================
Comment at: compiler-rt/test/gwp_asan/reuse_quarantine.cpp:8
+// RUN: %env_gwp_asan_options=MaxSimultaneousAllocations=2 %run %t
+// RUN: %env_gwp_asan_options=MaxSimultaneousAllocations=127 %run %t
+// RUN: %env_gwp_asan_options=MaxSimultaneousAllocations=128 %run %t
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > Maybe delete the 1/2 cases (will fail on runtimes that call malloc before main() and just call it with some other higher value that should always work like 32.
> I've instead rewrote it so that it exercises the allocator directly with no sampling, so that we avoid this issue. LGTY?
Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593