diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index c33020e5..b9c6d111 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -51,16 +51,12 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( ProcessSnapshotWin process_snapshot; if (!process_snapshot.Initialize(process, ProcessSuspensionState::kSuspended, + exception_information_address, debug_critical_section_address)) { LOG(WARNING) << "ProcessSnapshotWin::Initialize failed"; return kFailedTerminationCode; } - if (!process_snapshot.InitializeException(exception_information_address)) { - LOG(WARNING) << "ProcessSnapshotWin::InitializeException failed"; - return kFailedTerminationCode; - } - // Now that we have the exception information, even if something else fails we // can terminate the process with the correct exit code. const unsigned int termination_code = diff --git a/snapshot/crashpad_info_client_options_test.cc b/snapshot/crashpad_info_client_options_test.cc index 6f32dd41..af607c32 100644 --- a/snapshot/crashpad_info_client_options_test.cc +++ b/snapshot/crashpad_info_client_options_test.cc @@ -78,7 +78,7 @@ CrashpadInfoClientOptions SelfProcessSnapshotAndGetCrashpadOptions() { #elif defined(OS_WIN) ProcessSnapshotWin process_snapshot; EXPECT_TRUE(process_snapshot.Initialize( - GetCurrentProcess(), ProcessSuspensionState::kRunning, 0)); + GetCurrentProcess(), ProcessSuspensionState::kRunning, 0, 0)); #else #error Port. #endif // OS_MACOSX diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index a5540e58..1fa820e7 100755 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -325,7 +325,10 @@ def RunTests(cdb_path, 'other program dump exception code') out.Check('!Sleep', 'other program reasonable location') out.Check('hanging_program!Thread1', 'other program dump right thread') - out.Check('\. 1 Id', 'other program exception on correct thread') + out.Check('\. 1 Id.*Suspend: 0 ', + 'other program exception on correct thread and correct suspend') + out.Check(' 4 Id.*Suspend: 0 ', + 'other program injection thread correct suspend') out = CdbRun(cdb_path, other_program_no_exception_path, '.ecxr;k') out.Check('Unknown exception - code 0cca11ed', diff --git a/snapshot/win/exception_snapshot_win.cc b/snapshot/win/exception_snapshot_win.cc index ca29e1f0..8e96d342 100644 --- a/snapshot/win/exception_snapshot_win.cc +++ b/snapshot/win/exception_snapshot_win.cc @@ -72,8 +72,7 @@ ExceptionSnapshotWin::~ExceptionSnapshotWin() { bool ExceptionSnapshotWin::Initialize( ProcessReaderWin* process_reader, DWORD thread_id, - WinVMAddress exception_pointers_address, - const PointerVector& threads) { + WinVMAddress exception_pointers_address) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); const ProcessReaderWin::Thread* thread = nullptr; @@ -98,9 +97,9 @@ bool ExceptionSnapshotWin::Initialize( if (is_64_bit) { if (!InitializeFromExceptionPointers( - *process_reader, + process_reader, exception_pointers_address, - threads, + thread_id, &NativeContextToCPUContext64)) { return false; } @@ -109,9 +108,9 @@ bool ExceptionSnapshotWin::Initialize( if (!is_64_bit) { if (!InitializeFromExceptionPointers( - *process_reader, + process_reader, exception_pointers_address, - threads, + thread_id, &NativeContextToCPUContext32)) { return false; } @@ -168,16 +167,16 @@ template bool ExceptionSnapshotWin::InitializeFromExceptionPointers( - const ProcessReaderWin& process_reader, + ProcessReaderWin* process_reader, WinVMAddress exception_pointers_address, - const PointerVector& threads, + DWORD thread_id, void (*native_to_cpu_context)(const ContextType& context_record, CPUContext* context, CPUContextUnion* context_union)) { ExceptionPointersType exception_pointers; - if (!process_reader.ReadMemory(exception_pointers_address, - sizeof(exception_pointers), - &exception_pointers)) { + if (!process_reader->ReadMemory(exception_pointers_address, + sizeof(exception_pointers), + &exception_pointers)) { LOG(ERROR) << "EXCEPTION_POINTERS read failed"; return false; } @@ -187,7 +186,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( } ExceptionRecordType first_record; - if (!process_reader.ReadMemory( + if (!process_reader->ReadMemory( static_cast(exception_pointers.ExceptionRecord), sizeof(first_record), &first_record)) { @@ -195,9 +194,13 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( return false; } - if (first_record.ExceptionCode == CrashpadClient::kTriggeredExceptionCode && - first_record.NumberParameters == 2 && - first_record.ExceptionInformation[0] != 0) { + const bool triggered_by_client = + first_record.ExceptionCode == CrashpadClient::kTriggeredExceptionCode && + first_record.NumberParameters == 2; + if (triggered_by_client) + process_reader->DecrementThreadSuspendCounts(thread_id); + + if (triggered_by_client && first_record.ExceptionInformation[0] != 0) { // This special exception code indicates that the target was crashed by // another client calling CrashpadClient::DumpAndCrashTargetProcess(). In // this case the parameters are a thread id and an exception code which we @@ -206,18 +209,14 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( const ArgumentType thread_id = first_record.ExceptionInformation[0]; exception_code_ = static_cast(first_record.ExceptionInformation[1]); exception_flags_ = EXCEPTION_NONCONTINUABLE; - for (const auto* thread : threads) { - if (thread->ThreadID() == thread_id) { + for (const auto& thread : process_reader->Threads()) { + if (thread.id == thread_id) { thread_id_ = thread_id; - exception_address_ = thread->Context()->InstructionPointer(); - context_.architecture = thread->Context()->architecture; - if (context_.architecture == kCPUArchitectureX86_64) { - context_union_.x86_64 = *thread->Context()->x86_64; - context_.x86_64 = &context_union_.x86_64; - } else { - context_union_.x86 = *thread->Context()->x86; - context_.x86 = &context_union_.x86; - } + native_to_cpu_context( + *reinterpret_cast(&thread.context), + &context_, + &context_union_); + exception_address_ = context_.InstructionPointer(); break; } } @@ -239,7 +238,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( } ContextType context_record; - if (!process_reader.ReadMemory( + if (!process_reader->ReadMemory( static_cast(exception_pointers.ContextRecord), sizeof(context_record), &context_record)) { diff --git a/snapshot/win/exception_snapshot_win.h b/snapshot/win/exception_snapshot_win.h index 8ff7d381..69e81777 100644 --- a/snapshot/win/exception_snapshot_win.h +++ b/snapshot/win/exception_snapshot_win.h @@ -57,12 +57,15 @@ class ExceptionSnapshotWin final : public ExceptionSnapshot { //! `EXCEPTION_POINTERS` record in the target process, passed through from //! the exception handler. //! + //! \note If the exception was triggered by + //! CrashpadClient::DumpAndCrashTargetProcess(), this has the side-effect + //! of correcting the thread suspend counts for \a process_reader. + //! //! \return `true` if the snapshot could be created, `false` otherwise with //! an appropriate message logged. bool Initialize(ProcessReaderWin* process_reader, DWORD thread_id, - WinVMAddress exception_pointers, - const PointerVector& threads); + WinVMAddress exception_pointers); // ExceptionSnapshot: @@ -79,9 +82,9 @@ class ExceptionSnapshotWin final : public ExceptionSnapshot { class ExceptionPointersType, class ContextType> bool InitializeFromExceptionPointers( - const ProcessReaderWin& process_reader, + ProcessReaderWin* process_reader, WinVMAddress exception_pointers_address, - const PointerVector& threads, + DWORD thread_id, void (*native_to_cpu_context)(const ContextType& context_record, CPUContext* context, CPUContextUnion* context_union)); diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index 5c3b0609..88612df3 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -91,8 +91,8 @@ class CrashingDelegate : public ExceptionHandlerServer::Delegate { ProcessSnapshotWin snapshot; snapshot.Initialize(process, ProcessSuspensionState::kSuspended, + exception_information_address, debug_critical_section_address); - snapshot.InitializeException(exception_information_address); // Confirm the exception record was read correctly. EXPECT_NE(snapshot.Exception()->ThreadID(), 0u); @@ -190,8 +190,8 @@ class SimulateDelegate : public ExceptionHandlerServer::Delegate { ProcessSnapshotWin snapshot; snapshot.Initialize(process, ProcessSuspensionState::kSuspended, + exception_information_address, debug_critical_section_address); - snapshot.InitializeException(exception_information_address); EXPECT_TRUE(snapshot.Exception()); EXPECT_EQ(0x517a7ed, snapshot.Exception()->Exception()); diff --git a/snapshot/win/extra_memory_ranges_test.cc b/snapshot/win/extra_memory_ranges_test.cc index b623eccf..d6d56596 100644 --- a/snapshot/win/extra_memory_ranges_test.cc +++ b/snapshot/win/extra_memory_ranges_test.cc @@ -62,7 +62,7 @@ void TestExtraMemoryRanges(TestType type, ProcessSnapshotWin snapshot; ASSERT_TRUE(snapshot.Initialize( - child.process_handle(), ProcessSuspensionState::kRunning, 0)); + child.process_handle(), ProcessSuspensionState::kRunning, 0, 0)); // Verify the extra memory ranges set via the CrashpadInfo interface. std::set> all_ranges; diff --git a/snapshot/win/process_reader_win.cc b/snapshot/win/process_reader_win.cc index da43a8a8..b7bae6ac 100644 --- a/snapshot/win/process_reader_win.cc +++ b/snapshot/win/process_reader_win.cc @@ -326,6 +326,16 @@ const ProcessInfo& ProcessReaderWin::GetProcessInfo() const { return process_info_; } +void ProcessReaderWin::DecrementThreadSuspendCounts(uint64_t except_thread_id) { + Threads(); + for (auto& thread : threads_) { + if (thread.id != except_thread_id) { + DCHECK_GT(thread.suspend_count, 0u); + --thread.suspend_count; + } + } +} + template void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) { DCHECK(threads_.empty()); diff --git a/snapshot/win/process_reader_win.h b/snapshot/win/process_reader_win.h index e2f1b01a..1794a51a 100644 --- a/snapshot/win/process_reader_win.h +++ b/snapshot/win/process_reader_win.h @@ -132,6 +132,13 @@ class ProcessReaderWin { //! \return A ProcessInfo object for the process being read. const ProcessInfo& GetProcessInfo() const; + //! \brief Decrements the thread suspend counts for all thread ids other than + //! \a except_thread_id. + //! + //! Used to adjust the thread suspend count to correspond to the actual values + //! for the process before Crashpad got involved. + void DecrementThreadSuspendCounts(uint64_t except_thread_id); + private: template void ReadThreadData(bool is_64_reading_32); diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index 4601db01..80dda80e 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -20,6 +20,7 @@ #include "base/memory/ptr_util.h" #include "base/strings/stringprintf.h" #include "base/strings/utf_string_conversions.h" +#include "snapshot/win/exception_snapshot_win.h" #include "snapshot/win/memory_snapshot_win.h" #include "snapshot/win/module_snapshot_win.h" #include "util/win/nt_internals.h" @@ -50,6 +51,7 @@ ProcessSnapshotWin::~ProcessSnapshotWin() { bool ProcessSnapshotWin::Initialize( HANDLE process, ProcessSuspensionState suspension_state, + WinVMAddress exception_information_address, WinVMAddress debug_critical_section_address) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); @@ -58,6 +60,25 @@ bool ProcessSnapshotWin::Initialize( if (!process_reader_.Initialize(process, suspension_state)) return false; + if (exception_information_address != 0) { + ExceptionInformation exception_information = {}; + if (!process_reader_.ReadMemory(exception_information_address, + sizeof(exception_information), + &exception_information)) { + LOG(WARNING) << "ReadMemory ExceptionInformation failed"; + return false; + } + + exception_.reset(new internal::ExceptionSnapshotWin()); + if (!exception_->Initialize(&process_reader_, + exception_information.thread_id, + exception_information.exception_pointers)) { + exception_.reset(); + return false; + } + } + + system_.Initialize(&process_reader_); if (process_reader_.Is64Bit()) { @@ -92,31 +113,6 @@ bool ProcessSnapshotWin::Initialize( return true; } -bool ProcessSnapshotWin::InitializeException( - WinVMAddress exception_information_address) { - INITIALIZATION_STATE_DCHECK_VALID(initialized_); - DCHECK(!exception_); - - ExceptionInformation exception_information; - if (!process_reader_.ReadMemory(exception_information_address, - sizeof(exception_information), - &exception_information)) { - LOG(WARNING) << "ReadMemory ExceptionInformation failed"; - return false; - } - - exception_.reset(new internal::ExceptionSnapshotWin()); - if (!exception_->Initialize(&process_reader_, - exception_information.thread_id, - exception_information.exception_pointers, - threads_)) { - exception_.reset(); - return false; - } - - return true; -} - void ProcessSnapshotWin::GetCrashpadOptions( CrashpadInfoClientOptions* options) { INITIALIZATION_STATE_DCHECK_VALID(initialized_); diff --git a/snapshot/win/process_snapshot_win.h b/snapshot/win/process_snapshot_win.h index 2158f062..8ae59335 100644 --- a/snapshot/win/process_snapshot_win.h +++ b/snapshot/win/process_snapshot_win.h @@ -62,6 +62,9 @@ class ProcessSnapshotWin final : public ProcessSnapshot { //! \param[in] process The handle to create a snapshot from. //! \param[in] suspension_state Whether \a process has been suspended by the //! caller. + //! \param[in] exception_information_address The address in the client + //! process's address space of an ExceptionInformation structure. May be + //! `0`, in which case no exception data will be recorded. //! \param[in] debug_critical_section_address The address in the target //! process's address space of a `CRITICAL_SECTION` allocated with valid //! `.DebugInfo`. Used as a starting point to walk the process's locks. @@ -73,23 +76,9 @@ class ProcessSnapshotWin final : public ProcessSnapshot { //! \sa ScopedProcessSuspend bool Initialize(HANDLE process, ProcessSuspensionState suspension_state, + WinVMAddress exception_information_address, WinVMAddress debug_critical_section_address); - //! \brief Initializes the object's exception. - //! - //! This populates the data to be returned by Exception(). - //! - //! This method must not be called until after a successful call to - //! Initialize(). - //! - //! \param[in] exception_information_address The address in the client - //! process's address space of an ExceptionInformation structure. - //! - //! \return `true` if the exception information could be initialized, `false` - //! otherwise with an appropriate message logged. When this method returns - //! `false`, the ProcessSnapshotWin object's validity remains unchanged. - bool InitializeException(WinVMAddress exception_information_address); - //! \brief Sets the value to be returned by ReportID(). //! //! The crash report ID is under the control of the snapshot producer, which diff --git a/snapshot/win/process_snapshot_win_test.cc b/snapshot/win/process_snapshot_win_test.cc index dc253bd2..1870a4a1 100644 --- a/snapshot/win/process_snapshot_win_test.cc +++ b/snapshot/win/process_snapshot_win_test.cc @@ -54,7 +54,7 @@ void TestImageReaderChild(const base::string16& directory_modification) { ProcessSnapshotWin process_snapshot; ASSERT_TRUE(process_snapshot.Initialize( - child.process_handle(), ProcessSuspensionState::kSuspended, 0)); + child.process_handle(), ProcessSuspensionState::kSuspended, 0, 0)); ASSERT_GE(process_snapshot.Modules().size(), 2u); diff --git a/tools/generate_dump.cc b/tools/generate_dump.cc index 39921610..11fe0ca5 100644 --- a/tools/generate_dump.cc +++ b/tools/generate_dump.cc @@ -187,6 +187,7 @@ int GenerateDumpMain(int argc, char* argv[]) { options.suspend ? ProcessSuspensionState::kSuspended : ProcessSuspensionState::kRunning, + 0, 0)) { return EXIT_FAILURE; }