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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:52
+  Metadata = reinterpret_cast<AllocationMetadata *>(
+      malloc(NumGuardedSlots * sizeof(AllocationMetadata)));
+
----------------
hctim wrote:
> eugenis wrote:
> > hctim wrote:
> > > cryptoad wrote:
> > > > `GuardedPoolAllocator::init` is called within the constructor of `Allocator` for Scudo, so `malloc` here doesn't sound like it would work?
> > > > Any heap related function should probably be avoided.
> > > So there's a neat little trick we're using here. `SampleRate` is initialised intentionally at the end of `init()`, so that any call to `shouldSample()` returns false. This means that the `malloc()` calls do not fall into the guarded pool before the `GuardedPoolAllocator` is completely initialised. This trick also allows us to have GWP-ASan disabled at runtime without creating the allocation pool (and thus reducing static memory overhead).
> > Could full initialization be done lazily at the first sampled malloc?
> > At least, we should not allocate metadata array if the process is going to have sample rate of 0.
> We don't allocate if it's disabled (see the early return in `init()` on line 40. What benefits do we get from lazy-init?
lazy-init lets us use malloc.

user malloc -> gwp-asan-sample -> gwp-asan-malloc -> gwp-asan-init -> malloc -> (sampling disabled for recursive mallocs) -> scudo-malloc -> (possibly) scudo-init



================
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:
> > > 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?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593