[PATCH] D62929: [GWP-ASan] Integration with Scudo [5].

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/scudo/scudo_allocator.cpp:493
+#ifdef GWP_ASAN_HOOKS
+    if (UNLIKELY(GuardedAlloc.pointerIsMine(OldPtr))) {
+      size_t OldSize = GuardedAlloc.getSize(OldPtr);
----------------
vlad.tsyrklevich wrote:
> Just checking, does reallocate() have the same semantics as realloc() where NewSize == 0 means free? And does allocate(0) always return nullptr in that case?
Yep - have also added a unit test for this. Note that C++11 changed this behaviour, and made it implementation defined.


================
Comment at: compiler-rt/test/gwp_asan/invalid_free_left.cpp:3
+// RUN: %clangxx_gwp_asan %s -o %t
+// RUN: not %run %t 2>&1 | FileCheck %s
+
----------------
vlad.tsyrklevich wrote:
> This uses not while the others above use %expect_crash, you probably want %expect_crash here too and in the other places you use not?
So at the moment, invalid free/double free are detected internally, and terminate through `exit(EXIT_FAILURE)`. The tests with `%expect_crash` are the ones which terminate through a segv signal (`$? == 127`).

This is mirroring the behaviour of scudo, where they `exit(EXIT_FAILURE)` on checksum failure or other error conditions. What might be better though, is when we detect a failure condition internally, we raise a signal. This would allow platforms like Android to still have their special death handlers run.

@morehouse - WDYT? I would say that I'd prefer to raise a signal internally-detected error. I'm not sure that raising SEGV is appropriate here, and it would also increase the complexity of the logic (we'd have to reinstate the previous SEGV handler first). Maybe SIGTRAP or SIGABRT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62929