diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 69df244f..843ac0f5 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -48,7 +48,8 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( ScopedProcessSuspend suspend(process); ProcessSnapshotWin process_snapshot; - if (!process_snapshot.Initialize(process)) { + if (!process_snapshot.Initialize(process, + ProcessSuspensionState::kSuspended)) { LOG(WARNING) << "ProcessSnapshotWin::Initialize failed"; return kFailedTerminationCode; } diff --git a/snapshot/crashpad_info_client_options_test.cc b/snapshot/crashpad_info_client_options_test.cc index acdc3257..933b77e1 100644 --- a/snapshot/crashpad_info_client_options_test.cc +++ b/snapshot/crashpad_info_client_options_test.cc @@ -75,7 +75,8 @@ TEST(CrashpadInfoClientOptions, OneModule) { ASSERT_TRUE(process_snapshot.Initialize(mach_task_self())); #elif defined(OS_WIN) ProcessSnapshotWin process_snapshot; - ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); #else #error Port. #endif // OS_MACOSX @@ -188,7 +189,8 @@ TEST(CrashpadInfoClientOptions, TwoModules) { ASSERT_TRUE(process_snapshot.Initialize(mach_task_self())); #elif defined(OS_WIN) ProcessSnapshotWin process_snapshot; - ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_snapshot.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); #else #error Port. #endif // OS_MACOSX diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index f6509a51..db558cde 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -26,6 +26,7 @@ #include "util/win/exception_handler_server.h" #include "util/win/registration_protocol_win.h" #include "util/win/scoped_handle.h" +#include "util/win/scoped_process_suspend.h" namespace crashpad { namespace test { @@ -62,8 +63,9 @@ class ExceptionSnapshotWinTest : public testing::Test { unsigned int ExceptionHandlerServerException( HANDLE process, WinVMAddress exception_information_address) override { + ScopedProcessSuspend suspend(process); ProcessSnapshotWin snapshot; - snapshot.Initialize(process); + snapshot.Initialize(process, ProcessSuspensionState::kSuspended); snapshot.InitializeException(exception_information_address); // Confirm the exception record was read correctly. diff --git a/snapshot/win/pe_image_annotations_reader_test.cc b/snapshot/win/pe_image_annotations_reader_test.cc index 383ada99..2c88417d 100644 --- a/snapshot/win/pe_image_annotations_reader_test.cc +++ b/snapshot/win/pe_image_annotations_reader_test.cc @@ -55,7 +55,8 @@ class TestPEImageAnnotationsReader final : public WinMultiprocess { void WinMultiprocessParent() override { ProcessReaderWin process_reader; - ASSERT_TRUE(process_reader.Initialize(ChildProcess())); + ASSERT_TRUE(process_reader.Initialize(ChildProcess(), + ProcessSuspensionState::kRunning)); // Wait for the child process to indicate that it's done setting up its // annotations via the CrashpadInfo interface. diff --git a/snapshot/win/pe_image_reader_test.cc b/snapshot/win/pe_image_reader_test.cc index d79415da..8da329e4 100644 --- a/snapshot/win/pe_image_reader_test.cc +++ b/snapshot/win/pe_image_reader_test.cc @@ -28,7 +28,8 @@ namespace { TEST(PEImageReader, DebugDirectory) { PEImageReader pe_image_reader; ProcessReaderWin process_reader; - ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); HMODULE self = reinterpret_cast(&__ImageBase); MODULEINFO module_info; ASSERT_TRUE(GetModuleInformation( diff --git a/snapshot/win/process_reader_win.cc b/snapshot/win/process_reader_win.cc index 67e59e01..bad81cd7 100644 --- a/snapshot/win/process_reader_win.cc +++ b/snapshot/win/process_reader_win.cc @@ -117,8 +117,8 @@ template void FillThreadContextAndSuspendCount( const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION& thread_info, - ProcessReaderWin::Thread* thread) { - + ProcessReaderWin::Thread* thread, + ProcessSuspensionState suspension_state) { // Don't suspend the thread if it's this thread. This is really only for test // binaries, as we won't be walking ourselves, in general. bool is_current_thread = thread_info.ClientId.UniqueThread == @@ -130,6 +130,7 @@ void FillThreadContextAndSuspendCount( // TODO(scottmg): Handle cross-bitness in this function. if (is_current_thread) { + DCHECK(suspension_state == ProcessSuspensionState::kRunning); thread->suspend_count = 0; RtlCaptureContext(&thread->context); } else { @@ -138,7 +139,11 @@ void FillThreadContextAndSuspendCount( PLOG(ERROR) << "SuspendThread failed"; return; } - thread->suspend_count = previous_suspend_count; + DCHECK(previous_suspend_count > 0 || + suspension_state == ProcessSuspensionState::kRunning); + thread->suspend_count = + previous_suspend_count - + (suspension_state == ProcessSuspensionState::kSuspended ? 1 : 0); memset(&thread->context, 0, sizeof(thread->context)); thread->context.ContextFlags = CONTEXT_ALL; @@ -171,6 +176,7 @@ ProcessReaderWin::ProcessReaderWin() process_info_(), threads_(), modules_(), + suspension_state_(), initialized_threads_(false), initialized_() { } @@ -178,10 +184,12 @@ ProcessReaderWin::ProcessReaderWin() ProcessReaderWin::~ProcessReaderWin() { } -bool ProcessReaderWin::Initialize(HANDLE process) { +bool ProcessReaderWin::Initialize(HANDLE process, + ProcessSuspensionState suspension_state) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); process_ = process; + suspension_state_ = suspension_state; process_info_.Initialize(process); INITIALIZATION_STATE_SET_VALID(initialized_); @@ -254,7 +262,7 @@ const std::vector& ProcessReaderWin::Threads() { Thread thread; thread.id = thread_info.ClientId.UniqueThread; - FillThreadContextAndSuspendCount(thread_info, &thread); + FillThreadContextAndSuspendCount(thread_info, &thread, suspension_state_); // TODO(scottmg): I believe we could reverse engineer the PriorityClass from // the Priority, BasePriority, and diff --git a/snapshot/win/process_reader_win.h b/snapshot/win/process_reader_win.h index 3ef738ad..3f49f335 100644 --- a/snapshot/win/process_reader_win.h +++ b/snapshot/win/process_reader_win.h @@ -26,6 +26,15 @@ namespace crashpad { +//! \brief State of process being read by ProcessReaderWin. +enum class ProcessSuspensionState : bool { + //! \brief The process has not been suspended. + kRunning, + + //! \brief The process is suspended. + kSuspended, +}; + //! \brief Accesses information about another process, identified by a `HANDLE`. class ProcessReaderWin { public: @@ -52,11 +61,17 @@ class ProcessReaderWin { //! //! \param[in] process Process handle, must have `PROCESS_QUERY_INFORMATION`, //! `PROCESS_VM_READ`, and `PROCESS_DUP_HANDLE` access. + //! \param[in] suspension_state Whether \a process has already been suspended + //! by the caller. Typically, this will be + //! ProcessSuspensionState::kSuspended, except for testing uses and where + //! the reader is reading itself. //! //! \return `true` on success, indicating that this object will respond //! validly to further method calls. `false` on failure. On failure, no //! further method calls should be made. - bool Initialize(HANDLE process); + //! + //! \sa ScopedProcessSuspend + bool Initialize(HANDLE process, ProcessSuspensionState suspension_state); //! \return `true` if the target task is a 64-bit process. bool Is64Bit() const { return process_info_.Is64Bit(); } @@ -96,6 +111,7 @@ class ProcessReaderWin { ProcessInfo process_info_; std::vector threads_; std::vector modules_; + ProcessSuspensionState suspension_state_; bool initialized_threads_; InitializationStateDcheck initialized_; diff --git a/snapshot/win/process_reader_win_test.cc b/snapshot/win/process_reader_win_test.cc index 23306315..1cb4bdda 100644 --- a/snapshot/win/process_reader_win_test.cc +++ b/snapshot/win/process_reader_win_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" #include "test/win/win_multiprocess.h" +#include "util/win/scoped_process_suspend.h" namespace crashpad { namespace test { @@ -26,7 +27,8 @@ namespace { TEST(ProcessReaderWin, SelfBasic) { ProcessReaderWin process_reader; - ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); #if !defined(ARCH_CPU_64_BITS) EXPECT_FALSE(process_reader.Is64Bit()); @@ -55,7 +57,8 @@ class ProcessReaderChild final : public WinMultiprocess { private: void WinMultiprocessParent() override { ProcessReaderWin process_reader; - ASSERT_TRUE(process_reader.Initialize(ChildProcess())); + ASSERT_TRUE(process_reader.Initialize(ChildProcess(), + ProcessSuspensionState::kRunning)); #if !defined(ARCH_CPU_64_BITS) EXPECT_FALSE(process_reader.Is64Bit()); @@ -90,7 +93,8 @@ TEST(ProcessReaderWin, ChildBasic) { TEST(ProcessReaderWin, SelfOneThread) { ProcessReaderWin process_reader; - ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_reader.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); const std::vector& threads = process_reader.Threads(); @@ -110,6 +114,56 @@ TEST(ProcessReaderWin, SelfOneThread) { EXPECT_EQ(0, threads[0].suspend_count); } +class ProcessReaderChildThreadSuspendCount final : public WinMultiprocess { + public: + ProcessReaderChildThreadSuspendCount() : WinMultiprocess() {} + ~ProcessReaderChildThreadSuspendCount() {} + + private: + void WinMultiprocessParent() override { + { + ProcessReaderWin process_reader; + ASSERT_TRUE(process_reader.Initialize(ChildProcess(), + ProcessSuspensionState::kRunning)); + + const auto& threads = process_reader.Threads(); + ASSERT_FALSE(threads.empty()); + for (const auto& thread : threads) + EXPECT_EQ(0u, thread.suspend_count); + } + + { + ScopedProcessSuspend suspend(ChildProcess()); + + ProcessReaderWin process_reader; + ASSERT_TRUE(process_reader.Initialize( + ChildProcess(), ProcessSuspensionState::kSuspended)); + + // Confirm that thread counts are adjusted correctly for the process being + // suspended. + const auto& threads = process_reader.Threads(); + ASSERT_FALSE(threads.empty()); + for (const auto& thread : threads) + EXPECT_EQ(0u, thread.suspend_count); + } + } + + void WinMultiprocessChild() override { + WinVMAddress address = reinterpret_cast(kTestMemory); + CheckedWriteFile(WritePipeHandle(), &address, sizeof(address)); + + // Wait for the parent to signal that it's OK to exit by closing its end of + // the pipe. + CheckedReadFileAtEOF(ReadPipeHandle()); + } + + DISALLOW_COPY_AND_ASSIGN(ProcessReaderChildThreadSuspendCount); +}; + +TEST(ProcessReaderWin, ChildThreadSuspendCounts) { + WinMultiprocess::Run(); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index cb0c0d3a..d468c6ce 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -38,12 +38,13 @@ ProcessSnapshotWin::ProcessSnapshotWin() ProcessSnapshotWin::~ProcessSnapshotWin() { } -bool ProcessSnapshotWin::Initialize(HANDLE process) { +bool ProcessSnapshotWin::Initialize(HANDLE process, + ProcessSuspensionState suspension_state) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); GetTimeOfDay(&snapshot_time_); - if (!process_reader_.Initialize(process)) + if (!process_reader_.Initialize(process, suspension_state)) return false; system_.Initialize(&process_reader_); diff --git a/snapshot/win/process_snapshot_win.h b/snapshot/win/process_snapshot_win.h index 6cb4976d..5c0b1b35 100644 --- a/snapshot/win/process_snapshot_win.h +++ b/snapshot/win/process_snapshot_win.h @@ -52,10 +52,14 @@ class ProcessSnapshotWin final : public ProcessSnapshot { //! \brief Initializes the object. //! //! \param[in] process The handle to create a snapshot from. + //! \param[in] suspension_state Whether \a process has been suspended by the + //! caller. //! //! \return `true` if the snapshot could be created, `false` otherwise with //! an appropriate message logged. - bool Initialize(HANDLE process); + //! + //! \sa ScopedProcessSuspend + bool Initialize(HANDLE process, ProcessSuspensionState suspension_state); //! \brief Initializes the object's exception. //! diff --git a/snapshot/win/system_snapshot_win_test.cc b/snapshot/win/system_snapshot_win_test.cc index 7552ca56..735b2f45 100644 --- a/snapshot/win/system_snapshot_win_test.cc +++ b/snapshot/win/system_snapshot_win_test.cc @@ -41,7 +41,8 @@ class SystemSnapshotWinTest : public testing::Test { // testing::Test: void SetUp() override { - ASSERT_TRUE(process_reader_.Initialize(GetCurrentProcess())); + ASSERT_TRUE(process_reader_.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)); system_snapshot_.Initialize(&process_reader_); } diff --git a/tools/generate_dump.cc b/tools/generate_dump.cc index f68c17c2..c68d214a 100644 --- a/tools/generate_dump.cc +++ b/tools/generate_dump.cc @@ -182,7 +182,10 @@ int GenerateDumpMain(int argc, char* argv[]) { } #elif defined(OS_WIN) ProcessSnapshotWin process_snapshot; - if (!process_snapshot.Initialize(process.get())) { + if (!process_snapshot.Initialize(process.get(), + options.suspend + ? ProcessSuspensionState::kSuspended + : ProcessSuspensionState::kRunning)) { return EXIT_FAILURE; } #endif // OS_MACOSX