Revert 502f0cc0e38 "[GWP-ASan] Split the unwinder into segv/non-segv."

It was causing tests to fail in -DCOMPILER_RT_BUILD_BUILTINS=OFF builds:

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

see comment on the code review.

> 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.
>
> Reviewers: morehouse, cryptoad
>
> Reviewed By: morehouse, cryptoad
>
> Subscribers: cryptoad, mgorny, eugenis, pcc, #sanitizers
>
> Tags: #sanitizers
>
> Differential Revision: https://reviews.llvm.org/D83994

This reverts commit 502f0cc0e3889229e923e187f38dda91324ae139.

diff --git a/compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp b/compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp
index 92eb293dab499713d875877ce58a1a53cc913ad6..bb0aad224a14cb292d4fac71ee3993709cdc0aca 100644 (file)
--- a/compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp
+++ b/compiler-rt/lib/gwp_asan/optional/backtrace_linux_libc.cpp
@@ -23,14 +23,6 @@ size_t Backtrace(uintptr_t *TraceBuffer, size_t Size) {
   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) {
@@ -61,8 +53,4 @@ crash_handler::PrintBacktrace_t getPrintBacktraceFunction() {
   return PrintBacktrace;
 }
 } // namespace options
-
-namespace crash_handler {
-SegvBacktrace_t getSegvBacktraceFunction() { return SegvBacktrace; }
-} // namespace crash_handler
 } // namespace gwp_asan

diff --git a/compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp b/compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp
index a8083e4e64cb3181ae0d0068bc67ab850d07c369..3ac4b52bfc271f25e7b9f9a4d27f62664816838a 100644 (file)
--- a/compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp
+++ b/compiler-rt/lib/gwp_asan/optional/backtrace_sanitizer_common.cpp
@@ -22,45 +22,28 @@ 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, 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);
+  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);
 }
 
 namespace {
-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);
-
+size_t Backtrace(uintptr_t *TraceBuffer, size_t Size) {
   __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), Context,
-               UseFastUnwind, Size - 1);
+               (__sanitizer::uptr)__builtin_frame_address(0),
+               /* ucontext */ nullptr,
+               /* fast unwind */ true, 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;
@@ -94,8 +77,4 @@ crash_handler::PrintBacktrace_t getPrintBacktraceFunction() {
   return PrintBacktrace;
 }
 } // namespace options
-
-namespace crash_handler {
-SegvBacktrace_t getSegvBacktraceFunction() { return SegvBacktrace; }
-} // namespace crash_handler
 } // namespace gwp_asan

diff --git a/compiler-rt/lib/gwp_asan/optional/segv_handler.h b/compiler-rt/lib/gwp_asan/optional/segv_handler.h
index 0fed4f2e012e9c3284cda86604f7fc5f750270bd..10af15055e2a864e89c4e03f66ae4bcff9868d62 100644 (file)
--- a/compiler-rt/lib/gwp_asan/optional/segv_handler.h
+++ b/compiler-rt/lib/gwp_asan/optional/segv_handler.h
@@ -59,15 +59,6 @@ typedef void (*PrintBacktrace_t)(uintptr_t *TraceBuffer, size_t TraceLength,
 // 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
@@ -76,14 +67,14 @@ SegvBacktrace_t getSegvBacktraceFunction();
 // before this function.
 void installSignalHandlers(gwp_asan::GuardedPoolAllocator *GPA, Printf_t Printf,
                            PrintBacktrace_t PrintBacktrace,
-                           SegvBacktrace_t SegvBacktrace);
+                           options::Backtrace_t Backtrace);
 
 void uninstallSignalHandlers();
 
 void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
                 const gwp_asan::AllocationMetadata *Metadata,
-                SegvBacktrace_t SegvBacktrace, Printf_t Printf,
-                PrintBacktrace_t PrintBacktrace, void *Context);
+                options::Backtrace_t Backtrace, Printf_t Printf,
+                PrintBacktrace_t PrintBacktrace);
 } // namespace crash_handler
 } // namespace gwp_asan
 

diff --git a/compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp b/compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp
index 1bd7a606c21369a4dd3ae7aaa6f654012504a49b..22589b893604fa1e435f81ea3a1b28f2557ec149 100644 (file)
--- a/compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp
+++ b/compiler-rt/lib/gwp_asan/optional/segv_handler_posix.cpp
@@ -23,14 +23,14 @@ 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::crash_handler::SegvBacktrace_t;
+using gwp_asan::options::Backtrace_t;
 
 struct sigaction PreviousHandler;
 bool SignalHandlerInstalled;
 gwp_asan::GuardedPoolAllocator *GPAForSignalHandler;
 Printf_t PrintfForSignalHandler;
 PrintBacktrace_t PrintBacktraceForSignalHandler;
-SegvBacktrace_t BacktraceForSignalHandler;
+Backtrace_t BacktraceForSignalHandler;
 
 static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
   if (GPAForSignalHandler) {
@@ -40,7 +40,7 @@ static void sigSegvHandler(int sig, siginfo_t *info, void *ucontext) {
         reinterpret_cast<uintptr_t>(info->si_addr),
         GPAForSignalHandler->getAllocatorState(),
         GPAForSignalHandler->getMetadataRegion(), BacktraceForSignalHandler,
-        PrintfForSignalHandler, PrintBacktraceForSignalHandler, ucontext);
+        PrintfForSignalHandler, PrintBacktraceForSignalHandler);
   }
 
   // Process any previous handlers.
