win: Adjust thread suspend count for DumpAndCrashTargetProcess() case

Because DumpAndCrashTargetProcess() suspends the process, the thread
suspend count is one too high for all threads other than the injection
one in the thread snapshots. Compensate for this when we detect this
type of exception.

BUG=crashpad:103

Change-Id: Ib77112fddf5324fc0e43f598604e56f77d67ff54
Reviewed-on: https://chromium-review.googlesource.com/340372
Reviewed-by: Mark Mentovai <mark@chromium.org>

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 =

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

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',

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,
+    WinVMAddress exception_pointers_address) {
-    const PointerVector<internal::ThreadSnapshotWin>& threads) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   const ProcessReaderWin::Thread* thread = nullptr;
@@ -98,9 +97,9 @@ bool ExceptionSnapshotWin::Initialize(
   if (is_64_bit) {
     if (!InitializeFromExceptionPointers<EXCEPTION_RECORD64,
                                          process_types::EXCEPTION_POINTERS64>(
-            *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<EXCEPTION_RECORD32,
                                          process_types::EXCEPTION_POINTERS32>(
-            *process_reader,
+            process_reader,
             exception_pointers_address,
-            threads,
+            thread_id,
             &NativeContextToCPUContext32)) {
       return false;
     }
@@ -168,16 +167,16 @@ template <class ExceptionRecordType,
           class ExceptionPointersType,
           class ContextType>
 bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
-    const ProcessReaderWin& process_reader,
+    ProcessReaderWin* process_reader,
     WinVMAddress exception_pointers_address,
-    const PointerVector<internal::ThreadSnapshotWin>& 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,
+  if (!process_reader->ReadMemory(exception_pointers_address,
-                                 sizeof(exception_pointers),
+                                  sizeof(exception_pointers),
-                                 &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<WinVMAddress>(exception_pointers.ExceptionRecord),
           sizeof(first_record),
           &first_record)) {
@@ -195,9 +194,13 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
     return false;
   }
 
-  if (first_record.ExceptionCode == CrashpadClient::kTriggeredExceptionCode &&
+  const bool triggered_by_client =
-      first_record.NumberParameters == 2 &&
+      first_record.ExceptionCode == CrashpadClient::kTriggeredExceptionCode &&
-      first_record.ExceptionInformation[0] != 0) {
+      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<DWORD>(first_record.ExceptionInformation[1]);
     exception_flags_ = EXCEPTION_NONCONTINUABLE;
-    for (const auto* thread : threads) {
+    for (const auto& thread : process_reader->Threads()) {
-      if (thread->ThreadID() == thread_id) {
+      if (thread.id == thread_id) {
         thread_id_ = thread_id;
-        exception_address_ = thread->Context()->InstructionPointer();
+        native_to_cpu_context(
-        context_.architecture = thread->Context()->architecture;
+            *reinterpret_cast<const ContextType*>(&thread.context),
-        if (context_.architecture == kCPUArchitectureX86_64) {
+            &context_,
-          context_union_.x86_64 = *thread->Context()->x86_64;
+            &context_union_);
-          context_.x86_64 = &context_union_.x86_64;
+        exception_address_ = context_.InstructionPointer();
-        } else {
-          context_union_.x86 = *thread->Context()->x86;
-          context_.x86 = &context_union_.x86;
-        }
         break;
       }
     }
@@ -239,7 +238,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
     }
 
     ContextType context_record;
-    if (!process_reader.ReadMemory(
+    if (!process_reader->ReadMemory(
             static_cast<WinVMAddress>(exception_pointers.ContextRecord),
             sizeof(context_record),
             &context_record)) {

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,
+                  WinVMAddress exception_pointers);
-                  const PointerVector<internal::ThreadSnapshotWin>& threads);
 
   // 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<internal::ThreadSnapshotWin>& threads,
+      DWORD thread_id,
       void (*native_to_cpu_context)(const ContextType& context_record,
                                     CPUContext* context,
                                     CPUContextUnion* context_union));

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());
 

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<CheckedRange<uint64_t>> all_ranges;

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 <class Traits>
 void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) {
   DCHECK(threads_.empty());

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 <class Traits>
   void ReadThreadData(bool is_64_reading_32);

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_);

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

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);
 

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;
     }