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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added a comment.

A few higher level comments:

- do you have numbers for the overhead? code+data increase in Scudo .so, impact of VA & RSS/PSS of a "sample" process?
- 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)
- 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`)
- 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++.

The other comments are from a cursory glance, I will redo a more detailed pass.



================
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}
----------------
GWP_ASAN_OBJECT_LIBS not defined?


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


================
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;
----------------
const uintptr_t P?


================
Comment at: compiler-rt/lib/gwp_asan/mutex.h:34
+
+  std::atomic<bool> Semaphore{false};
+};
----------------
Does using `std::atomic` bring in a libc++ (or equivalent) dependency?


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:243
+    : Quarantine(LINKER_INITIALIZED) {
+      gwp_asan::options::initOptions();
+      GuardedAlloc.init(*gwp_asan::options::getOptions());
----------------
We are avoiding any non-trivial initialization on static objects. Ideally those 2 would go in `init`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593