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

Fri May 3 10:56:10 PDT 2019

hctim 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)
----------------
morehouse wrote:
> 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.
I've changed this flag to not exist any more, as it's synonymous with `COMPILER_RT_GWP_ASAN_ENABLED` anyway.

@cryptoad and I have confirmed offline that always-built on supported platforms is okay :)

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:182
+
+  bool ShouldRightAlign = (*Size % 2 == 1);
+
----------------
morehouse wrote:
> 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.
I think the major reason for me is that it may be a use case to have a guarded pool with only a single slot for short-lived processes on memory-constrained devices. We're planning on process sampling as well for Android, and it may be useful to service short-lived processes with a single allocation instead of disabling it completely.

================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:162
+  // slots are available.
+  bool reserveSlot(std::size_t *PageIndex);
+
----------------
morehouse wrote:
> 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.
Gotcha. `ssize_t` is POSIX-only (see [[ https://docs.microsoft.com/en-us/cpp/c-runtime-library/standard-types?view=vs-2019 | Windows docs ]]) so I've made it return `kInvalidSlotID` instead.

================
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
----------------
morehouse wrote:
> Technically doesn't this make it "thread-compatible" until init(), then thread-safe?
Yep, updated documentation to reflect this.

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

================
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.")
+
----------------
morehouse wrote:
> What happens if I set a negative value?
The error checking is done in the flag parser, but I've also added explicit checking in `init()` as well.

================
Comment at: compiler-rt/test/gwp_asan/num_usable_slots.cpp:60
+  char *Ptr = reinterpret_cast<char*>(malloc(PageSize));
+  // The system allocator doesn't have use-after-free READ detection. If this
+  // was a sampled allocation, this would be caught.
----------------
vlad.tsyrklevich wrote:
> I'm not sure what a better test is but this isn't always true (e.g. the allocator could actually take back that page if all the allocations on it are gone.) These tests could test stronger assertions and be simpler if there was a __gwp_asan_pointer_is_mine() or something like that.
Changed it to use the same mechanism from above to detect if it's in the pool.

I think the current assumption is strong enough, but may be broken if `atoi()` or `alloca()` changes to call malloc (unlikely), or if a shared library calls `malloc()` during dynamic loading. WDYT?

Edit: On further thought, this also will spuriously pass if the implementing allocator uses the page immediately after the allocation pool to back the allocation. I've added interface definitions and started a cleanup so that we can exercise the allocator internals directly.

================
Comment at: compiler-rt/test/gwp_asan/num_usable_slots.cpp:12
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=128 %run %t 128 2>&1
+// RUN: %env_gwp_asan_options=NumUsableGuardedSlots=129 %run %t 129 2>&1
+
----------------
vlad.tsyrklevich wrote:
> vlad.tsyrklevich wrote:
> > Add a negative test or two (e.g. do we actually fail when invoked with 8 slots but we iterate 9 times? Do we fail when we have 8 slots but iterate 7 times?)
> We test +1 case but not the -1 case.
I'm confused why we would need to test the -1 case (assuming that -1 means `NumIterations == MaxSimultaneousAllocations - 1`), given that  `NumIterations == MSA` provide this coverage. Is my assumption correct?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593