From e1347a740c051d9a44aa2dd0b9d11067a53e0447 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 2 Apr 2015 15:49:51 -0400 Subject: [PATCH] Handle EXC_RESOURCE and EXC_GUARD exceptions properly. These two exception types use all 64 bits of the code[0] field. The ExceptionSnapshot was unprepared to stuff this into a 32-bit field. To resolve the discrepancy, the more-significant data is taken from the high 32 bits of code[0]. No information is lost because the full code[0] is made available as part of the Codes() vector. BUG=crashpad:34 R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1050313003 --- client/crashpad_client_mac.cc | 3 +- compat/non_win/dbghelp.h | 16 ++-- handler/mac/crash_report_exception_handler.cc | 16 +++- snapshot/exception_snapshot.h | 13 ++- snapshot/mac/exception_snapshot_mac.cc | 93 +++++++++++++++++-- snapshot/mac/exception_snapshot_mac.h | 1 + snapshot/mac/process_snapshot_mac.cc | 2 + snapshot/mac/process_snapshot_mac.h | 3 +- 8 files changed, 123 insertions(+), 24 deletions(-) diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 2ac274d0..e97e58b1 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -188,7 +188,8 @@ bool CrashpadClient::UseHandler() { // xnu-2422.115.4/bsd/uxkern/ux_exception.c catch_mach_exception_raise(). If a // core-generating signal (triggered through this hardware mechanism or a // software mechanism such as abort() sending SIGABRT) is unhandled and the - // process exits, the exception becomes EXC_CRASH. See 10.9.5 + // process exits, or if the process is killed with SIGKILL for code-signing + // reasons, an EXC_CRASH exception will be sent. See 10.9.5 // xnu-2422.115.4/bsd/kern/kern_exit.c proc_prepareexit(). // // EXC_RESOURCE and EXC_GUARD do not become signals or EXC_CRASH exceptions. diff --git a/compat/non_win/dbghelp.h b/compat/non_win/dbghelp.h index 21a58dc2..3be6e5af 100644 --- a/compat/non_win/dbghelp.h +++ b/compat/non_win/dbghelp.h @@ -422,11 +422,15 @@ struct __attribute__((packed, aligned(4))) MINIDUMP_EXCEPTION { //! operating system-specific values. //! //! For Mac OS X minidumps, this will be the value of the exception code at - //! index 0 as received by a Mach exception handler. For exception type - //! `EXC_CRASH` generated from another preceding exception, the original - //! exception code will appear here, not the code as received by the Mach - //! exception handler. The code as it was received will appear at index 1 of - //! #ExceptionInformation. + //! index 0 as received by a Mach exception handler, except: + //! * For exception type `EXC_CRASH` generated from another preceding + //! exception, the original exception code will appear here, not the code + //! as received by the Mach exception handler. + //! * For exception types `EXC_RESOURCE` and `EXC_GUARD`, the high 32 bits of + //! the code received by the Mach exception handler will appear here. + //! + //! In all cases for Mac OS X minidumps, the code as it was received by the + //! Mach exception handler will appear at index 1 of #ExceptionInformation. //! //! \todo Document the possible values by OS. There may be OS-specific enums //! in minidump_extensions.h. @@ -456,7 +460,7 @@ struct __attribute__((packed, aligned(4))) MINIDUMP_EXCEPTION { //! `codes[1]` (exception code and subcode) parameters supplied to the Mach //! exception handler. Unlike #ExceptionCode and #ExceptionFlags, the values //! received by a Mach exception handler are used directly here even for the - //! `EXC_CRASH` exception type. + //! `EXC_CRASH`, `EXC_RESOURCE`, and `EXC_GUARD` exception types. uint64_t ExceptionInformation[EXCEPTION_MAXIMUM_PARAMETERS]; }; diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index ba7e97e7..9cd6629f 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -31,6 +31,7 @@ #include "util/mach/mach_extensions.h" #include "util/mach/mach_message.h" #include "util/mach/scoped_task_suspend.h" +#include "util/mach/symbolic_constants_mach.h" #include "util/misc/tri_state.h" #include "util/misc/uuid.h" @@ -101,11 +102,15 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( // carries identity information (valid thread and task ports). if (!ExceptionBehaviorHasIdentity(behavior)) { LOG(ERROR) << base::StringPrintf( - "unexpected exception behavior 0x%x, rejecting", behavior); + "unexpected exception behavior %s, rejecting", + ExceptionBehaviorToString( + behavior, kUseFullName | kUnknownIsNumeric | kUseOr).c_str()); return KERN_FAILURE; } else if (behavior != (EXCEPTION_STATE_IDENTITY | kMachExceptionCodes)) { LOG(WARNING) << base::StringPrintf( - "unexpected exception behavior 0x%x, proceeding", behavior); + "unexpected exception behavior %s, proceeding", + ExceptionBehaviorToString( + behavior, kUseFullName | kUnknownIsNumeric | kUseOr).c_str()); } if (task == mach_task_self()) { @@ -140,7 +145,8 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( process_snapshot.GetCrashpadOptions(&client_options); if (client_options.crashpad_handler_behavior != TriState::kDisabled) { - if (!process_snapshot.InitializeException(thread, + if (!process_snapshot.InitializeException(behavior, + thread, exception, code, code_count, @@ -206,6 +212,10 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( // processes that haven’t actually crashed, and could result in reports not // actually associated with crashes being sent to the operating system // vendor. + // + // Note that normally, EXC_RESOURCE and EXC_GUARD exceptions are sent to the + // system-level com.apple.ReportCrash.Root job, and not to the user-level + // job that they are forwarded to here. mach_port_t system_crash_reporter_port; const char kSystemCrashReporterServiceName[] = "com.apple.ReportCrash"; kern_return_t kr = bootstrap_look_up(bootstrap_port, diff --git a/snapshot/exception_snapshot.h b/snapshot/exception_snapshot.h index 20598332..f5ffd06a 100644 --- a/snapshot/exception_snapshot.h +++ b/snapshot/exception_snapshot.h @@ -60,10 +60,15 @@ class ExceptionSnapshot { //! This is an operating system-specific value. //! //! For Mac OS X, this will be the value of the exception code at index 0 as - //! received by a Mach exception handler. For `EXC_CRASH` exceptions generated - //! from another preceding exception, the original exception code will appear - //! here, not the code as received by the Mach exception handler. The code as - //! it was received will appear at index 1 of Codes(). + //! received by a Mach exception handler, except: + //! * For `EXC_CRASH` exceptions generated from another preceding exception, + //! the original exception code will appear here, not the code as received + //! by the Mach exception handler. + //! * For `EXC_RESOURCE` and `EXC_GUARD` exceptions, the high 32 bits of the + //! exception code at index 0 will appear here. + //! + //! In all cases on Mac OS X, the full exception code at index 0 as it was + //! received will appear at index 1 of Codes(). virtual uint32_t ExceptionInfo() const = 0; //! \brief Returns the address that triggered the exception. diff --git a/snapshot/mac/exception_snapshot_mac.cc b/snapshot/mac/exception_snapshot_mac.cc index 69ac6b52..9a9703a2 100644 --- a/snapshot/mac/exception_snapshot_mac.cc +++ b/snapshot/mac/exception_snapshot_mac.cc @@ -19,6 +19,8 @@ #include "snapshot/mac/cpu_context_mac.h" #include "snapshot/mac/process_reader.h" #include "util/mach/exc_server_variants.h" +#include "util/mach/exception_behaviors.h" +#include "util/mach/symbolic_constants_mach.h" #include "util/numeric/safe_assignment.h" namespace crashpad { @@ -40,6 +42,7 @@ ExceptionSnapshotMac::~ExceptionSnapshotMac() { } bool ExceptionSnapshotMac::Initialize(ProcessReader* process_reader, + exception_behavior_t behavior, thread_t exception_thread, exception_type_t exception, const mach_exception_data_type_t* code, @@ -62,16 +65,75 @@ bool ExceptionSnapshotMac::Initialize(ProcessReader* process_reader, if (exception_ == EXC_CRASH) { exception_ = ExcCrashRecoverOriginalException( exception_code_0, &exception_code_0, nullptr); + + if (exception_ == EXC_RESOURCE || exception_ == EXC_GUARD) { + // These are software exceptions that are never mapped to EXC_CRASH. The + // only time EXC_CRASH is generated is for processes exiting due to an + // unhandled core-generating signal or being killed by SIGKILL for + // code-signing reasons. Neither of these applies to EXC_RESOURCE or + // EXC_GUARD. See 10.10 xnu-2782.1.97/bsd/kern/kern_exit.c + // proc_prepareexit(). + // + // Receiving these exception types wrapped in EXC_CRASH would lose + // information because their code[0] uses all 64 bits (see below) and the + // code[0] recovered from EXC_CRASH only contains 20 significant bits. + LOG(WARNING) << base::StringPrintf( + "exception %s invalid in EXC_CRASH", + ExceptionToString(exception_, kUseFullName | kUnknownIsNumeric) + .c_str()); + } } - // ExceptionInfo() returns code[0] in a 32-bit field. This shouldn’t be a - // problem because code[0]’s values never exceed 32 bits. Only code[1] is ever - // expected to be that wide. - if (!AssignIfInRange(&exception_code_0_, exception_code_0)) { - LOG(WARNING) - << base::StringPrintf("exception_code_0 0x%llx out of range", - exception_code_0); - return false; + // The operations that follow put exception_code_0 (a mach_exception_code_t, + // a typedef for int64_t) into exception_code_0_ (a uint32_t). The range + // checks and bit shifts involved need the same signedness on both sides to + // work properly. + const uint64_t unsigned_exception_code_0 = exception_code_0; + + // ExceptionInfo() returns code[0] as a 32-bit value, but exception_code_0 is + // a 64-bit value. The best treatment for this inconsistency depends on the + // exception type. + if (exception_ == EXC_RESOURCE || exception_ == EXC_GUARD) { + // All 64 bits of code[0] are significant for these exceptions. See + // for EXC_RESOURCE and 10.10 + // xnu-2782.1.97/bsd/kern/kern_guarded.c fd_guard_ast() for EXC_GUARD. + // code[0] is structured similarly for these two exceptions. + // + // EXC_RESOURCE: see . The resource type and “flavor” + // together define the resource and are in the highest bits. The resource + // limit is in the lowest bits. + // + // EXC_GUARD: see 10.10 xnu-2782.1.97/osfmk/ipc/mach_port.c + // mach_port_guard_exception() and xnu-2782.1.97/bsd/kern/kern_guarded.c + // fd_guard_ast(). The guard type (GUARD_TYPE_MACH_PORT or GUARD_TYPE_FD) + // and “flavor” (from the mach_port_guard_exception_codes or + // guard_exception_codes enums) are in the highest bits. The violating Mach + // port name or file descriptor number is in the lowest bits. + + // If MACH_EXCEPTION_CODES is not set in |behavior|, code[0] will only carry + // 32 significant bits, and the interesting high bits will have been + // truncated. + if (!ExceptionBehaviorHasMachExceptionCodes(behavior)) { + LOG(WARNING) << base::StringPrintf( + "behavior %s invalid for exception %s", + ExceptionBehaviorToString( + behavior, kUseFullName | kUnknownIsNumeric | kUseOr).c_str(), + ExceptionToString(exception_, kUseFullName | kUnknownIsNumeric) + .c_str()); + } + + // Include the more-significant information from the high bits of code[0] in + // the value to be returned by ExceptionInfo(). The full value of codes[0] + // including the less-significant lower bits is still available via Codes(). + exception_code_0_ = unsigned_exception_code_0 >> 32; + } else { + // For other exceptions, code[0]’s values never exceed 32 bits. + if (!base::IsValueInRangeForNumericType( + unsigned_exception_code_0)) { + LOG(WARNING) << base::StringPrintf("exception_code_0 0x%llx out of range", + unsigned_exception_code_0); + } + exception_code_0_ = unsigned_exception_code_0; } const ProcessReader::Thread* thread = nullptr; @@ -83,7 +145,7 @@ bool ExceptionSnapshotMac::Initialize(ProcessReader* process_reader, } if (!thread) { - LOG(WARNING) << "exception_thread not found in task"; + LOG(ERROR) << "exception_thread not found in task"; return false; } @@ -132,6 +194,19 @@ bool ExceptionSnapshotMac::Initialize(ProcessReader* process_reader, #endif if (code_1_is_exception_address) { + if (process_reader->Is64Bit() && + !ExceptionBehaviorHasMachExceptionCodes(behavior)) { + // If code[1] is an address from a 64-bit process, the exception must have + // been received with MACH_EXCEPTION_CODES or the address will have been + // truncated. + LOG(WARNING) << base::StringPrintf( + "behavior %s invalid for exception %s code %d in 64-bit process", + ExceptionBehaviorToString( + behavior, kUseFullName | kUnknownIsNumeric | kUseOr).c_str(), + ExceptionToString(exception_, kUseFullName | kUnknownIsNumeric) + .c_str(), + exception_code_0_); + } exception_address_ = code[1]; } else { exception_address_ = context_.InstructionPointer(); diff --git a/snapshot/mac/exception_snapshot_mac.h b/snapshot/mac/exception_snapshot_mac.h index e6255b1b..ea1161c3 100644 --- a/snapshot/mac/exception_snapshot_mac.h +++ b/snapshot/mac/exception_snapshot_mac.h @@ -51,6 +51,7 @@ class ExceptionSnapshotMac final : public ExceptionSnapshot { //! \return `true` if the snapshot could be created, `false` otherwise with //! an appropriate message logged. bool Initialize(ProcessReader* process_reader, + exception_behavior_t behavior, thread_t exception_thread, exception_type_t exception, const mach_exception_data_type_t* code, diff --git a/snapshot/mac/process_snapshot_mac.cc b/snapshot/mac/process_snapshot_mac.cc index 71d45c3c..6d907ee0 100644 --- a/snapshot/mac/process_snapshot_mac.cc +++ b/snapshot/mac/process_snapshot_mac.cc @@ -57,6 +57,7 @@ bool ProcessSnapshotMac::Initialize(task_t task) { } bool ProcessSnapshotMac::InitializeException( + exception_behavior_t behavior, thread_t exception_thread, exception_type_t exception, const mach_exception_data_type_t* code, @@ -69,6 +70,7 @@ bool ProcessSnapshotMac::InitializeException( exception_.reset(new internal::ExceptionSnapshotMac()); if (!exception_->Initialize(&process_reader_, + behavior, exception_thread, exception, code, diff --git a/snapshot/mac/process_snapshot_mac.h b/snapshot/mac/process_snapshot_mac.h index d9d34804..6e3c07c1 100644 --- a/snapshot/mac/process_snapshot_mac.h +++ b/snapshot/mac/process_snapshot_mac.h @@ -70,7 +70,8 @@ class ProcessSnapshotMac final : public ProcessSnapshot { //! \return `true` if the exception information could be initialized, `false` //! otherwise with an appropriate message logged. When this method returns //! `false`, the ProcessSnapshotMac object’s validity remains unchanged. - bool InitializeException(thread_t exception_thread, + bool InitializeException(exception_behavior_t behavior, + thread_t exception_thread, exception_type_t exception, const mach_exception_data_type_t* code, mach_msg_type_number_t code_count,