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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/include/sanitizer/gwp_asan_interface.h:1
+//===-- sanitizer/gwp_asan_interface.h --------------------------*- C++ -*-===//
+//
----------------
I think I missed something.  What is the purpose of these functions?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:76
+  if (PreviousSigactionHandler)
+    PreviousSigactionHandler(sig, info, ucontext);
+}
----------------
hctim wrote:
> morehouse wrote:
> > There are also `SIG_DFL` and `SIG_IGN` cases.  See https://critique.corp.google.com/#review/197822848/depot/google3/tcmalloc/guarded_page_allocator.cc for an example.
> Done with some slight modifications. If the previous handler was `SIG_IGN`, we die as we'll constantly restart on the memory error and repeatedly spam GWP-ASan output. This also aligns with the exit behaviour of DOUBLE_FREE/INVALID_FREE and that of SIGSEGV-delivered errors.
> 
> We also don't explicitly handle `SIG_DFL`, as the fallthrough should call the default signal handler just fine.
> 
> 
I am skeptical that calling `SIG_DFL()` will work.

SIG_DFL is not a function address.

https://en.cppreference.com/w/cpp/utility/program/SIG_strategies


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:74
+    signal(SIGSEGV, SIG_DFL);
+    raise(SIGSEGV);
+  } else {
----------------
I'm not opposed to ignoring `SIG_IGN`, but this should be documented somewhere.


================
Comment at: compiler-rt/lib/gwp_asan/gwp_asan_interface_internal.h:1
+//===-- gwp_asan_interface_internal.h ---------------------------*- C++ -*-===//
+//
----------------
Why do we want this?


================
Comment at: compiler-rt/lib/gwp_asan/random.cpp:17
+TLS_INITIAL_EXEC uint64_t RandomStateA = static_cast<uint64_t>(time(nullptr));
+TLS_INITIAL_EXEC uint64_t RandomStateB = xorShiftStar64();
+
----------------
Looks like initialization of these globals could happen after the first call to `getRandom`.


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:305
+#ifdef GWP_ASAN_HOOKS
+    if (UNLIKELY(GuardedAlloc.shouldSample())) {
+      if (void *Ptr = GuardedAlloc.allocate(Size))
----------------
hctim wrote:
> morehouse wrote:
> > What does the assembly for this look like on the fast path?
> See attached image in description.
Could you just post the assembly?  Preferably a before/after of this fastpath change.

The CFG is confusing for me to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593