From 667424894f19f4e83d56920eec94106b526a800c Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Thu, 20 Jan 2022 08:57:13 -0800 Subject: [PATCH] linux: re-order first-chance-handlers, and disabled signal handlers Both running first chance handlers and checking for disabled signal handlers should no longer interact with DumpWithoutCrashing(). First-chance-handlers should also run even with disabled crashpad signal handlers or else those signals would be reported by the next chained signal handlers as crashes. Change-Id: I64b3da42c400a1c431c6228d4da181ed56bfda89 Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3403413 Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- client/crashpad_client.h | 2 +- client/crashpad_client_linux.cc | 22 ++++---- client/crashpad_client_linux_test.cc | 59 +++++++++++++--------- snapshot/linux/exception_snapshot_linux.cc | 4 ++ 4 files changed, 48 insertions(+), 39 deletions(-) diff --git a/client/crashpad_client.h b/client/crashpad_client.h index a5e54a58..6944fb69 100644 --- a/client/crashpad_client.h +++ b/client/crashpad_client.h @@ -421,7 +421,7 @@ class CrashpadClient { //! CaptureContext() or similar. static void DumpWithoutCrash(NativeCPUContext* context); - //! \brief Disables any installed crash handler, including any + //! \brief Disables any installed crash handler, not including any //! FirstChanceHandler and crashes the current process. //! //! \param[in] message A message to be logged before crashing. diff --git a/client/crashpad_client_linux.cc b/client/crashpad_client_linux.cc index 4c9a307c..cb07ef08 100644 --- a/client/crashpad_client_linux.cc +++ b/client/crashpad_client_linux.cc @@ -148,17 +148,7 @@ class SignalHandler { // The base implementation for all signal handlers, suitable for calling // directly to simulate signal delivery. - bool HandleCrash(int signo, siginfo_t* siginfo, void* context) { - if (disabled_for_thread_) { - return false; - } - - if (first_chance_handler_ && - first_chance_handler_( - signo, siginfo, static_cast(context))) { - return true; - } - + void HandleCrash(int signo, siginfo_t* siginfo, void* context) { exception_information_.siginfo_address = FromPointerCast( siginfo); @@ -169,7 +159,6 @@ class SignalHandler { ScopedPrSetDumpable set_dumpable(false); HandleCrashImpl(); - return false; } protected: @@ -198,9 +187,16 @@ class SignalHandler { static void HandleOrReraiseSignal(int signo, siginfo_t* siginfo, void* context) { - if (handler_->HandleCrash(signo, siginfo, context)) { + if (handler_->first_chance_handler_ && + handler_->first_chance_handler_( + signo, siginfo, static_cast(context))) { return; } + + if (!handler_->disabled_for_thread_) { + handler_->HandleCrash(signo, siginfo, context); + } + Signals::RestoreHandlerAndReraiseSignalOnReturn( siginfo, handler_->old_actions_.ActionForSignal(signo)); } diff --git a/client/crashpad_client_linux_test.cc b/client/crashpad_client_linux_test.cc index 08c78b0b..e58352b0 100644 --- a/client/crashpad_client_linux_test.cc +++ b/client/crashpad_client_linux_test.cc @@ -15,6 +15,7 @@ #include "client/crashpad_client.h" #include +#include #include #include #include @@ -108,10 +109,6 @@ class StartHandlerForSelfTest StartHandlerForSelfTestOptions options_; }; -bool HandleCrashSuccessfully(int, siginfo_t*, ucontext_t*) { - return true; -} - bool InstallHandler(CrashpadClient* client, bool start_at_crash, const base::FilePath& handler_path, @@ -222,13 +219,21 @@ int RecurseInfinitely(int* ptr) { } #pragma clang diagnostic pop +sigjmp_buf do_crash_sigjmp_env; + +bool HandleCrashSuccessfully(int, siginfo_t*, ucontext_t*) { + siglongjmp(do_crash_sigjmp_env, 1); + return true; +} + void DoCrash(const StartHandlerForSelfTestOptions& options, CrashpadClient* client) { + if (sigsetjmp(do_crash_sigjmp_env, 1) != 0) { + return; + } + switch (options.crash_type) { case CrashType::kSimulated: - if (options.set_first_chance_handler) { - client->SetFirstChanceExceptionHandler(HandleCrashSuccessfully); - } CRASHPAD_SIMULATE_CRASH(); break; @@ -364,6 +369,10 @@ CRASHPAD_CHILD_TEST_MAIN(StartHandlerForSelfTestChild) { return EXIT_FAILURE; } + if (options.set_first_chance_handler) { + client.SetFirstChanceExceptionHandler(HandleCrashSuccessfully); + } + #if BUILDFLAG(IS_ANDROID) if (android_set_abort_message) { android_set_abort_message(kTestAbortMessage); @@ -386,17 +395,19 @@ class StartHandlerForSelfInChildTest : public MultiprocessExec { StartHandlerForSelfInChildTest(const StartHandlerForSelfTestOptions& options) : MultiprocessExec(), options_(options) { SetChildTestMainFunction("StartHandlerForSelfTestChild"); - switch (options.crash_type) { - case CrashType::kSimulated: - // kTerminationNormal, EXIT_SUCCESS - break; - case CrashType::kBuiltinTrap: - SetExpectedChildTerminationBuiltinTrap(); - break; - case CrashType::kInfiniteRecursion: - SetExpectedChildTermination(TerminationReason::kTerminationSignal, - SIGSEGV); - break; + if (!options.set_first_chance_handler) { + switch (options.crash_type) { + case CrashType::kSimulated: + // kTerminationNormal, EXIT_SUCCESS + break; + case CrashType::kBuiltinTrap: + SetExpectedChildTerminationBuiltinTrap(); + break; + case CrashType::kInfiniteRecursion: + SetExpectedChildTermination(TerminationReason::kTerminationSignal, + SIGSEGV); + break; + } } } @@ -447,9 +458,12 @@ class StartHandlerForSelfInChildTest : public MultiprocessExec { reports.clear(); ASSERT_EQ(database->GetPendingReports(&reports), CrashReportDatabase::kNoError); - ASSERT_EQ(reports.size(), options_.set_first_chance_handler ? 0u : 1u); - if (options_.set_first_chance_handler) { + bool report_expected = !options_.set_first_chance_handler || + options_.crash_type == CrashType::kSimulated; + ASSERT_EQ(reports.size(), report_expected ? 1u : 0u); + + if (!report_expected) { return; } @@ -463,11 +477,6 @@ class StartHandlerForSelfInChildTest : public MultiprocessExec { }; TEST_P(StartHandlerForSelfTest, StartHandlerInChild) { - if (Options().set_first_chance_handler && - Options().crash_type != CrashType::kSimulated) { - // TODO(jperaza): test first chance handlers with real crashes. - return; - } #if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \ defined(UNDEFINED_SANITIZER) if (Options().crash_type == CrashType::kInfiniteRecursion) { diff --git a/snapshot/linux/exception_snapshot_linux.cc b/snapshot/linux/exception_snapshot_linux.cc index 6726d048..efc9e569 100644 --- a/snapshot/linux/exception_snapshot_linux.cc +++ b/snapshot/linux/exception_snapshot_linux.cc @@ -24,6 +24,7 @@ #include "util/linux/traits.h" #include "util/misc/reinterpret_bytes.h" #include "util/numeric/safe_assignment.h" +#include "util/posix/signals.h" namespace crashpad { namespace internal { @@ -445,6 +446,9 @@ bool ExceptionSnapshotLinux::ReadSiginfo(ProcessReaderLinux* reader, PUSH_CODE(siginfo.sigval.sigval); break; + case Signals::kSimulatedSigno: + break; + default: LOG(WARNING) << "Unhandled signal " << siginfo.signo; }