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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:23
+namespace gwp_asan {
+GuardedPoolAllocator::~GuardedPoolAllocator() {
+  if (GuardedPagePool) {
----------------
hctim wrote:
> morehouse wrote:
> > This is going to cause racy use-after-dtors on thread exit.
> > 
> > We need a trivial dtor if we want a global `GuardedPoolAllocator`, or we need to avoid destruction.
> Have removed the nontrivial d'tor and explained that the class leaks if destructed during the lifetime of a program.
I think we still need a `static_assert` to make sure the destructor is actually trivial.  It would be pretty easy for someone to introduce a new member that precludes trivial dtor.

Or you need to tell the user to never construct as a global and have a code sample showing how to construct with placement new.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:267
+GuardedPoolAllocator::reportErrorAndDieInternal(uintptr_t AccessPtr,
+                                                GwpAsanError ErrorType) {
+  if (!pointerIsMine(reinterpret_cast<void *>(AccessPtr)))
----------------
morehouse wrote:
> This function is way too long.
Can we put some logic in helper functions?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:55
+    if (UNLIKELY(SampleRate == 0))
+      return false;
+
----------------
vlad.tsyrklevich wrote:
> hctim wrote:
> > vlad.tsyrklevich wrote:
> > > morehouse wrote:
> > > > morehouse wrote:
> > > > > hctim wrote:
> > > > > > eugenis wrote:
> > > > > > > morehouse wrote:
> > > > > > > > It would be nice to avoid this check on the fast path.
> > > > > > > > 
> > > > > > > > Perhaps we can do `rand() % (SampleRate + 1)` and then check `SampleRate !=0` only just before we would return true.
> > > > > > > Or it could be another special value of NextSampleCounter, which could also serve as a way to temporarily disable sampling in a thread.
> > > > > > > 
> > > > > > Yes and no. We would have to set `SampleRate = Opts.SampleRate - 1` to get precise sampling behaviour, which would make a sample rate of always on ("1") be the same as uninitialised.
> > > > > > 
> > > > > > I've added another flag `IsInitialised` in order to counteract this.
> > > > > This doesn't seem any better.  We're still checking for initialization on the fast path.
> > > > Sorry, disregard the last comment; it's actually not on the fast path.  This looks like it will have better perf.
> > > Is it not still on the fast path? I thought we'd want the if (IsInitialized) check to be in if (UNLIKELY(counter ==0)) to get it out of there?
> > Tying to the discussion in `scudo_allocator:236`, I think that we should scrap calling `malloc()` from within GwpAsan's init. This would allow us to not have an initialisation check entirely, with the strict requirement that `GuardedPoolAllocator::init()` is called before any malloc takes place. WDYT?
> I'm fine with getting rid of it. I do think it's much nicer to use the simple malloc primitives, but it's not the end of the world.
+1 to removing `malloc` inside init.  The recursion seems subtle and potentially bug-prone.  We're already mapping pages, so why not map some extra for internal structures?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:78
+  ALWAYS_INLINE bool pointerIsMine(const void *Ptr) const {
+    uintptr_t P = reinterpret_cast<const uintptr_t>(Ptr);
+    return GuardedPagePool <= P && P < GuardedPagePoolEnd;
----------------
hctim wrote:
> cryptoad wrote:
> > const uintptr_t P?
> I don't think this is necessary, P is a copy of `Ptr` (just a casted version of `Ptr`) :)
Then why are we casting to `const uintptr_t`?


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:21
+GWP_ASAN_OPTION(
+    bool, PerfectlyRightAlign, false,
+    "When allocations are right-aligned, should we perfectly align them up to "
----------------
malloc is required to return memory that is "suitably aligned".  Won't perfect right alignment violate this?

E.g.,
```
uint8_t *buf = malloc(5);
// ... Copy some data into buf ...
uint32_t *word = (uint32_t *) buf;
*word += buf[4];
```

I think if malloc right-aligns, some processors would fail to load the 32-bit word on the last line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593