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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:243
+    : Quarantine(LINKER_INITIALIZED) {
+      gwp_asan::options::initOptions();
+      GuardedAlloc.init(*gwp_asan::options::getOptions());
----------------
hctim wrote:
> eugenis wrote:
> > hctim wrote:
> > > eugenis wrote:
> > > > hctim wrote:
> > > > > cryptoad wrote:
> > > > > > We are avoiding any non-trivial initialization on static objects. Ideally those 2 would go in `init`.
> > > > > So this kind of conflicts with my previous statement. If we move this initialisation into `init()`, we will recurse into `init() -> Allocator::allocate() -> initThreadMaybe() -> init()` and we deadlock on `pthread_once`.
> > > > > 
> > > > > We have two options here:
> > > > > 1. Leave the GwpAsan initialisation in the c'tor, which forces the initialisation of Scudo at startup, rather than lazily initialising.
> > > > > 2. We avoid malloc() and instead do our own memory management in GwpAsan's `init()`. 
> > > > > 
> > > > > @eugenis - What do you think is the right long-term support option for GwpAsan here?
> > > > how about
> > > > 3. Do "fast pass" initialization in init(), then do full initialization on first sampled malloc
> > > > 
> > > > "fast pass" initialization is just enough to do sampling, i.e. the inlined part of shouldSample().
> > > > 
> > > > Since init() can run in a secondary thread, the access to SampleRate can race and needs to be changed to an atomic.
> > > So this would be a problem with lazy-init of GWP-ASan. For lazy-initialisation, we would require  `NextSampleCounter`, `SampleRate`, `GuardedPagePool`, and `GuardedPagePoolEnd` to all be atomic (and at least one with . At the moment, `init()` is run before any allocations take place, so it shouldn't race. Changing these to atomics would incur a significant performace overhead to the allocator's fast path.
> > > 
> > > I'm leaning towards #2 here, and requiring the c'tor to initialise GwpAsan before any allocations take place.
> > Relaxed atomic. We don't need any fancy memory ordering here, only atomicity of individual stores and loads. They will compile to the same code as non-atomic, or very similar.
> > 
> > #2 is also fine, but it limits us a little. Not a big deal, I guess, if we already are not allowed to use libc in common gwp-asan code because of fuchsia - is that correct?
> > 
> I think we've decided to do memory-mapped pages instead.
I've just remembered that I forgot to update the aforementioned values to atomics, working on it now. My initial experiments show that the relaxed atomics (on x86_64) compile down to the same bytecode, and should not create a performance issue.

Will run the synthetic tests as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593