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

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 15 inline comments as done.
hctim added inline comments.


================
Comment at: compiler-rt/include/sanitizer/gwp_asan_interface.h:1
+//===-- sanitizer/gwp_asan_interface.h --------------------------*- C++ -*-===//
+//
----------------
morehouse wrote:
> I think I missed something.  What is the purpose of these functions?
See comment in `compiler-rt/lib/gwp_asan/gwp_asan_interface_internal.h`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:76
+  if (PreviousSigactionHandler)
+    PreviousSigactionHandler(sig, info, ucontext);
+}
----------------
morehouse wrote:
> 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
Surprisingly, it worked on my machine. Have changed to ensure this works everywhere.


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


================
Comment at: compiler-rt/lib/gwp_asan/gwp_asan_interface_internal.h:1
+//===-- gwp_asan_interface_internal.h ---------------------------*- C++ -*-===//
+//
----------------
morehouse wrote:
> Why do we want this?
To allow us to exercise the allocator directly for testing. The comments in `compiler-rt/test/gwp_asan/num_usable_slots.cpp` has some more context, but the summary is:

  # Allows us to be precise with exactly which allocations are sampled (important, for example, in `tests/num_usable_slots.cpp` where if a library calls malloc() during its init, we will break. Also, in the `thread_contention*` tests, we want to be certain that we are exercising the sampled allocator, rather than potentially falling back to the system allocator.
  # Allows us to test directly against the interface whether something is sampled through `__gwp_asan_pointer_is_mine()`, avoiding relying on implementation specific assumptions. In `tests/num_usable_slots.cpp`, we were previously relying on left-to-right sequential usage of the sampled slots to determine whether an allocation was sampled. We then had no method to determine whether an allocation was not sampled, as the system allocator may place the unsampled allocation in the page directly after the guarded pool, leading us to spurious failure.



================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:305
+#ifdef GWP_ASAN_HOOKS
+    if (UNLIKELY(GuardedAlloc.shouldSample())) {
+      if (void *Ptr = GuardedAlloc.allocate(Size))
----------------
morehouse wrote:
> 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.
[[ https://pastebin.com/RNUyYBkC | before ]], [[ https://pastebin.com/XfyBtDsp | after ]] the changes


================
Comment at: compiler-rt/test/gwp_asan/no_reuse_ahead_of_time.cpp:35
+
+    // CAREFUL: We always use a sampled allocation to initialise GWP-ASan
+    // in the interface helpers. For this case, we already have called malloc
----------------
vlad.tsyrklevich wrote:
> hctim wrote:
> > vlad.tsyrklevich wrote:
> > > Why does anything need to care about initializing the GPA ahead of time for anything other than allocate? The underlying methods should return the correct values. And even for allocate() we can get around this edge case by doing allocate(PageSize+1).
> > These helper funcs call through the singleton ptr, which is `nullptr` until `init()`. Calling `pointerIsMine()` (for example) would access `(nullptr)->GuardedPagePool`.
> > 
> > I like the `maximumAllocSize() + 1`. At the moment, `GPA::maximumAllocSize()`doesn't rely on any data members, but is still UB to call with a `nullptr` instance. I think I'll do this but with an arbritrarily large allocation and leave a note for me to update it when we support multipage slots.
> But that's not true, all of the __gwp_asan_* functions fail gracefully if initialization hasn't occurred (except for deallocate which is never valid to call when uninitialized), e.g. pointer_is_mine would just return false. We do need initialization for the allocate() case so it works as expected, and I'm fine with wrapping all of the gwp_asan_* functions for consistency with allocate(), but I think we should only call maybeInit in the allocate case to make it explicit that that is the only one that needs it (and if that changes we can get rid of it and use the underlying methods directly.)
My mistake. They don't cause error, but they don't return correct values until init has taken place.

We also need init support for `maximumAllocationSize()` from `interface_test.cpp` and `interface_test_failures.cpp` with their current implementation. We could add explicit initialisation by caling a dummy `__internal::allocate() / deallocate()` in these specific tests. (And, I guess `thread_contention_with_release.cpp` as well if we can't make the strong guarantee that `std::vector<std::thread>::emplace_back()` always calls the system allocator.)

It seems to make the interface a lot less prone to test implementation errors. Happy to change if if you feel strongly about this, but apart from performance of the tests, I don't see what we gain (especially when `scudo::allocate` always does a once-init check as well through `initThreadMaybe()`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593