From d7f90b45b6318eb3e54002aa52bfd579e5346852 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 9 Sep 2015 12:29:29 -0700 Subject: [PATCH] win: Fix incorrect thread suspend count due to ScopedProcessSuspend After https://codereview.chromium.org/1303173011/, the thread suspend count would be one too large because the count is adjusted when the process is suspended. Counteract this by passing in whether the process is suspended or not so that the thread's suspension count can be adjusted. Add a test to sanity-check thread suspend count. R=mark@chromium.org Review URL: https://codereview.chromium.org/1326443007 . --- handler/win/crash_report_exception_handler.cc | 3 +- snapshot/crashpad_info_client_options_test.cc | 6 +- snapshot/win/exception_snapshot_win_test.cc | 4 +- .../win/pe_image_annotations_reader_test.cc | 3 +- snapshot/win/pe_image_reader_test.cc | 3 +- snapshot/win/process_reader_win.cc | 18 ++++-- snapshot/win/process_reader_win.h | 18 +++++- snapshot/win/process_reader_win_test.cc | 60 ++++++++++++++++++- snapshot/win/process_snapshot_win.cc | 5 +- snapshot/win/process_snapshot_win.h | 6 +- snapshot/win/system_snapshot_win_test.cc | 3 +- tools/generate_dump.cc | 5 +- 12 files changed, 114 insertions(+), 20 deletions(-) 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