@@ -138,11 +138,11 @@ PrintBacktrace_t getBasicPrintBacktraceFunction() {
 
 void installSignalHandlers(gwp_asan::GuardedPoolAllocator *GPA, Printf_t Printf,
                            PrintBacktrace_t PrintBacktrace,
-                           SegvBacktrace_t SegvBacktrace) {
+                           options::Backtrace_t Backtrace) {
   GPAForSignalHandler = GPA;
   PrintfForSignalHandler = Printf;
   PrintBacktraceForSignalHandler = PrintBacktrace;
-  BacktraceForSignalHandler = SegvBacktrace;
+  BacktraceForSignalHandler = Backtrace;
 
   struct sigaction Action;
   Action.sa_sigaction = sigSegvHandler;
@@ -160,8 +160,8 @@ void uninstallSignalHandlers() {
 
 void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
                 const gwp_asan::AllocationMetadata *Metadata,
-                SegvBacktrace_t SegvBacktrace, Printf_t Printf,
-                PrintBacktrace_t PrintBacktrace, void *Context) {
+                options::Backtrace_t Backtrace, Printf_t Printf,
+                PrintBacktrace_t PrintBacktrace) {
   assert(State && "dumpReport missing Allocator State.");
   assert(Metadata && "dumpReport missing Metadata.");
   assert(Printf && "dumpReport missing Printf.");
@@ -194,8 +194,7 @@ void dumpReport(uintptr_t ErrorPtr, const gwp_asan::AllocatorState *State,
   // Print the fault backtrace.
   static constexpr unsigned kMaximumStackFramesForCrashTrace = 512;
   uintptr_t Trace[kMaximumStackFramesForCrashTrace];
-  size_t TraceLength =
-      SegvBacktrace(Trace, kMaximumStackFramesForCrashTrace, Context);
+  size_t TraceLength = Backtrace(Trace, kMaximumStackFramesForCrashTrace);
 
   PrintBacktrace(Trace, TraceLength, Printf);
 

diff --git a/compiler-rt/lib/gwp_asan/tests/CMakeLists.txt b/compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
index f88d90c19d5b917ceeb1a4baca02e394fb9ad622..feac23df9fe5d6c7ddc76129c8fd94729f7c83f9 100644 (file)
--- a/compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
+++ b/compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
@@ -5,8 +5,7 @@ set(GWP_ASAN_UNITTEST_CFLAGS
   ${COMPILER_RT_GTEST_CFLAGS}
   -I${COMPILER_RT_SOURCE_DIR}/lib/
   -O2
-  -g
-  -fno-omit-frame-pointer)
+  -g)
 
 file(GLOB GWP_ASAN_HEADERS ../*.h)
 set(GWP_ASAN_UNITTESTS

diff --git a/compiler-rt/lib/gwp_asan/tests/harness.h b/compiler-rt/lib/gwp_asan/tests/harness.h
index d303b2cfa647072703ad8f980bd1d9a23a2391a3..e47254e13c467dcf3152be0a69e50eb53514cc38 100644 (file)
--- a/compiler-rt/lib/gwp_asan/tests/harness.h
+++ b/compiler-rt/lib/gwp_asan/tests/harness.h
@@ -86,8 +86,7 @@ public:
 
     gwp_asan::crash_handler::installSignalHandlers(
         &GPA, gwp_asan::test::getPrintfFunction(),
-        gwp_asan::options::getPrintBacktraceFunction(),
-        gwp_asan::crash_handler::getSegvBacktraceFunction());
+        gwp_asan::options::getPrintBacktraceFunction(), Opts.Backtrace);
   }
 
   void TearDown() override {

diff --git a/compiler-rt/lib/scudo/scudo_allocator.cpp b/compiler-rt/lib/scudo/scudo_allocator.cpp
index 343f85a4ef88bf1e45e4944e57680a5d9d05939f..d9023c2f7ab64dbd5a5d1c681fa5aa0f69200512 100644 (file)
--- a/compiler-rt/lib/scudo/scudo_allocator.cpp
+++ b/compiler-rt/lib/scudo/scudo_allocator.cpp
@@ -29,7 +29,6 @@
 # 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>
@@ -680,8 +679,7 @@ void initScudo() {
   if (Opts.InstallSignalHandlers)
     gwp_asan::crash_handler::installSignalHandlers(
         &GuardedAlloc, __sanitizer::Printf,
-        gwp_asan::options::getPrintBacktraceFunction(),
-        gwp_asan::crash_handler::getSegvBacktraceFunction());
+        gwp_asan::options::getPrintBacktraceFunction(), Opts.Backtrace);
 #endif // GWP_ASAN_HOOKS
 }
 

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index ae085befc4f152922dc3747f94ddb8a8a24abf7d..3bb41eca88f72c00e40f83e24944cf6441c0baba 100644 (file)
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -192,7 +192,7 @@ public:
     if (Opt.InstallSignalHandlers)
       gwp_asan::crash_handler::installSignalHandlers(
           &GuardedAlloc, Printf, gwp_asan::options::getPrintBacktraceFunction(),
-          gwp_asan::crash_handler::getSegvBacktraceFunction());
+          Opt.Backtrace);
 #endif // GWP_ASAN_HOOKS
   }
 

diff --git a/compiler-rt/test/gwp_asan/backtrace.c b/compiler-rt/test/gwp_asan/backtrace.c
deleted file mode 100644 (file)
index 0ba32f8..0000000
--- a/compiler-rt/test/gwp_asan/backtrace.c
+++ /dev/null
@@ -1,29 +0,0 @@
-// 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;
-}