From 352160906b96c28289db2235022d872d69a8a370 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 1 Apr 2015 12:16:22 -0400 Subject: [PATCH] Add ExcServerCopyState(). ExcServerCopyState() properly sets the new_state and new_state_count out-parameters for exception handler routines that may deal with state-carrying exceptions. This used to exist inline in catch_exception_tool, but that implementation had a bug caught by the new test. TEST=crashpad_util_test ExcServerVariants.ExcServerCopyState and others R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1049023003 --- client/simulate_crash_mac_test.cc | 3 + handler/mac/crash_report_exception_handler.cc | 14 +++- tools/mac/catch_exception_tool.cc | 10 +-- util/mach/exc_server_variants.cc | 13 ++++ util/mach/exc_server_variants.h | 37 +++++++++ util/mach/exc_server_variants_test.cc | 77 +++++++++++++++++++ 6 files changed, 145 insertions(+), 9 deletions(-) diff --git a/client/simulate_crash_mac_test.cc b/client/simulate_crash_mac_test.cc index 1ef0a775..5ab975fd 100644 --- a/client/simulate_crash_mac_test.cc +++ b/client/simulate_crash_mac_test.cc @@ -225,6 +225,9 @@ class TestSimulateCrashMac final : public MachMultiprocess, return KERN_ABORTED; } + ExcServerCopyState( + behavior, old_state, old_state_count, new_state, new_state_count); + return ExcServerSuccessfulReturnValue(behavior, true); } diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index 404bfcac..1eaec68c 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -193,6 +193,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( upload_thread_->ReportPending(); } + bool forwarded = false; if (client_options.system_crash_reporter_forwarding != TriState::kDisabled && (exception == EXC_CRASH || exception == EXC_RESOURCE || @@ -243,11 +244,20 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( old_state_count, new_state_forward_count ? &new_state_forward[0] : nullptr, &new_state_forward_count); - MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr) - << "UniversalExceptionRaise " << kSystemCrashReporterServiceName; + if (kr == KERN_SUCCESS) { + forwarded = true; + } else { + MACH_LOG(WARNING, kr) + << "UniversalExceptionRaise " << kSystemCrashReporterServiceName; + } } } + if (!forwarded) { + ExcServerCopyState( + behavior, old_state, old_state_count, new_state, new_state_count); + } + return ExcServerSuccessfulReturnValue(behavior, false); } diff --git a/tools/mac/catch_exception_tool.cc b/tools/mac/catch_exception_tool.cc index 016ff4c3..eb97befb 100644 --- a/tools/mac/catch_exception_tool.cc +++ b/tools/mac/catch_exception_tool.cc @@ -133,13 +133,6 @@ class ExceptionServer : public UniversalMachExcServer::Interface { } if (ExceptionBehaviorHasState(behavior)) { - // If this is a state-carrying exception, make new_state something valid. - memcpy( - new_state, - old_state, - std::min(old_state_count, *new_state_count) * sizeof(old_state[0])); - *new_state_count = old_state_count; - std::string flavor_string = ThreadStateFlavorToString(*flavor, kUseFullName | kUnknownIsNumeric); fprintf(options_.file, @@ -156,6 +149,9 @@ class ExceptionServer : public UniversalMachExcServer::Interface { return KERN_FAILURE; } + ExcServerCopyState( + behavior, old_state, old_state_count, new_state, new_state_count); + return ExcServerSuccessfulReturnValue(behavior, false); } diff --git a/util/mach/exc_server_variants.cc b/util/mach/exc_server_variants.cc index 6e5cd3f2..b6e046a4 100644 --- a/util/mach/exc_server_variants.cc +++ b/util/mach/exc_server_variants.cc @@ -14,6 +14,8 @@ #include "util/mach/exc_server_variants.h" +#include + #include #include @@ -801,4 +803,15 @@ kern_return_t ExcServerSuccessfulReturnValue(exception_behavior_t behavior, return KERN_SUCCESS; } +void ExcServerCopyState(exception_behavior_t behavior, + const natural_t* old_state, + mach_msg_type_number_t old_state_count, + thread_state_t new_state, + mach_msg_type_number_t* new_state_count) { + if (ExceptionBehaviorHasState(behavior)) { + *new_state_count = std::min(old_state_count, *new_state_count); + memcpy(new_state, old_state, *new_state_count * sizeof(old_state[0])); + } +} + } // namespace crashpad diff --git a/util/mach/exc_server_variants.h b/util/mach/exc_server_variants.h index d5d5cc54..3109ca0d 100644 --- a/util/mach/exc_server_variants.h +++ b/util/mach/exc_server_variants.h @@ -199,6 +199,43 @@ exception_type_t ExcCrashRecoverOriginalException( kern_return_t ExcServerSuccessfulReturnValue(exception_behavior_t behavior, bool set_thread_state); +//! \brief Copies the old state to the new state for state-carrying exceptions. +//! +//! When the kernel sends a state-carrying exception request and the response is +//! successful (`MACH_MSG_SUCCESS`, a synonym for `KERN_SUCCESS`), it will set +//! a new thread state based on \a new_state and \a new_state_count. To ease +//! initialization of the new state, this function copies \a old_state and +//! \a old_state_count. This is only done if \a behavior indicates a +//! state-carrying exception. +//! +//! \param[in] behavior The behavior of the exception handler as invoked. This +//! may be taken directly from the \a behavior parameter of +//! internal::SimplifiedExcServer::Interface::CatchException(), for example. +//! \param[in] old_state The original state value. This may be taken directly +//! from the \a old_state parameter of +//! internal::SimplifiedExcServer::Interface::CatchException(), for example. +//! \param[in] old_state_count The number of significant `natural_t` words in \a +//! old_state. This may be taken directly from the \a old_state_count +//! parameter of internal::SimplifiedExcServer::Interface::CatchException(), +//! for example. +//! \param[out] new_state The state value to be set. This may be taken directly +//! from the \a new_state parameter of +//! internal::SimplifiedExcServer::Interface::CatchException(), for example. +//! This parameter is untouched if \a behavior is not state-carrying. +//! \param[inout] new_state_count On entry, the number of `natural_t` words +//! available to be written to in \a new_state. On return, the number of +//! significant `natural_t` words in \a new_state. This may be taken +//! directly from the \a new_state_count parameter of +//! internal::SimplifiedExcServer::Interface::CatchException(), for example. +//! This parameter is untouched if \a behavior is not state-carrying. If \a +//! \a behavior is state-carrying, this parameter should be at least as +//! large as \a old_state_count. +void ExcServerCopyState(exception_behavior_t behavior, + const natural_t* old_state, + mach_msg_type_number_t old_state_count, + thread_state_t new_state, + mach_msg_type_number_t* new_state_count); + } // namespace crashpad #endif // CRASHPAD_UTIL_MACH_EXC_SERVER_VARIANTS_H_ diff --git a/util/mach/exc_server_variants_test.cc b/util/mach/exc_server_variants_test.cc index b9015118..b0309bee 100644 --- a/util/mach/exc_server_variants_test.cc +++ b/util/mach/exc_server_variants_test.cc @@ -1032,6 +1032,9 @@ class TestExcServerVariants : public MachMultiprocess, EXPECT_EQ(REQUESTED_TRAILER_SIZE(kMachMessageOptions), trailer->msgh_trailer_size); + ExcServerCopyState( + behavior, old_state, old_state_count, new_state, new_state_count); + return ExcServerSuccessfulReturnValue(behavior, false); } @@ -1275,6 +1278,80 @@ TEST(ExcServerVariants, ExcServerSuccessfulReturnValue) { } } +TEST(ExcServerVariants, ExcServerCopyState) { + const natural_t old_state[] = {1, 2, 3, 4, 5}; + natural_t new_state[10] = {}; + + const mach_msg_type_number_t old_state_count = arraysize(old_state); + mach_msg_type_number_t new_state_count = arraysize(new_state); + + // EXCEPTION_DEFAULT (with or without MACH_EXCEPTION_CODES) is not + // state-carrying. new_state and new_state_count should be untouched. + ExcServerCopyState(EXCEPTION_DEFAULT, + old_state, + old_state_count, + new_state, + &new_state_count); + EXPECT_EQ(arraysize(new_state), new_state_count); + for (size_t i = 0; i < arraysize(new_state); ++i) { + EXPECT_EQ(0u, new_state[i]) << "i " << i; + } + + ExcServerCopyState(MACH_EXCEPTION_CODES | EXCEPTION_DEFAULT, + old_state, + old_state_count, + new_state, + &new_state_count); + EXPECT_EQ(arraysize(new_state), new_state_count); + for (size_t i = 0; i < arraysize(new_state); ++i) { + EXPECT_EQ(0u, new_state[i]) << "i " << i; + } + + // This is a state-carrying exception where old_state_count is small. + mach_msg_type_number_t copy_limit = 2; + ExcServerCopyState( + EXCEPTION_STATE, old_state, copy_limit, new_state, &new_state_count); + EXPECT_EQ(copy_limit, new_state_count); + for (size_t i = 0; i < copy_limit; ++i) { + EXPECT_EQ(old_state[i], new_state[i]) << "i " << i; + } + for (size_t i = copy_limit; i < arraysize(new_state); ++i) { + EXPECT_EQ(0u, new_state[i]) << "i " << i; + } + + // This is a state-carrying exception where new_state_count is small. + copy_limit = 3; + new_state_count = copy_limit; + ExcServerCopyState(EXCEPTION_STATE_IDENTITY, + old_state, + old_state_count, + new_state, + &new_state_count); + EXPECT_EQ(copy_limit, new_state_count); + for (size_t i = 0; i < copy_limit; ++i) { + EXPECT_EQ(old_state[i], new_state[i]) << "i " << i; + } + for (size_t i = copy_limit; i < arraysize(new_state); ++i) { + EXPECT_EQ(0u, new_state[i]) << "i " << i; + } + + // This is a state-carrying exception where all of old_state is copied to + // new_state, which is large enough to receive it and then some. + new_state_count = arraysize(new_state); + ExcServerCopyState(MACH_EXCEPTION_CODES | EXCEPTION_STATE_IDENTITY, + old_state, + old_state_count, + new_state, + &new_state_count); + EXPECT_EQ(old_state_count, new_state_count); + for (size_t i = 0; i < arraysize(old_state); ++i) { + EXPECT_EQ(old_state[i], new_state[i]) << "i " << i; + } + for (size_t i = arraysize(old_state); i < arraysize(new_state); ++i) { + EXPECT_EQ(0u, new_state[i]) << "i " << i; + } +} + } // namespace } // namespace test } // namespace crashpad