[PATCH] D73557: [GWP-ASan] Crash Handler API.

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 33 inline comments as done.
hctim added inline comments.
Herald added a subscriber: dexonsmith.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:1
 //===-- guarded_pool_allocator.cpp ------------------------------*- C++ -*-===//
 //
----------------
Most of the changes in this file are `/s/MyMember/State.MyMember`


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:130
   if (Opts.SampleRate < 0) {
-    Opts.Printf("GWP-ASan Error: SampleRate is < 0.\n");
-    exit(EXIT_FAILURE);
+    assert(false && "GWP-ASan Error: SampleRate is < 0.");
+    __builtin_trap();
----------------
No `Printf` maintained internally any more, so we assert and trap instead.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:221
+
+static uintptr_t getPageAddr(uintptr_t Ptr, uintptr_t PageSize) {
+  return Ptr & ~(PageSize - 1);
----------------
Used to be a member function (`GPA::getPageAddr()`), but shouldn't be.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:320
 
-size_t GuardedPoolAllocator::maximumAllocationSize() const { return PageSize; }
+static AllocationMetadata *addrToMetadata(const AllocatorState *State,
+                                          AllocationMetadata *Metadata,
----------------
Slight functional change, `addrToMetadata` used to be implemented differently.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:409
+
+// ============================================================================
+// Implementation of crash handler API.
----------------
Start of implementation of new crash handler functions. All very simple implementation (just find the metadata, and then return data).


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:413
-namespace {
-// Prints the provided error and metadata information.
-void printErrorType(Error E, uintptr_t AccessPtr, AllocationMetadata *Meta,
----------------
This code was moved (with some modifications) to `crash_handler_posix.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:516
 
-struct ScopedEndOfReportDecorator {
-  ScopedEndOfReportDecorator(options::Printf_t Printf) : Printf(Printf) {}
----------------
Code here (and below) moved to `crash_handler_posix.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:22
 namespace gwp_asan {
+enum class Error {
+  UNKNOWN,
+  USE_AFTER_FREE,
+  DOUBLE_FREE,
+  INVALID_FREE,
+  BUFFER_OVERFLOW,
+  BUFFER_UNDERFLOW
+};
+
+const char *ErrorToString(const Error &E);
+
+// This struct contains all the metadata recorded about a single allocation made
+// by GWP-ASan. If `AllocationMetadata.Addr` is zero, the metadata is non-valid.
+struct AllocationMetadata {
+  static constexpr uint64_t kInvalidThreadID = UINT64_MAX;
+
+  // The number of bytes used to store a compressed stack frame. On 64-bit
+  // platforms, assuming a compression ratio of 50%, this should allow us to
+  // store ~64 frames per trace.
+  static constexpr size_t kStackFrameStorageBytes = 256;
+
+  // Maximum number of stack frames to collect on allocation/deallocation. The
+  // actual number of collected frames may be less than this as the stack
+  // frames are compressed into a fixed memory range.
+  static constexpr size_t kMaxTraceLengthToCollect = 128;
+
+  // Records the given allocation metadata into this struct.
+  void RecordAllocation(uintptr_t Addr, size_t Size,
+                        options::Backtrace_t Backtrace);
+
+  // Record that this allocation is now deallocated.
+  void RecordDeallocation(options::Backtrace_t Backtrace, bool &RecursiveGuard);
+
+  struct CallSiteInfo {
+    // The compressed backtrace to the allocation/deallocation.
+    uint8_t CompressedTrace[kStackFrameStorageBytes];
+    // The thread ID for this trace, or kInvalidThreadID if not available.
+    uint64_t ThreadID = kInvalidThreadID;
+    // The size of the compressed trace (in bytes). Zero indicates that no
+    // trace was collected.
+    size_t TraceSize = 0;
+  };
+
+  // The address of this allocation. If zero, the rest of this struct isn't
+  // valid, as the allocation has never occurred.
+  uintptr_t Addr = 0;
+  // Represents the actual size of the allocation.
+  size_t Size = 0;
+
+  CallSiteInfo AllocationTrace;
+  CallSiteInfo DeallocationTrace;
+
+  // Whether this allocation has been deallocated yet.
+  bool IsDeallocated = false;
+};
+
+// This holds the state that's shared between the GWP-ASan allocator and the
+// crash handler. This, in conjunction with the Metadata array, forms the entire
+// set of information required for understanding a GWP-ASan crash.
+struct AllocatorState {
+  // Returns whether the provided pointer is a current sampled allocation that
+  // is owned by this pool.
+  GWP_ASAN_ALWAYS_INLINE bool pointerIsMine(const void *Ptr) const {
+    uintptr_t P = reinterpret_cast<uintptr_t>(Ptr);
+    return P < GuardedPagePoolEnd && GuardedPagePool <= P;
+  }
+
+  // Returns the address of the N-th guarded slot.
+  uintptr_t slotToAddr(size_t N) const;
+
+  // Returns the largest allocation that is supported by this pool.
+  size_t maximumAllocationSize() const;
+
+  // Gets the nearest slot to the provided address.
+  size_t getNearestSlot(uintptr_t Ptr) const;
+
+  // Returns whether the provided pointer is a guard page or not. The pointer
+  // must be within memory owned by this pool, else the result is undefined.
+  bool isGuardPage(uintptr_t Ptr) const;
+
+  // The number of guarded slots that this pool holds.
+  size_t MaxSimultaneousAllocations = 0;
+
+  // Pointer to the pool of guarded slots. Note that this points to the start of
+  // the pool (which is a guard page), not a pointer to the first guarded page.
+  uintptr_t GuardedPagePool = 0;
+  uintptr_t GuardedPagePoolEnd = 0;
+
+  // Cached page size for this system in bytes.
+  size_t PageSize = 0;
+
+  // The type and address of an internally-detected failure. For INVALID_FREE
+  // and DOUBLE_FREE, these errors are detected in GWP-ASan, which will set
+  // these values and terminate the process.
+  Error FailureType = Error::UNKNOWN;
+  uintptr_t FailureAddress = 0;
+};
+
 // This class is the primary implementation of the allocator portion of GWP-
 // ASan. It is the sole owner of the pool of sequentially allocated guarded
----------------
This is moved just down below in this file.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:78
+
+// This holds the state that's shared between the GWP-ASan allocator and the
+// crash handler. This, in conjunction with the Metadata array, forms the entire
----------------
These functions and members are pulled out of `GuardedPoolAllocator`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:149
 
-  // Returns the largest allocation that is supported by this pool. Any
-  // allocations larger than this should go to the regular system allocator.
----------------
Moved into `AllocatorState`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:153
-
-  // Dumps an error report (including allocation and deallocation stack traces).
-  // An optional error may be provided if the caller knows what the error is
----------------
Removed in favour of new crash handler API.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:194
 
-  // Install the SIGSEGV crash handler for printing use-after-free and heap-
-  // buffer-{under|over}flow exceptions. This is platform specific as even
----------------
Installation of segv is now in `optional/crash_handler.h`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:202
-
-  // Returns the index of the slot that this pointer resides in. If the pointer
-  // is not owned by this pool, the result is undefined.
----------------
Functions moved into `AllocatorState`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:213
 
-  // Returns the address of the page that this pointer resides in.
-  uintptr_t getPageAddr(uintptr_t Ptr) const;
----------------
Functions moved into `AllocatorState`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:235
 
-  // Returns the diagnosis for an unknown error. If the diagnosis is not
-  // Error::INVALID_FREE or Error::UNKNOWN, the metadata for the slot
----------------
Deprecated in favour of new crash handler.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:247
 
-  // Cached page size for this system in bytes.
-  size_t PageSize = 0;
----------------
Members (and below) moved into `AllocatorState`.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:274
 
-  // Printf function supplied by the implementing allocator. We can't (in
-  // general) use printf() from the cstdlib as it may malloc(), causing infinite
----------------
We now don't store a `Printf` or `PrintBacktrace` function, as these are the crash handler's responsibility (and GPA no longer is responsible for handling crashes).


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler.h:17
+namespace crash_handler {
+// ================================ Requirements ===============================
+// This function must be provided by the supporting allocator only when this
----------------
Moved from `options.h`. Now `Printf_t` and `PrintBacktrace_t` are correctly only used by the inbuilt crash handler, and not managed by `GPA`.


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler.h:62
+
+// Install the SIGSEGV crash handler for printing use-after-free and heap-
+// buffer-{under|over}flow exceptions if the user asked for it. This is platform
----------------
This is now the entry point to install the internal (segv-based) crash handler, and the allocator is responsible for inspecting `Options.installSignalHandlers` and call this function if true.

The allocator is responsible for providing us with the proper printing/backtrace functions, but we also provide multiple implementations that can be used.


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:34
+
+static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
+  if (GPAForSignalHandler) {
----------------
Moved from `guarded_pool_allocator.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:60
+
+struct ScopedEndOfReportDecorator {
+  ScopedEndOfReportDecorator(gwp_asan::crash_handler::Printf_t Printf)
----------------
Moved from `guarded_pool_allocator.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:68
+// Prints the provided error and metadata information.
+void printHeader(Error E, uintptr_t AccessPtr,
+                 const gwp_asan::AllocatorState *State,
----------------
Moved from `guarded_pool_allocator.cpp` (with minor modifications to use new API instead of old).


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:116
+
+void defaultPrintStackTrace(uintptr_t *Trace, size_t TraceLength,
+                            gwp_asan::crash_handler::Printf_t Printf) {
----------------
Moved from `guarded_pool_allocator.cpp` (with minor modifications to use new API instead of old).


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:135
+
+void installSignalHandlers(gwp_asan::GuardedPoolAllocator *GPA, Printf_t Printf,
+                           PrintBacktrace_t PrintBacktrace,
----------------
Moved from `guarded_pool_allocator.cpp` (with minor modifications to use new API instead of old).


================
Comment at: compiler-rt/lib/gwp_asan/optional/crash_handler_posix.cpp:157
+
+void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
+                const gwp_asan::AllocationMetadata *Metadata,
----------------
Moved from `guarded_pool_allocator.cpp` (with minor modifications to use new API instead of old).


================
Comment at: compiler-rt/lib/gwp_asan/options.h:18
-// ================================ Requirements ===============================
-// This function is required to be implemented by the supporting allocator. The
-// sanitizer::Printf() function can be simply used here.
----------------
Moved to `crash_handler_api.h`.


================
Comment at: compiler-rt/lib/gwp_asan/options.h:53
 
-// ================================ Requirements ===============================
-// This function is optional for the supporting allocator, but one of the two
----------------
Moved to `crash_handler_api.h`.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:41
   if (Ptr == MAP_FAILED) {
-    Printf("Failed to map guarded pool allocator memory, errno: %d\n", errno);
-    Printf("  mmap(nullptr, %zu, ...) failed.\n", Size);
-    exit(EXIT_FAILURE);
+    assert(false && "Failed to map guarded pool allocator memory");
+    __builtin_trap();
----------------
No more `Printf` in GPA, so assert and trap instread.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:94
-
-static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
-  gwp_asan::GuardedPoolAllocator::reportError(
----------------
Moved to `crash_handler_posix.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/platform_specific/guarded_pool_allocator_posix.cpp:125
 
-void GuardedPoolAllocator::installSignalHandlers() {
-  struct sigaction Action;
----------------
Moved to `crash_handler_posix.cpp`.


================
Comment at: compiler-rt/lib/gwp_asan/tests/crash_handler_api.cpp:1
+//===-- crash_handler_api.cpp -----------------------------------*- C++ -*-===//
+//
----------------
New tests for the crash handler API.


================
Comment at: compiler-rt/lib/scudo/standalone/CMakeLists.txt:116
+  list(APPEND SCUDO_OBJECT_LIBS
+       RTGwpAsan RTGwpAsanBacktraceLibc RTGwpAsanCrashHandler)
   list(APPEND SCUDO_CFLAGS -DGWP_ASAN_HOOKS)
----------------
Note for @cryptoad: This is the libc backtrace and the GWP-ASan internal (segv) crash handler, no dependencies on compiler-rt here :)


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:173
     Opt.InstallForkHandlers = false;
-    Opt.Printf = Printf;
+    Opt.Backtrace = gwp_asan::options::getBacktraceFunction();
     GuardedAlloc.init(Opt);
----------------
Backtrace function comes from libc. Not sure if this WAI on Fuchsia, but we don't support GWP-ASan there so /shrug/.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73557