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

Wed May 1 14:21:23 PDT 2019

morehouse added inline comments.

================
Comment at: compiler-rt/CMakeLists.txt:281
+option(GWP_ASAN_SCUDO_HOOKS
+  "When building scudo, should we install GWP-ASan hooks (default=False)?" OFF)
+pythonize_bool(GWP_ASAN_SCUDO_HOOKS)
----------------
hctim wrote:
> morehouse wrote:
> > I think the "(default=False)" is unnecessary since the default value is clearly "OFF".
> > 
> > Also, shouldn't this go in the Scudo cmake?
> Done. Also turned this default to True for supported platforms.
> 
> Needs to be defined this high in the build as it's used by `compiler-rt/test/lit.common.configured`.
This option has to do specifically with Scudo.  Therefore it should go in the Scudo cmake.

And Kostya K. needs to be OK with hooks being installed by default.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:206
+  if (SingletonPtr == nullptr)
+    exit(EXIT_FAILURE);
+  SingletonPtr->reportErrorAndDieInternal(AccessPtr, Error);
----------------
hctim wrote:
> morehouse wrote:
> > We should have an error message so the user knows what happened.
> At this point, we don't have access to a `printf()` library. 
> 
> Also, I'm not sure whether this is actually reachable. SingletonPtr is always initialised in this TU before the signal handlers are installed. Have changed this to an assert.
Ok, but the assert isn't enough.  In release builds we could still null-deref.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:182
+
+  bool ShouldRightAlign = (*Size % 2 == 1);
+
----------------
hctim wrote:
> vlad.tsyrklevich wrote:
> > hctim wrote:
> > > vlad.tsyrklevich wrote:
> > > > In Chrome we left- or right-align based on whether the (randomly chosen) slot is even/odd, that means an allocation will test for both under- and over-flows across different runs. However in this case we always left-/right-align based on the size which means a given type will only ever be tested one way (and I'd expect we have a lot more even allocations than odd allocations.)
> > > Gotcha. In this case, we need a runtime option (like the previous `AlignmentStrategy`) to guarantee left && right alignment for testing, as we don't want flaky tests.
> > > 
> > > How would you prefer to go about this? Two runtime options, `NoRandomAlignment` and `AlignRight`?
> > Why not just have the tests either 1) Use full-page allocations so that we know exactly where the bounds are or 2) a helper routine to get a left-/right-aligned allocation? Those are the two things I end up using in Chrome and they've been sufficient.
> FYI - I have adjusted this to be purely PRNG based as if `NumGuardedSlots` is odd, it will give us a higher probability of left-alignment. If `NumGuardedSlots == 1`, we want to still do random left/right alignment.
I expect it doesn't matter in practice.  GWP-ASan is already probabilistic, so who cares if we're x% more likely to detect underflow than overflow.  You could even invert things so we get more right-aligned allocations since overflow is more common than underflow.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:263
+      return Error::BUFFER_OVERFLOW;
+    else
+      return Error::BUFFER_UNDERFLOW;
----------------
Nit:  else is unnecessary

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:67
+    return NextSampleCounter-- == 1 &&
+           __atomic_load_n(&IsInitialised, __ATOMIC_RELAXED);
+  }
----------------
hctim wrote:
> morehouse wrote:
> > Can perf be improved by marking the `== 1` branch as `UNLIKELY`?
> It wont on x64 as AFAIK the static predictors can't be primed (and doesn't appear to change the optimiser), but have as it may affect other other platforms.
Regardless of branch prediction, the compiler may choose to make "other optimizations" to improve the hot path.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:162
+  // slots are available.
+  bool reserveSlot(std::size_t *PageIndex);
+
----------------
hctim wrote:
> morehouse wrote:
> > vlad.tsyrklevich wrote:
> > > morehouse wrote:
> > > > `ssize_t reserveSlot()` seems cleaner to me.  Also the function comment could be reworded clearer.
> > > I think this is because it's modeled on my implementation since with my design I reserve slots/metadata indices separately.
> > With `ssize_t` return value, we can still detect failure (-1) while not having to pass a return param.  What's the benefit of the current setup?
> For future expansion, we will want to populate a slot index and a separate metadata index. I've reworded up the comment but left the implementation as-is for now. WDYT?
I'd prefer simpler for now, with refactoring later.  It could be a while until "future expansion", so let's keep the code simple in the meantime.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:171
+  // slot in which the allocation should live.
+  void applyAllocationStrategy(std::size_t Size, uintptr_t *AllocationPtr);
+
----------------
hctim wrote:
> morehouse wrote:
> > Confusing interface and comment.  How about `uintptr_t AlignAllocation(size_t SlotIdx, size_t Size)`?
> I had a good think about the best way to do this adjustment. Does the rework LGTY?
Looks better.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:25
+// slots. It should always be treated as a singleton.
+// This class is **thread-hostile** until init() is completely finished. Any
+// implementing allocator must ensure that init() is called in a thread-safe
----------------
Technically doesn't this make it "thread-compatible" until init(), then thread-safe?

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:39
+
+  struct AllocationMetadata {
+    // Maximum number of stack trace frames to collect for allocations + frees.
----------------
hctim wrote:
> vitalybuka wrote:
> > can you hide this struct into cpp file and just keep forward declaration here?
> See https://reviews.llvm.org/D60593?id=195942#inline-541269. Happy to change.
GPA is the only user of `AllocationMetadata`, and the struct is relatively small.  Do we really want an entire file for ~20 lines of code?

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:78
+  // as this may cause a use-after-free on shutdown.
+  ~GuardedPoolAllocator() = default;
+
----------------
hctim wrote:
> vitalybuka wrote:
> > why do you need this this line at all "~GuardedPoolAllocator() = default;"?
> It's simply an explicit note to the reader that we have a trivial destructor. If they want to add a d'tor, they'll come to this line and read the note above.
Not necessary, but I think it does make the intention clear.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:76
+  if (PreviousSigactionHandler)
+    PreviousSigactionHandler(sig, info, ucontext);
+}
----------------
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.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:68
+  if (info == nullptr)
+    return;
+
----------------
These two checks are probably unnecessary.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator_posix.cpp:71
+  gwp_asan::GuardedPoolAllocator::reportErrorAndDie(
+      reinterpret_cast<uintptr_t>(info->si_addr));
+
----------------
I think we should avoid dying here so the previous signal handler can examine the SEGV too.

================
Comment at: compiler-rt/lib/gwp_asan/options.inc:28
+    int, NumUsableGuardedSlots, 16,
+    "Number of usable guarded slots in the allocation pool. Defaults to 16.")
+
----------------
What happens if I set a negative value?

================
Comment at: compiler-rt/lib/gwp_asan/options.inc:38
+    "GWP-ASan sampling. Default is 5000. Sample rates up to 2^63 are "
+    "supported.")
+
----------------
What happens if I set a negative sample rate?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593