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

Fri Apr 12 13:22:50 PDT 2019

hctim added a comment.

In D60593#1463706 <https://reviews.llvm.org/D60593#1463706>, @vlad.tsyrklevich wrote:

> - AllocationStrategy/AlignmentStrategy don’t seem useful to me as they are. Are they just for testing? Could we get rid of them by allocating full-page allocations instead in the under/overflow tests instead? They complicate the implementation and I don’t know why people would use them outside of flipping a flag to say “full right-alignment” instead of whatever reasonable alignment strategy we choose for them. Perhaps collapse both into a single bool PerfectlyRightAlign flag and just have the default alignment strategy be something we choose?

I've implemented them here because it gives us a few benefits:

1. Allows us to test different configuration options at runtime without a recompile.
2. Gives allocators more choice as to how they use GWP-ASan (they might provide alignment guarantees that we can't).
3. We can A/B test deployments in production. We can have half a fleet using `OddLeftEvenRight` and half a fleet using `OddRightEvenLeft`, and this may hypothetically give us the best chance of finding bugs.

Happy to take feedback on this, if others see it as unneccessary we can delete them.

In D60593#1463706 <https://reviews.llvm.org/D60593#1463706>, @vlad.tsyrklevich wrote:

> - What is the plan for adding stack trace support?

The plan is to add another function pointer to the runtime configuration flags. This would allow each allocator to support the unwinding in their own way, and we can back onto the sanitizer_common flags by-default if an allocator doesn't want to write their own unwinder/wrap someone elses (similar to how we do the runtime env flag parsing right now).

---

In D60593#1464464 <https://reviews.llvm.org/D60593#1464464>, @cryptoad wrote:

> - do you have numbers for the overhead? code+data increase in Scudo .so, impact of VA & RSS/PSS of a "sample" process?

No, but I'll find those out for you very shortly and report back.

In D60593#1464464 <https://reviews.llvm.org/D60593#1464464>, @cryptoad wrote:

> - the integration in Scudo should be gated by defines/cmake rules: there is currently a discrepancy between the platform supported by Scudo & GWP-Asan (Fuchsia for one and not the other) so a strong way to exclude all GWP-Asan should exist here (as far as I know there is no Fuchsia bot yet)

Done. You now have to add `-DGWP_ASAN_SCUDO_HOOKS` to your CMake to enable GWP-ASan.

In D60593#1464464 <https://reviews.llvm.org/D60593#1464464>, @cryptoad wrote:

> - was it tested on Android? As discussed, not all platforms support ELF TLS (Android added it somewhat recently, but before that it used emutls which uses `malloc`)

Not yet, but Android >= Q support ELF TLS, and that's our current support goal. Will be testing soon (before submit).

In D60593#1464464 <https://reviews.llvm.org/D60593#1464464>, @cryptoad wrote:

> - there are some dependencies on standard C++ (std::atomic for example), and I want to make sure that this won't pull in libc++ (or other C++ runtime), the reason being that if a program is C only, it shouldn't load libc++.

(See more detailed comment here <https://reviews.llvm.org/D60593#inline-537093>) I've intentionally written it at this point to use header-only libstdc++, so that after GwpAsan is compiled, there is no dependency on the standard library.

================
Comment at: compiler-rt/lib/gwp_asan/CMakeLists.txt:47
+      ADDITIONAL_HEADERS ${GWP_ASAN_HEADERS}
+      OBJECT_LIBS ${GWP_ASAN_OBJECT_LIBS}
+      CFLAGS ${GWP_ASAN_CFLAGS}
----------------
cryptoad wrote:
> GWP_ASAN_OBJECT_LIBS not defined?
Removed.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:52
+  Metadata = reinterpret_cast<AllocationMetadata *>(
+      malloc(NumGuardedSlots * sizeof(AllocationMetadata)));
+
----------------
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).

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:55
+  // Allocate memory and set up the free pages queue.
+  FreePages = reinterpret_cast<std::size_t *>(
+      malloc(NumGuardedSlots * sizeof(std::size_t)));
----------------
vlad.tsyrklevich wrote:
> Is std::size_t a scudo thing? Otherwise I'd just replace with just size_t?
AFAIK, the official stddef for size_t in C++ is `std::size_t`. At the least, `/s/std::size_t/size_t` in `guarded_pool_allocator.h` will error when trying to find `size_t`.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:321
+  } else {
+    Meta = addrToMetadata(AccessPtr);
+  }
----------------
vlad.tsyrklevich wrote:
> The rest of this routine assumes Meta is valid but it may not be (e.g. a wild access to a page that's never been allocated yet.)
The invariant here is that the only time when `Meta` is invalid is with an `UNKNOWN` error. I've made the switch for `UNKNOWN` exit instead of fallthrough and added an `assert()` to clarify this invariant below.

A wild access should set `Meta` but keep `ErrorType == UNKNOWN`. Hopefully this helps.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:55
+    if (UNLIKELY(SampleRate == 0))
+      return false;
+
----------------
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.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:67
+    case 1:
+      NextSampleCounter = (Random::getRandomUnsigned64() % SampleRate) + 1;
+      return true;
----------------
vlad.tsyrklevich wrote:
> vlad.tsyrklevich wrote:
> > vlad.tsyrklevich wrote:
> > > Random::getRandomUnsigned64() % SampleRate means we are sampling more often than the actual sampling rate. This should be % (SampleRate * 2), or, even better, sample from a geometric distribution
> > Woops, seems I made the same mistake in http://crrev.com/c/1400050
> Actually I take that back. After reading it again that code uses a complicated sampling mechanism that does sample 1/N, the reason I chose it is so that we did not undersample the first SampleRate allocations as this scheme will. If it's possible to use the standard library I'd just replace it with a geometric distribution from the beginning if you can.
Unfortunately I'm trying to stick to header-only c++stdlib, is it okay as currently implemented?

================
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;
----------------
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`) :)

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:88
+
+uint64_t GuardedPoolAllocator::getThreadID() { return getpid(); }
+
----------------
vlad.tsyrklevich wrote:
> gettid()?
Unsupported until glibc 2.30, but have implemented the syscall w/ fallback to `UINT64_MAX`.

================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:34
+
+  std::atomic<bool> Semaphore{false};
+};
----------------
cryptoad wrote:
> Does using `std::atomic` bring in a libc++ (or equivalent) dependency?
It brings in a libc++ dependency when building GWP-ASan, but no further than that (i.e. does not affect Scudo). `std::atomic<bool>` is header-only, and although we don't have any strong guarantess for this, I believe it to be a safe bet.

@pcc - can you confirm that this is a safe bet?

================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:243
+    : Quarantine(LINKER_INITIALIZED) {
+      gwp_asan::options::initOptions();
+      GuardedAlloc.init(*gwp_asan::options::getOptions());
----------------
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?

================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:491
+      void *NewPtr = allocate(NewSize, MinAlignment, FromMalloc);
+      if (NewPtr) {
+        gwp_asan::AllocationMetadata *Meta =
----------------
vlad.tsyrklevich wrote:
> if (!NewPtr) then we never deallocate? Also is it possible that this will be reached with NewSize=0 like in the realloc() case or not? e.g. do we need to explicitly handle it here
Fixed. Also, if `NewSize==0`, then Scudo will provide the backing allocation through `allocate`, and we should proceed as normal (zero bytes will be copied and dealloc from GPA will take place).

================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:494
+            GuardedAlloc.addrToMetadata(reinterpret_cast<uintptr_t>(OldPtr));
+        memcpy(NewPtr, OldPtr, Meta->Size);
+        GuardedAlloc.deallocate(OldPtr);
----------------
vlad.tsyrklevich wrote:
> std::min(Meta->Size, NewSize)
Done (but w/o `std::` as Scudo doesn't want dependency on `c++stdlib`).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593