From ce7f0f1de9bb9cdb88ff93046fe3e498b1f19295 Mon Sep 17 00:00:00 2001 From: Keishi Hattori Date: Thu, 3 Aug 2023 19:10:49 +0000 Subject: [PATCH] Revert "Add SetLastChanceExceptionHandler to implement permissive MTE mode" This reverts commit b1e66e322ddd07f4640ee8bad93397a0511cd313. Reason for revert: test was flaky on Android bot Original change's description: > Add SetLastChanceExceptionHandler to implement permissive MTE mode > > SetLastChanceExceptionHandler sets a callback to be called after a > crash has been reported. Returning true from this callback will > not reraise the signal so the execution can continue. This will be > used to implement permissive MTE mode, which will continue execution > after a MTE crash. > > Bug: chromium:1467915 > Change-Id: I93a28ceea921fe977805482cf47c07643ca6133c > Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4707688 > Reviewed-by: Robert Sesek > Commit-Queue: Keishi Hattori Bug: chromium:1467915 Change-Id: Id815a780b576088974101117a4587adec64cfe8c No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4750459 Commit-Queue: Keishi Hattori Bot-Commit: Rubber Stamper --- client/crashpad_client.h | 18 ------------- client/crashpad_client_linux.cc | 18 ------------- client/crashpad_client_linux_test.cc | 39 +++------------------------- 3 files changed, 3 insertions(+), 72 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index 3070e2e0..11fa66e2 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -456,24 +456,6 @@ class CrashpadClient { //! \param[in] handler The custom crash signal handler to install. static void SetFirstChanceExceptionHandler(FirstChanceHandler handler); - //! \brief Installs a custom crash signal handler which runs after the - //! currently installed Crashpad handler. - //! - //! Handling signals appropriately can be tricky and use of this method - //! should be avoided, if possible. - //! - //! A handler must have already been installed before calling this method. - //! - //! The custom handler runs in a signal handler context and must be safe for - //! that purpose. - //! - //! If the custom handler returns `true`, the signal is not reraised. - //! - //! \param[in] handler The custom crash signal handler to install. - static void SetLastChanceExceptionHandler(bool (*handler)(int, - siginfo_t*, - ucontext_t*)); - //! \brief Configures a set of signals that shouldn't have Crashpad signal //! handlers installed. //! diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index f805ff1f..630c24f1 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -131,8 +131,6 @@ std::vector BuildArgsToLaunchWithLinker( #endif // BUILDFLAG(IS_ANDROID) -using LastChanceHandler = bool (*)(int, siginfo_t*, ucontext_t*); - // A base class for Crashpad signal handler implementations. class SignalHandler { public: @@ -156,10 +154,6 @@ class SignalHandler { first_chance_handler_ = handler; } - void SetLastChanceExceptionHandler(LastChanceHandler handler) { - last_chance_handler_ = handler; - } - // The base implementation for all signal handlers, suitable for calling // directly to simulate signal delivery. void HandleCrash(int signo, siginfo_t* siginfo, void* context) { @@ -218,11 +212,6 @@ class SignalHandler { if (!handler_->disabled_.test_and_set()) { handler_->HandleCrash(signo, siginfo, context); handler_->WakeThreads(); - if (handler_->last_chance_handler_ && - handler_->last_chance_handler_( - signo, siginfo, static_cast(context))) { - return; - } } else { // Processes on Android normally have several chained signal handlers that // co-operate to report crashes. e.g. WebView will have this signal @@ -265,7 +254,6 @@ class SignalHandler { Signals::OldActions old_actions_ = {}; ExceptionInformation exception_information_ = {}; CrashpadClient::FirstChanceHandler first_chance_handler_ = nullptr; - LastChanceHandler last_chance_handler_ = nullptr; int32_t dump_done_futex_ = kDumpNotDone; #if !defined(__cpp_lib_atomic_value_initialization) || \ __cpp_lib_atomic_value_initialization < 201911L @@ -751,12 +739,6 @@ void CrashpadClient::SetFirstChanceExceptionHandler( SignalHandler::Get()->SetFirstChanceHandler(handler); } -// static -void CrashpadClient::SetLastChanceExceptionHandler(LastChanceHandler handler) { - DCHECK(SignalHandler::Get()); - SignalHandler::Get()->SetLastChanceExceptionHandler(handler); -} - void CrashpadClient::SetUnhandledSignals(const std::set& signals) { DCHECK(!SignalHandler::Get()); unhandled_signals_ = signals; diff --git a/client/crashpad_client_linux_test.cc b/client/crashpad_client_linux_test.cc index 8f4151e5..9b207db3 100644 --- a/client/crashpad_client_linux_test.cc +++ b/client/crashpad_client_linux_test.cc @@ -71,14 +71,11 @@ enum class CrashType : uint32_t { kBuiltinTrap, kInfiniteRecursion, kSegvWithTagBits, - // kFakeSegv is meant to simulate a MTE segv error. - kFakeSegv, }; struct StartHandlerForSelfTestOptions { bool start_handler_at_crash; bool set_first_chance_handler; - bool set_last_chance_handler; bool crash_non_main_thread; bool client_uses_signals; bool gather_indirectly_referenced_memory; @@ -87,7 +84,7 @@ struct StartHandlerForSelfTestOptions { class StartHandlerForSelfTest : public testing::TestWithParam< - std::tuple> { + std::tuple> { public: StartHandlerForSelfTest() = default; @@ -102,7 +99,6 @@ class StartHandlerForSelfTest memset(&options_, 0, sizeof(options_)); std::tie(options_.start_handler_at_crash, options_.set_first_chance_handler, - options_.set_last_chance_handler, options_.crash_non_main_thread, options_.client_uses_signals, options_.gather_indirectly_referenced_memory, @@ -248,10 +244,6 @@ bool HandleCrashSuccessfully(int, siginfo_t*, ucontext_t*) { #pragma clang diagnostic pop } -bool HandleCrashSuccessfullyAfterReporting(int, siginfo_t*, ucontext_t*) { - return true; -} - void DoCrash(const StartHandlerForSelfTestOptions& options, CrashpadClient* client) { if (sigsetjmp(do_crash_sigjmp_env, 1) != 0) { @@ -281,11 +273,6 @@ void DoCrash(const StartHandlerForSelfTestOptions& options, *x; break; } - - case CrashType::kFakeSegv: { - raise(SIGSEGV); - break; - } } } @@ -416,10 +403,6 @@ CRASHPAD_CHILD_TEST_MAIN(StartHandlerForSelfTestChild) { client.SetFirstChanceExceptionHandler(HandleCrashSuccessfully); } - if (options.set_last_chance_handler) { - client.SetLastChanceExceptionHandler(HandleCrashSuccessfullyAfterReporting); - } - #if BUILDFLAG(IS_ANDROID) if (android_set_abort_message) { android_set_abort_message(kTestAbortMessage); @@ -457,16 +440,6 @@ class StartHandlerForSelfInChildTest : public MultiprocessExec { case CrashType::kSegvWithTagBits: SetExpectedChildTermination(TerminationReason::kTerminationSignal, SIGSEGV); - break; - case CrashType::kFakeSegv: - if (!options.set_last_chance_handler) { - SetExpectedChildTermination(TerminationReason::kTerminationSignal, - SIGSEGV); - } else { - SetExpectedChildTermination(TerminationReason::kTerminationNormal, - EXIT_SUCCESS); - } - break; } } } @@ -498,11 +471,7 @@ class StartHandlerForSelfInChildTest : public MultiprocessExec { writer.Close(); if (options_.client_uses_signals && !options_.set_first_chance_handler && - options_.crash_type != CrashType::kSimulated && - // The last chance handler will prevent the client handler from being - // called if crash type is kFakeSegv. - (!options_.set_last_chance_handler || - options_.crash_type != CrashType::kFakeSegv)) { + options_.crash_type != CrashType::kSimulated) { // Wait for child's client signal handler. char c; EXPECT_TRUE(LoggingReadFileExactly(ReadPipeHandle(), &c, sizeof(c))); @@ -580,12 +549,10 @@ INSTANTIATE_TEST_SUITE_P( testing::Bool(), testing::Bool(), testing::Bool(), - testing::Bool(), testing::Values(CrashType::kSimulated, CrashType::kBuiltinTrap, CrashType::kInfiniteRecursion, - CrashType::kSegvWithTagBits, - CrashType::kFakeSegv))); + CrashType::kSegvWithTagBits))); // Test state for starting the handler for another process. class StartHandlerForClientTest {