[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.
Matt Morehouse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon May 6 18:22:21 PDT 2019
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
More information about the llvm-commits
mailing list