[GWP-ASan] Split the unwinder into segv/non-segv.

[hctim]

Summary

Splits the unwinder into a non-segv (for allocation/deallocation traces) and a
segv unwinder. This ensures that implementations can select an accurate, slower
unwinder in the segv handler (if they choose to use the GWP-ASan provided one).
This is important as fast frame-pointer unwinders (like the sanitizer unwinder)
don't like unwinding through signal handlers.

[hctim]

Correct diff.

[hctim]

+@cryptoad as this touches non-standalone scudo, but to a very limited degree.

[hctim]

• Remove vestigal changes to sanitizer_common.

[hctim]

• And patch in the changes to scudo/standalone.

(last change, promise)

[morehouse]

compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp

30

I'm probably missing something here... Since this is the same implementation as Backtrace, the whole change seems pointless?

[hctim]

compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp

30

The libc unwinder doesn't have the problems as it's DWARF based.

Scudo prebuilts out of the LLVM tree use this unwinder (so no problems), but Scudo when built from source on other codebases gives us the flexibility to drop in a faster, frame-pointer based unwinder - where this change is necessary. Or, when using the sanitizer unwinder, this change is also necessary to fallback from the FP unwinder to a DWARF unwinder in the segv handler.

[morehouse]

compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp

71

Very similar to Backtrace. Let's factor out a helper function that does fast unwind only when Context == nullptr.

[hctim]

• Refactor the backtrace code into a common core.

compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp

71

Done.

[hans]

After this change, two gwp-asan tests started failing:

GwpAsan-Unittest :: ./GwpAsan-x86_64-Test/BacktraceGuardedPoolAllocator.DoubleFree
GwpAsan-Unittest :: ./GwpAsan-x86_64-Test/BacktraceGuardedPoolAllocator.UseAfterFree

It seems variable names are not symbolized (but the backtrace and function names look fine), and so the death test expectations don't match.

This seems to happen in builds configured with -DCOMPILER_RT_BUILD_BUILTINS=OFF

Can you please take a look? Full error message and reproducer here: https://bugs.chromium.org/p/chromium/issues/detail?id=1107769

I've reverted in ab6263c9258ccd0e3c9cb4f675979f22cfba6131 in the meantime.

compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp

 
 namespace {
 size_t Backtrace(uintptr_t *TraceBuffer, size_t Size) {
   static_assert(sizeof(uintptr_t) == sizeof(void *), "uintptr_t is not void*");
 
   return backtrace(reinterpret_cast<void **>(TraceBuffer), Size);
 }
 
+// We don't need any custom handling for the Segv backtrace - the libc unwinder
+// has no problems with unwinding through a signal handler. Force inlining here
+// to avoid the additional frame.
+GWP_ASAN_ALWAYS_INLINE size_t SegvBacktrace(uintptr_t *TraceBuffer, size_t Size,
+                                            void * /*Context*/) {
+  return Backtrace(TraceBuffer, Size);
+}
+
 static void PrintBacktrace(uintptr_t *Trace, size_t TraceLength,
                            gwp_asan::crash_handler::Printf_t Printf) {
   if (TraceLength == 0) {
     Printf("  <not found (does your allocator support backtracing?)>\n\n");
     return;
   }
 
   char **BacktraceSymbols =

 
 namespace gwp_asan {
 namespace options {
 Backtrace_t getBacktraceFunction() { return Backtrace; }
 crash_handler::PrintBacktrace_t getPrintBacktraceFunction() {
   return PrintBacktrace;
 }
 } // namespace options
+
+namespace crash_handler {
+SegvBacktrace_t getSegvBacktraceFunction() { return SegvBacktrace; }
+} // namespace crash_handler
 } // namespace gwp_asan

compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp

 #include "sanitizer_common/sanitizer_flag_parser.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_stacktrace.h"
 
 void __sanitizer::BufferedStackTrace::UnwindImpl(uptr pc, uptr bp,
                                                  void *context,
                                                  bool request_fast,
                                                  u32 max_depth) {
-  if (!StackTrace::WillUseFastUnwind(request_fast)) {
-    return Unwind(max_depth, pc, bp, context, 0, 0, request_fast);
-  }
-  Unwind(max_depth, pc, 0, context, 0, 0, false);
+  if (!StackTrace::WillUseFastUnwind(request_fast))
+    return Unwind(max_depth, pc, 0, context, 0, 0, false);
+
+  uptr top = 0;
+  uptr bottom = 0;
+  GetThreadStackTopAndBottom(/*at_initialization*/ false, &top, &bottom);
+
+  return Unwind(max_depth, pc, bp, context, top, bottom, request_fast);
 }
 
 namespace {
-size_t Backtrace(uintptr_t *TraceBuffer, size_t Size) {
+size_t BacktraceCommon(uintptr_t *TraceBuffer, size_t Size, void *Context) {
+  // Use the slow sanitizer unwinder in the segv handler. Fast frame pointer
+  // unwinders can end up dropping frames because the kernel sigreturn() frame's
+  // return address is the return address at time of fault. This has the result
+  // of never actually capturing the PC where the signal was raised.
+  bool UseFastUnwind = (Context == nullptr);
+
   __sanitizer::BufferedStackTrace Trace;
   Trace.Reset();
   if (Size > __sanitizer::kStackTraceMax)
     Size = __sanitizer::kStackTraceMax;
 
   Trace.Unwind((__sanitizer::uptr)__builtin_return_address(0),
-               (__sanitizer::uptr)__builtin_frame_address(0),
-               /* ucontext */ nullptr,
-               /* fast unwind */ true, Size - 1);
+               (__sanitizer::uptr)__builtin_frame_address(0), Context,
+               UseFastUnwind, Size - 1);
 
   memcpy(TraceBuffer, Trace.trace, Trace.size * sizeof(uintptr_t));
   return Trace.size;
 }
 
+size_t Backtrace(uintptr_t *TraceBuffer, size_t Size) {
+  return BacktraceCommon(TraceBuffer, Size, nullptr);
+}
+
+size_t SegvBacktrace(uintptr_t *TraceBuffer, size_t Size, void *Context) {
+  return BacktraceCommon(TraceBuffer, Size, Context);
+}
+
 static void PrintBacktrace(uintptr_t *Trace, size_t TraceLength,
                            gwp_asan::crash_handler::Printf_t Printf) {
   __sanitizer::StackTrace StackTrace;
   StackTrace.trace = reinterpret_cast<__sanitizer::uptr *>(Trace);
   StackTrace.size = TraceLength;
 
   if (StackTrace.size == 0) {
     Printf("  <unknown (does your allocator support backtracing?)>\n\n");
     return;
   }
 
   StackTrace.Print();
 }
 } // anonymous namespace
 
 namespace gwp_asan {
Backtrace_t getBacktraceFunction() {
   __sanitizer::SetCommonFlagsDefaults();
   __sanitizer::InitializeCommonFlags();
   return Backtrace;
 }
 crash_handler::PrintBacktrace_t getPrintBacktraceFunction() {
   return PrintBacktrace;
 }
 } // namespace options
+
+namespace crash_handler {
+SegvBacktrace_t getSegvBacktraceFunction() { return SegvBacktrace; }
+} // namespace crash_handler
 } // namespace gwp_asan

compiler-rt/lib/gwp_asan/optional/segv_handler.h

 typedef void (*PrintBacktrace_t)(uintptr_t *TraceBuffer, size_t TraceLength,
                                  Printf_t Print);
 
 // Returns a function pointer to a basic PrintBacktrace implementation. This
 // implementation simply prints the stack trace in a human readable fashion
 // without any symbolization.
 PrintBacktrace_t getBasicPrintBacktraceFunction();
 
+// Returns a function pointer to a backtrace function that's suitable for
+// unwinding through a signal handler. This is important primarily for frame-
+// pointer based unwinders, DWARF or other unwinders can simply provide the
+// normal backtrace function as the implementation here. On POSIX, SignalContext
+// should be the `ucontext_t` from the signal handler.
+typedef size_t (*SegvBacktrace_t)(uintptr_t *TraceBuffer, size_t Size,
+                                  void *SignalContext);
+SegvBacktrace_t getSegvBacktraceFunction();
+
 // 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
 // specific as even though POSIX and Windows both support registering handlers
 // through signal(), we have to use platform-specific signal handlers to obtain
 // the address that caused the SIGSEGV exception. GPA->init() must be called
 // before this function.
 void installSignalHandlers(gwp_asan::GuardedPoolAllocator *GPA, Printf_t Printf,
                            PrintBacktrace_t PrintBacktrace,
-                           options::Backtrace_t Backtrace);
+                           SegvBacktrace_t SegvBacktrace);
 
 void uninstallSignalHandlers();
 
 void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
                 const gwp_asan::AllocationMetadata *Metadata,
-                options::Backtrace_t Backtrace, Printf_t Printf,
-                PrintBacktrace_t PrintBacktrace);
+                SegvBacktrace_t SegvBacktrace, Printf_t Printf,
+                PrintBacktrace_t PrintBacktrace, void *Context);
 } // namespace crash_handler
 } // namespace gwp_asan
 
 #endif // GWP_ASAN_OPTIONAL_CRASH_HANDLER_H_

compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp

 #include <stdio.h>
 
 namespace {
 using gwp_asan::AllocationMetadata;
 using gwp_asan::Error;
 using gwp_asan::GuardedPoolAllocator;
 using gwp_asan::crash_handler::PrintBacktrace_t;
 using gwp_asan::crash_handler::Printf_t;
-using gwp_asan::options::Backtrace_t;
+using gwp_asan::crash_handler::SegvBacktrace_t;
 
 struct sigaction PreviousHandler;
 bool SignalHandlerInstalled;
 gwp_asan::GuardedPoolAllocator *GPAForSignalHandler;
 Printf_t PrintfForSignalHandler;
 PrintBacktrace_t PrintBacktraceForSignalHandler;
-Backtrace_t BacktraceForSignalHandler;
+SegvBacktrace_t BacktraceForSignalHandler;
 
 static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
   if (GPAForSignalHandler) {
     GPAForSignalHandler->stop();
 
     gwp_asan::crash_handler::dumpReport(
         reinterpret_cast<uintptr_t>(info->si_addr),
         GPAForSignalHandler->getAllocatorState(),
         GPAForSignalHandler->getMetadataRegion(), BacktraceForSignalHandler,
-        PrintfForSignalHandler, PrintBacktraceForSignalHandler);
+        PrintfForSignalHandler, PrintBacktraceForSignalHandler, ucontext);
   }
 
   // Process any previous handlers.
   if (PreviousHandler.sa_flags & SA_SIGINFO) {
     PreviousHandler.sa_sigaction(sig, info, ucontext);
   } else if (PreviousHandler.sa_handler == SIG_DFL) {
     // If the previous handler was the default handler, cause a core dump.
     signal(SIGSEGV, SIG_DFL);

 namespace gwp_asan {
 namespace crash_handler {
 PrintBacktrace_t getBasicPrintBacktraceFunction() {
   return defaultPrintStackTrace;
 }
 
 void installSignalHandlers(gwp_asan::GuardedPoolAllocator *GPA, Printf_t Printf,
                            PrintBacktrace_t PrintBacktrace,
-                           options::Backtrace_t Backtrace) {
+                           SegvBacktrace_t SegvBacktrace) {
   GPAForSignalHandler = GPA;
   PrintfForSignalHandler = Printf;
   PrintBacktraceForSignalHandler = PrintBacktrace;
-  BacktraceForSignalHandler = Backtrace;
+  BacktraceForSignalHandler = SegvBacktrace;
 
   struct sigaction Action;
   Action.sa_sigaction = sigSegvHandler;
   Action.sa_flags = SA_SIGINFO;
   sigaction(SIGSEGV, &Action, &PreviousHandler);
   SignalHandlerInstalled = true;
 }
 
 void uninstallSignalHandlers() {
   if (SignalHandlerInstalled) {
     sigaction(SIGSEGV, &PreviousHandler, nullptr);
     SignalHandlerInstalled = false;
   }
 }
 
 void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
                 const gwp_asan::AllocationMetadata *Metadata,
-                options::Backtrace_t Backtrace, Printf_t Printf,
-                PrintBacktrace_t PrintBacktrace) {
+                SegvBacktrace_t SegvBacktrace, Printf_t Printf,
+                PrintBacktrace_t PrintBacktrace, void *Context) {
   assert(State && "dumpReport missing Allocator State.");
   assert(Metadata && "dumpReport missing Metadata.");
   assert(Printf && "dumpReport missing Printf.");
 
   if (!__gwp_asan_error_is_mine(State, ErrorPtr))
     return;
 
   Printf("*** GWP-ASan detected a memory error ***\n");
const gwp_asan::AllocationMetadata *AllocMeta =
       __gwp_asan_get_metadata(State, Metadata, ErrorPtr);
 
   // Print the error header.
   printHeader(E, ErrorPtr, AllocMeta, Printf);
 
   // Print the fault backtrace.
   static constexpr unsigned kMaximumStackFramesForCrashTrace = 512;
   uintptr_t Trace[kMaximumStackFramesForCrashTrace];
-  size_t TraceLength = Backtrace(Trace, kMaximumStackFramesForCrashTrace);
+  size_t TraceLength =
+      SegvBacktrace(Trace, kMaximumStackFramesForCrashTrace, Context);
 
   PrintBacktrace(Trace, TraceLength, Printf);
 
   if (AllocMeta == nullptr)
     return;
 
   // Maybe print the deallocation trace.
   if (__gwp_asan_is_deallocated(AllocMeta)) {

compiler-rt/lib/gwp_asan/tests/CMakeLists.txt

 include(CompilerRTCompile)
 
 set(GWP_ASAN_UNITTEST_CFLAGS
   ${COMPILER_RT_UNITTEST_CFLAGS}
   ${COMPILER_RT_GTEST_CFLAGS}
   -I${COMPILER_RT_SOURCE_DIR}/lib/
   -O2
-  -g)
+  -g
+  -fno-omit-frame-pointer)
 
 file(GLOB GWP_ASAN_HEADERS ../*.h)
 set(GWP_ASAN_UNITTESTS
   optional/printf_sanitizer_common.cpp
   alignment.cpp
   backtrace.cpp
   basic.cpp
   compression.cpp

compiler-rt/lib/gwp_asan/tests/harness.h

void SetUp() override {
     Opts.setDefaults();
 
     Opts.Backtrace = gwp_asan::options::getBacktraceFunction();
     Opts.InstallForkHandlers = gwp_asan::test::OnlyOnce();
     GPA.init(Opts);
 
     gwp_asan::crash_handler::installSignalHandlers(
         &GPA, gwp_asan::test::getPrintfFunction(),
-        gwp_asan::options::getPrintBacktraceFunction(), Opts.Backtrace);
+        gwp_asan::options::getPrintBacktraceFunction(),
+        gwp_asan::crash_handler::getSegvBacktraceFunction());
   }
 
   void TearDown() override {
     GPA.uninitTestOnly();
     gwp_asan::crash_handler::uninstallSignalHandlers();
   }
 
 protected:
   gwp_asan::GuardedPoolAllocator GPA;
 };
 
 #endif // GWP_ASAN_TESTS_HARNESS_H_

compiler-rt/lib/scudo/scudo_allocator.cpp

 #include "sanitizer_common/sanitizer_allocator_checks.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_quarantine.h"
 
 #ifdef GWP_ASAN_HOOKS
 # include "gwp_asan/guarded_pool_allocator.h"
 # include "gwp_asan/optional/backtrace.h"
 # include "gwp_asan/optional/options_parser.h"
+#include "gwp_asan/optional/segv_handler.h"
 #endif // GWP_ASAN_HOOKS
 
 #include <errno.h>
 #include <string.h>
 
 namespace __scudo {
 
 // Global static cookie, initialized at start-up.
#ifdef GWP_ASAN_HOOKS
   gwp_asan::options::initOptions();
   gwp_asan::options::Options &Opts = gwp_asan::options::getOptions();
   Opts.Backtrace = gwp_asan::options::getBacktraceFunction();
   GuardedAlloc.init(Opts);
 
   if (Opts.InstallSignalHandlers)
     gwp_asan::crash_handler::installSignalHandlers(
         &GuardedAlloc, __sanitizer::Printf,
-        gwp_asan::options::getPrintBacktraceFunction(), Opts.Backtrace);
+        gwp_asan::options::getPrintBacktraceFunction(),
+        gwp_asan::crash_handler::getSegvBacktraceFunction());
 #endif // GWP_ASAN_HOOKS
 }
 
 void ScudoTSD::init() {
   getBackend().initCache(&Cache);
   memset(QuarantineCachePlaceHolder, 0, sizeof(QuarantineCachePlaceHolder));
 }
 

compiler-rt/lib/scudo/standalone/combined.h

#ifdef GWP_ASAN_HOOKS
     // handler.
     Opt.InstallForkHandlers = false;
     Opt.Backtrace = gwp_asan::options::getBacktraceFunction();
     GuardedAlloc.init(Opt);
 
     if (Opt.InstallSignalHandlers)
       gwp_asan::crash_handler::installSignalHandlers(
           &GuardedAlloc, Printf, gwp_asan::options::getPrintBacktraceFunction(),
-          Opt.Backtrace);
+          gwp_asan::crash_handler::getSegvBacktraceFunction());
 #endif // GWP_ASAN_HOOKS
   }
 
   void reset() { memset(this, 0, sizeof(*this)); }
 
   void unmapTestOnly() {
     TSDRegistry.unmapTestOnly();
     Primary.unmapTestOnly();

compiler-rt/test/gwp_asan/backtrace.c

+// REQUIRES: gwp_asan
+// RUN: %clang_gwp_asan %s -g -o %t
+// RUN: %expect_crash %t 2>&1 | FileCheck %s
+
+#include <stdlib.h>
+
+__attribute__((noinline)) void *allocate_mem() { return malloc(1); }
+
+__attribute__((noinline)) void free_mem(void *ptr) { free(ptr); }
+
+__attribute__((noinline)) void touch_mem(void *ptr) {
+  volatile char sink = *((volatile char *)ptr);
+}
+
+// CHECK: Use After Free
+// CHECK: touch_mem
+// CHECK: was deallocated
+// CHECK: free_mem
+// CHECK: was allocated
+// CHECK: allocate_mem
+
+int main() {
+  for (unsigned i = 0; i < 0x10000; ++i) {
+    void *ptr = allocate_mem();
+    free_mem(ptr);
+    touch_mem(ptr);
+  }
+  return 0;
+}