win: Fix incorrect thread suspend count due to ScopedProcessSuspend

snapshot/win/process_reader_win.cc

 // Copyright 2015 The Crashpad Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
 // You may obtain a copy of the License at
 //
 //     http://www.apache.org/licenses/LICENSE-2.0
 //
 // Unless required by applicable law or agreed to in writing, software
 // distributed under the License is distributed on an "AS IS" BASIS,
@@ skipping 99 matching lines @@
   return handle;
 }
 
 // It's necessary to suspend the thread to grab CONTEXT. SuspendThread has a
 // side-effect of returning the SuspendCount of the thread on success, so we
 // fill out these two pieces of semi-unrelated data in the same function.
 template <class Traits>
 void FillThreadContextAndSuspendCount(
     const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<Traits>&
         thread_info,
-    ProcessReaderWin::Thread* thread) {
-
+    ProcessReaderWin::Thread* thread,
+    ProcessSuspensionState process_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 ==
                            reinterpret_cast<process_types::TEB<Traits>*>(
                                NtCurrentTeb())->ClientId.UniqueThread;
 
   ScopedKernelHANDLE thread_handle(OpenThread(thread_info));
 
   // TODO(scottmg): Handle cross-bitness in this function.
 
   if (is_current_thread) {
     thread->suspend_count = 0;

[Mark Mentovai, 2015/09/09 18:57:27]

LOG(ERROR) or DCHECK() if the suspension state is kSuspended here too.

[scottmg, 2015/09/09 19:13:25]

Done.

     RtlCaptureContext(&thread->context);
   } else {
     DWORD previous_suspend_count = SuspendThread(thread_handle.get());
     if (previous_suspend_count == -1) {
       PLOG(ERROR) << "SuspendThread failed";
       return;
     }
-    thread->suspend_count = previous_suspend_count;
+    if (previous_suspend_count == 0 &&
+        process_state == ProcessSuspensionState::kSuspended) {
+      LOG(ERROR) << "suspend count was 0, but process should be suspended";

[Mark Mentovai, 2015/09/09 18:57:27]

Upgrade (sidegrade) to a DCHECK()? This indicates a gross logic error.

[scottmg, 2015/09/09 19:13:25]

Done.

+    }
+    thread->suspend_count =
+        previous_suspend_count -
+        (process_state == ProcessSuspensionState::kSuspended ? 1 : 0);
 
     memset(&thread->context, 0, sizeof(thread->context));
     thread->context.ContextFlags = CONTEXT_ALL;
     if (!GetThreadContext(thread_handle.get(), &thread->context)) {
       PLOG(ERROR) << "GetThreadContext failed";
       return;
     }
 
     if (!ResumeThread(thread_handle.get())) {
       PLOG(ERROR) << "ResumeThread failed";
@@ skipping 12 matching lines @@
       suspend_count(0),
       priority_class(0),
       priority(0) {
 }
 
 ProcessReaderWin::ProcessReaderWin()
     : process_(INVALID_HANDLE_VALUE),
       process_info_(),
       threads_(),
       modules_(),
+      process_state_(),
       initialized_threads_(false),
       initialized_() {
 }
 
 ProcessReaderWin::~ProcessReaderWin() {
 }
 
-bool ProcessReaderWin::Initialize(HANDLE process) {
+bool ProcessReaderWin::Initialize(HANDLE process,
+                                  ProcessSuspensionState process_state) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   process_ = process;
+  process_state_ = process_state;
   process_info_.Initialize(process);
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
 
 bool ProcessReaderWin::ReadMemory(WinVMAddress at,
                                   WinVMSize num_bytes,
                                   void* into) {
   SIZE_T bytes_read;
@@ skipping 52 matching lines @@
       GetProcessInformation<SizeTraits>(process_, &buffer);
   if (!process_information)
     return threads_;
 
   for (unsigned long i = 0; i < process_information->NumberOfThreads; ++i) {
     const process_types::SYSTEM_EXTENDED_THREAD_INFORMATION<SizeTraits>&
         thread_info = process_information->Threads[i];
     Thread thread;
     thread.id = thread_info.ClientId.UniqueThread;
 
-    FillThreadContextAndSuspendCount(thread_info, &thread);
+    FillThreadContextAndSuspendCount(thread_info, &thread, process_state_);
 
     // TODO(scottmg): I believe we could reverse engineer the PriorityClass from
     // the Priority, BasePriority, and
     // https://msdn.microsoft.com/en-us/library/windows/desktop/ms685100 .
     // MinidumpThreadWriter doesn't handle it yet in any case, so investigate
     // both of those at the same time if it's useful.
     thread.priority_class = NORMAL_PRIORITY_CLASS;
 
     thread.priority = thread_info.Priority;
     thread.teb = thread_info.TebBase;
@@ skipping 19 matching lines @@
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
 
   if (!process_info_.Modules(&modules_)) {
     LOG(ERROR) << "couldn't retrieve modules";
   }
 
   return modules_;
 }
 
 }  // namespace crashpad