From e47918b80abf61c9d013012b2df1769e0c0b18d2 Mon Sep 17 00:00:00 2001 From: Justin Cohen Date: Wed, 11 Aug 2021 12:40:54 -0400 Subject: [PATCH] ios: Move and update ObjcExceptionPreprocessor. More cleanly integration the ObjExceptionPreprocessor with the Crashpad client and in process handler, to record bought 'caught' and 'uncaught' NSExceptions. Bug: crashpad: 31 Change-Id: I77a77ca6d893cdc74da476c1888d9bcb338339d8 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2920851 Commit-Queue: Justin Cohen Reviewed-by: Mark Mentovai --- client/BUILD.gn | 7 +- client/crashpad_client_ios.cc | 28 ++- .../ios_handler}/exception_processor.h | 29 ++- .../ios_handler}/exception_processor.mm | 179 ++++++++++++++---- .../ios_handler}/exception_processor_test.mm | 0 util/BUILD.gn | 3 - 6 files changed, 201 insertions(+), 45 deletions(-) rename {util/ios => client/ios_handler}/exception_processor.h (60%) rename {util/ios => client/ios_handler}/exception_processor.mm (67%) rename {util/ios => client/ios_handler}/exception_processor_test.mm (100%) diff --git a/client/BUILD.gn b/client/BUILD.gn index 5cf5532e..aa1eaaa4 100644 --- a/client/BUILD.gn +++ b/client/BUILD.gn @@ -33,6 +33,8 @@ crashpad_static_library("client") { if (crashpad_is_ios) { sources += [ "crashpad_client_ios.cc", + "ios_handler/exception_processor.h", + "ios_handler/exception_processor.mm", "simulate_crash_ios.h", ] } @@ -160,7 +162,10 @@ source_set("client_test") { } if (crashpad_is_ios) { - sources += [ "crashpad_client_ios_test.mm" ] + sources += [ + "crashpad_client_ios_test.mm", + "ios_handler/exception_processor_test.mm", + ] sources -= [ "annotation_list_test.cc", "annotation_test.cc", diff --git a/client/crashpad_client_ios.cc b/client/crashpad_client_ios.cc index a8ef224f..f53a6d5c 100644 --- a/client/crashpad_client_ios.cc +++ b/client/crashpad_client_ios.cc @@ -22,7 +22,7 @@ #include "base/logging.h" #include "base/mac/mach_logging.h" #include "base/mac/scoped_mach_port.h" -#include "util/ios/exception_processor.h" +#include "client/ios_handler/exception_processor.h" #include "util/ios/ios_system_data_collector.h" #include "util/mach/exc_server_variants.h" #include "util/mach/exception_ports.h" @@ -38,7 +38,9 @@ namespace crashpad { namespace { // A base class for signal handler and Mach exception server. -class CrashHandler : public Thread, public UniversalMachExcServer::Interface { +class CrashHandler : public Thread, + public UniversalMachExcServer::Interface, + public ObjcExceptionDelegate { public: static CrashHandler* Get() { static CrashHandler* instance = new CrashHandler(); @@ -188,6 +190,26 @@ class CrashHandler : public Thread, public UniversalMachExcServer::Interface { Signals::RestoreHandlerAndReraiseSignalOnReturn(siginfo, &old_action_); } + void HandleUncaughtNSException(const uint64_t* frames, + const size_t num_frames) override { + // TODO(justincohen): Call into in_process_handler. + + // After uncaught exceptions are reported, the system immediately triggers a + // call to std::terminate()/abort(). Remove the abort handler so a second + // dump isn't generated. + CHECK(Signals::InstallDefaultHandler(SIGABRT)); + } + + void HandleUncaughtNSExceptionWithContext( + NativeCPUContext* context) override { + // TODO(justincohen): Call into in_process_handler. + + // After uncaught exceptions are reported, the system immediately triggers a + // call to std::terminate()/abort(). Remove the abort handler so a second + // dump isn't generated. + CHECK(Signals::InstallDefaultHandler(SIGABRT)); + } + base::mac::ScopedMachReceiveRight exception_port_; ExceptionPorts::ExceptionHandlerVector original_handlers_; struct sigaction old_action_ = {}; @@ -208,10 +230,10 @@ void CrashpadClient::StartCrashpadInProcessHandler( const base::FilePath& database, const std::string& url, const std::map& annotations) { - InstallObjcExceptionPreprocessor(); CrashHandler* crash_handler = CrashHandler::Get(); DCHECK(crash_handler); + InstallObjcExceptionPreprocessor(crash_handler); crash_handler->Initialize(); } diff --git a/util/ios/exception_processor.h b/client/ios_handler/exception_processor.h similarity index 60% rename from util/ios/exception_processor.h rename to client/ios_handler/exception_processor.h index 76a8c8b3..7318e214 100644 --- a/util/ios/exception_processor.h +++ b/client/ios_handler/exception_processor.h @@ -15,8 +15,35 @@ #ifndef CRASHPAD_UTIL_IOS_EXCEPTION_PROCESSOR_H_ #define CRASHPAD_UTIL_IOS_EXCEPTION_PROCESSOR_H_ +#include + +#include "util/misc/capture_context.h" + namespace crashpad { +//! \brief An interface for notifying the CrashpadClient of NSExceptions. +class ObjcExceptionDelegate { + public: + //! \brief The exception processor detected an exception as it was thrown and + //! captured the cpu context. + //! + //! \param context The cpu context of the thread throwing an exception. + virtual void HandleUncaughtNSExceptionWithContext( + NativeCPUContext* context) = 0; + + //! \brief The exception processor did not detect the exception as it was + //! thrown, and instead caught the exception via the + //! NSUncaughtExceptionHandler. + //! + //! \param frames An array of call stack frame addresses. + //! \param num_frames The number of frames in |frames|. + virtual void HandleUncaughtNSException(const uint64_t* frames, + const size_t num_frames) = 0; + + protected: + ~ObjcExceptionDelegate() {} +}; + //! \brief Installs the Objective-C exception preprocessor. //! //! When code raises an Objective-C exception, unwind the stack looking for @@ -30,7 +57,7 @@ namespace crashpad { //! //! This should be installed at the same time the CrashpadClient installs the //! signal handler. It should only be installed once. -void InstallObjcExceptionPreprocessor(); +void InstallObjcExceptionPreprocessor(ObjcExceptionDelegate* delegate); } // namespace crashpad diff --git a/util/ios/exception_processor.mm b/client/ios_handler/exception_processor.mm similarity index 67% rename from util/ios/exception_processor.mm rename to client/ios_handler/exception_processor.mm index 2b2857e0..d8c94e54 100644 --- a/util/ios/exception_processor.mm +++ b/client/ios_handler/exception_processor.mm @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "util/ios/exception_processor.h" +#include "client/ios_handler/exception_processor.h" #include #import @@ -35,10 +35,15 @@ #include #include "base/bit_cast.h" +#include "base/format_macros.h" #include "base/logging.h" #include "base/memory/free_deleter.h" +#include "base/numerics/safe_conversions.h" +#include "base/strings/stringprintf.h" #include "base/strings/sys_string_conversions.h" #include "build/build_config.h" +#include "client/annotation.h" +#include "client/simulate_crash_ios.h" namespace { @@ -80,20 +85,101 @@ struct __cxa_exception { _Unwind_Exception unwindHeader; }; -objc_exception_preprocessor g_next_preprocessor; -bool g_exception_preprocessor_installed; +int LoggingUnwStep(unw_cursor_t* cursor) { + int rv = unw_step(cursor); + if (rv < 0) { + LOG(ERROR) << "unw_step: " << rv; + } + return rv; +} -void TerminatingFromUncaughtNSException(id exception, const char* sinkhole) { - // TODO(justincohen): This is incomplete, as the signal handler will not have - // access to the exception name and reason. Pass that along somehow here. - NSString* exception_message_ns = [NSString - stringWithFormat:@"%@: %@", [exception name], [exception reason]]; - std::string exception_message = base::SysNSStringToUTF8(exception_message_ns); - LOG(INFO) << "Terminating from Objective-C exception: " << exception_message - << " with sinkhole: " << sinkhole; - // TODO(justincohen): This is temporary, as crashpad can capture this - // exception directly instead. - std::terminate(); +std::string FormatStackTrace(const std::vector& addresses, + size_t max_length) { + std::string stack_string; + for (uint64_t address : addresses) { + std::string address_string = base::StringPrintf("0x%" PRIx64, address); + if (stack_string.size() + address_string.size() > max_length) + break; + stack_string += address_string + " "; + } + + if (!stack_string.empty() && stack_string.back() == ' ') { + stack_string.resize(stack_string.size() - 1); + } + + return stack_string; +} + +std::string GetTraceString() { + std::vector addresses; + unw_context_t context; + unw_getcontext(&context); + unw_cursor_t cursor; + unw_init_local(&cursor, &context); + while (LoggingUnwStep(&cursor) > 0) { + unw_word_t ip = 0; + unw_get_reg(&cursor, UNW_REG_IP, &ip); + addresses.push_back(ip); + } + return FormatStackTrace(addresses, 1024); +} + +crashpad::ObjcExceptionDelegate* g_exception_delegate; +objc_exception_preprocessor g_next_preprocessor; +NSUncaughtExceptionHandler* g_next_uncaught_exception_handler; + +static void SetNSExceptionAnnotations(id exception, + std::string& name, + std::string& reason) { + name = base::SysNSStringToUTF8([exception name]); + reason = base::SysNSStringToUTF8([exception reason]); + static crashpad::StringAnnotation<256> nameKey("exceptionName"); + nameKey.Set(name); + static crashpad::StringAnnotation<512> reasonKey("exceptionReason"); + reasonKey.Set(reason); +} + +static void ObjcUncaughtExceptionHandler(NSException* exception) { + std::string name, reason; + SetNSExceptionAnnotations(exception, name, reason); + NSArray* addressArray = [exception callStackReturnAddresses]; + if ([addressArray count] > 0) { + static crashpad::StringAnnotation<256> nameKey("UncaughtNSException"); + nameKey.Set("true"); + std::vector addresses; + NSArray* addressArray = [exception callStackReturnAddresses]; + for (NSNumber* address in addressArray) + addresses.push_back([address unsignedLongLongValue]); + g_exception_delegate->HandleUncaughtNSException(&addresses[0], + addresses.size()); + } else { + LOG(WARNING) << "Uncaught Objective-C exception name: " << name + << " reason: " << reason << " with no " + << " -callStackReturnAddresses."; + crashpad::NativeCPUContext cpu_context; + crashpad::CaptureContext(&cpu_context); + g_exception_delegate->HandleUncaughtNSExceptionWithContext(&cpu_context); + } +} + +// This function is used to make it clear to the crash processor that an +// uncaught NSException was recorded here. +static __attribute__((noinline)) id HANDLE_UNCAUGHT_NSEXCEPTION( + id exception, + const char* sinkhole) { + std::string name, reason; + SetNSExceptionAnnotations(exception, name, reason); + LOG(WARNING) << "Handling Objective-C exception name: " << name + << " reason: " << reason << " with sinkhole: " << sinkhole; + crashpad::NativeCPUContext cpu_context; + crashpad::CaptureContext(&cpu_context); + g_exception_delegate->HandleUncaughtNSExceptionWithContext(&cpu_context); + + // Remove the uncaught exception handler so we don't record this twice. + NSSetUncaughtExceptionHandler(g_next_uncaught_exception_handler); + g_next_uncaught_exception_handler = nullptr; + + return g_next_preprocessor ? g_next_preprocessor(exception) : exception; } // Returns true if |path| equals |sinkhole| on device. Simulator paths prepend @@ -113,15 +199,25 @@ bool ModulePathMatchesSinkhole(const char* path, const char* sinkhole) { #endif } -int LoggingUnwStep(unw_cursor_t* cursor) { - int rv = unw_step(cursor); - if (rv < 0) { - LOG(ERROR) << "unw_step: " << rv; - } - return rv; -} - id ObjcExceptionPreprocessor(id exception) { + static bool seen_first_exception; + + static crashpad::StringAnnotation<256> firstexception("firstexception"); + static crashpad::StringAnnotation<256> lastexception("lastexception"); + static crashpad::StringAnnotation<1024> firstexception_bt( + "firstexception_bt"); + static crashpad::StringAnnotation<1024> lastexception_bt("lastexception_bt"); + auto* key = seen_first_exception ? &lastexception : &firstexception; + auto* bt_key = seen_first_exception ? &lastexception_bt : &firstexception_bt; + NSString* value = [NSString + stringWithFormat:@"%@ reason %@", [exception name], [exception reason]]; + key->Set(base::SysNSStringToUTF8(value)); + + // This exception preprocessor runs prior to the one in libobjc, which sets + // the -[NSException callStackReturnAddresses]. + bt_key->Set(GetTraceString()); + seen_first_exception = true; + // Unwind the stack looking for any exception handlers. If an exception // handler is encountered, test to see if it is a function known to catch- // and-rethrow as a "top-level" exception handler. Various routines in @@ -229,10 +325,13 @@ id ObjcExceptionPreprocessor(id exception) { "CFRunLoopRunSpecific", "_CFXNotificationPost", "__NSFireDelayedPerform", + // If this exception is going to end up at EHFrame, record the uncaught + // exception instead. + "_ZN4base3mac15CallWithEHFrameEU13block_pointerFvvE", }; for (const char* sinkhole : kExceptionSymbolNameSinkholes) { if (strcmp(sinkhole, proc_name) == 0) { - TerminatingFromUncaughtNSException(exception, sinkhole); + return HANDLE_UNCAUGHT_NSEXCEPTION(exception, sinkhole); } } @@ -257,7 +356,7 @@ id ObjcExceptionPreprocessor(id exception) { 0) { for (const char* sinkhole : kExceptionLibraryPathSinkholes) { if (ModulePathMatchesSinkhole(dl_info.dli_fname, sinkhole)) { - TerminatingFromUncaughtNSException(exception, sinkhole); + return HANDLE_UNCAUGHT_NSEXCEPTION(exception, sinkhole); } } } @@ -305,7 +404,7 @@ id ObjcExceptionPreprocessor(id exception) { reinterpret_cast(gesture_environment_min_imp) && caller_frame_info.start_ip <= reinterpret_cast(gesture_environment_max_imp)) { - TerminatingFromUncaughtNSException(exception, + return HANDLE_UNCAUGHT_NSEXCEPTION(exception, "_UIGestureEnvironmentUpdate"); } } @@ -319,37 +418,43 @@ id ObjcExceptionPreprocessor(id exception) { // If no handler is found, __cxa_throw would call failed_throw and terminate. // See: // https://github.com/llvm/llvm-project/blob/c5d2746fbea7/libcxxabi/src/cxa_exception.cpp - // __cxa_throw. Instead, terminate via TerminatingFromUncaughtNSException so - // the exception name and reason are properly recorded. + // __cxa_throw. Instead, call HANDLE_UNCAUGHT_NSEXCEPTION so the exception + // name and reason are properly recorded. if (!handler_found) { - TerminatingFromUncaughtNSException(exception, "__cxa_throw"); + return HANDLE_UNCAUGHT_NSEXCEPTION(exception, "__cxa_throw"); } // Forward to the next preprocessor. - if (g_next_preprocessor) - return g_next_preprocessor(exception); - - return exception; + return g_next_preprocessor ? g_next_preprocessor(exception) : exception; } } // namespace namespace crashpad { -void InstallObjcExceptionPreprocessor() { - DCHECK(!g_exception_preprocessor_installed); +void InstallObjcExceptionPreprocessor(ObjcExceptionDelegate* delegate) { + DCHECK(!g_next_preprocessor); + // Preprocessor. g_next_preprocessor = objc_setExceptionPreprocessor(&ObjcExceptionPreprocessor); - g_exception_preprocessor_installed = true; + + // Uncaught processor. + g_exception_delegate = delegate; + g_next_uncaught_exception_handler = NSGetUncaughtExceptionHandler(); + NSSetUncaughtExceptionHandler(&ObjcUncaughtExceptionHandler); } void UninstallObjcExceptionPreprocessor() { - DCHECK(g_exception_preprocessor_installed); + DCHECK(g_next_preprocessor); objc_setExceptionPreprocessor(g_next_preprocessor); + g_exception_delegate = nullptr; + + NSSetUncaughtExceptionHandler(g_next_uncaught_exception_handler); + g_next_uncaught_exception_handler = nullptr; + g_next_preprocessor = nullptr; - g_exception_preprocessor_installed = false; } } // namespace crashpad diff --git a/util/ios/exception_processor_test.mm b/client/ios_handler/exception_processor_test.mm similarity index 100% rename from util/ios/exception_processor_test.mm rename to client/ios_handler/exception_processor_test.mm diff --git a/util/BUILD.gn b/util/BUILD.gn index dfb1a9a0..4f284140 100644 --- a/util/BUILD.gn +++ b/util/BUILD.gn @@ -378,8 +378,6 @@ crashpad_static_library("util") { if (crashpad_is_ios) { sources += [ - "ios/exception_processor.h", - "ios/exception_processor.mm", "ios/ios_intermediate_dump_data.cc", "ios/ios_intermediate_dump_data.h", "ios/ios_intermediate_dump_format.h", @@ -807,7 +805,6 @@ source_set("util_test") { if (crashpad_is_ios) { sources += [ - "ios/exception_processor_test.mm", "ios/ios_intermediate_dump_reader_test.cc", "ios/ios_intermediate_dump_writer_test.cc", "ios/scoped_vm_read_test.cc",