win: Capture some memory pointed at by context

R=mark@chromium.org
BUG=crashpad:86, chromium:571144

Review URL: https://codereview.chromium.org/1533183002 .

handler/win/crashy_test_program.cc

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <intrin.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <sys/types.h>
@@ -39,6 +40,11 @@ namespace {
 
 CRITICAL_SECTION g_test_critical_section;
 
+unsigned char g_test_memory[] = {
+  99, 98, 97, 96, 95, 94, 93, 92, 91, 90,
+  89, 88, 87, 86, 85, 84, 83, 82, 81, 80,
+};
+
 ULONG RtlNtStatusToDosError(NTSTATUS status) {
   static const auto rtl_nt_status_to_dos_error =
       GET_FUNCTION_REQUIRED(L"ntdll.dll", ::RtlNtStatusToDosError);
@@ -90,6 +96,12 @@ void SomeCrashyFunction() {
   // LastStatusError of the TEB as a side-effect, and we'll be setting
   // ERROR_FILE_NOT_FOUND for GetLastError().
   SetLastError(RtlNtStatusToDosError(STATUS_NO_SUCH_FILE));
+
+  // Set a register to point at some memory we can test to confirm it makes it
+  // into the minidump. We use __movsb as a way to set SI/DI without needing an
+  // external .asm file.
+  __movsb(g_test_memory, g_test_memory, 0);
+
   volatile int* foo = reinterpret_cast<volatile int*>(7);
   *foo = 42;
 }

minidump/minidump_exception_writer.h

@@ -31,6 +31,7 @@ namespace crashpad {
 
 class ExceptionSnapshot;
 class MinidumpContextWriter;
+class MinidumpMemoryListWriter;
 
 //! \brief The writer for a MINIDUMP_EXCEPTION_STREAM stream in a minidump file.
 class MinidumpExceptionWriter final : public internal::MinidumpStreamWriter {

minidump/minidump_file_writer.cc

@@ -28,6 +28,7 @@
 #include "minidump/minidump_thread_id_map.h"
 #include "minidump/minidump_thread_writer.h"
 #include "minidump/minidump_writer_util.h"
+#include "snapshot/exception_snapshot.h"
 #include "snapshot/process_snapshot.h"
 #include "util/file/file_writer.h"
 #include "util/numeric/safe_assignment.h"
@@ -119,6 +120,8 @@ void MinidumpFileWriter::InitializeFromSnapshot(
   }
 
   memory_list->AddFromSnapshot(process_snapshot->ExtraMemory());
+  if (exception_snapshot)
+    memory_list->AddFromSnapshot(exception_snapshot->ExtraMemory());
 
   AddStream(std::move(memory_list));
 }

snapshot/exception_snapshot.h

@@ -19,6 +19,8 @@
 
 #include <vector>
 
+#include "snapshot/memory_snapshot.h"
+
 namespace crashpad {
 
 struct CPUContext;
@@ -103,6 +105,15 @@ class ExceptionSnapshot {
   //! `RaiseException()`. See the documentation for `ExceptionInformation` in
   //! `EXCEPTION_RECORD`.
   virtual const std::vector<uint64_t>& Codes() const = 0;
+
+  //! \brief Returns a vector of additional memory blocks that should be
+  //!     included in a minidump.
+  //!
+  //! \return A vector of MemorySnapshot objects that will be included in the
+  //!     crash dump. The caller does not take ownership of these objects, they
+  //!     are scoped to the lifetime of the ThreadSnapshot object that they
+  //!     were obtained from.
+  virtual std::vector<const MemorySnapshot*> ExtraMemory() const = 0;
 };
 
 }  // namespace crashpad

snapshot/mac/exception_snapshot_mac.cc

@@ -249,5 +249,10 @@ const std::vector<uint64_t>& ExceptionSnapshotMac::Codes() const {
   return codes_;
 }
 
+std::vector<const MemorySnapshot*> ExceptionSnapshotMac::ExtraMemory() const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  return std::vector<const MemorySnapshot*>();
+}
+
 }  // namespace internal
 }  // namespace crashpad

snapshot/mac/exception_snapshot_mac.h

@@ -68,6 +68,7 @@ class ExceptionSnapshotMac final : public ExceptionSnapshot {
   uint32_t ExceptionInfo() const override;
   uint64_t ExceptionAddress() const override;
   const std::vector<uint64_t>& Codes() const override;
+  virtual std::vector<const MemorySnapshot*> ExtraMemory() const override;
 
  private:
 #if defined(ARCH_CPU_X86_FAMILY)

snapshot/snapshot.gyp

@@ -89,6 +89,8 @@
         'process_snapshot.h',
         'system_snapshot.h',
         'thread_snapshot.h',
+        'win/capture_context_memory.cc',
+        'win/capture_context_memory.h',
         'win/cpu_context_win.cc',
         'win/cpu_context_win.h',
         'win/exception_snapshot_win.cc',

snapshot/test/test_exception_snapshot.cc

@@ -55,5 +55,12 @@ const std::vector<uint64_t>& TestExceptionSnapshot::Codes() const {
   return codes_;
 }
 
+std::vector<const MemorySnapshot*> TestExceptionSnapshot::ExtraMemory() const {
+  std::vector<const MemorySnapshot*> extra_memory;
+  for (const auto& em : extra_memory_)
+    extra_memory.push_back(em);
+  return extra_memory;
+}
+
 }  // namespace test
 }  // namespace crashpad

snapshot/test/test_exception_snapshot.h

@@ -20,8 +20,10 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
 #include "snapshot/cpu_context.h"
 #include "snapshot/exception_snapshot.h"
+#include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
 namespace test {
@@ -57,6 +59,9 @@ class TestExceptionSnapshot final : public ExceptionSnapshot {
     exception_address_ = exception_address;
   }
   void SetCodes(const std::vector<uint64_t>& codes) { codes_ = codes; }
+  void AddExtraMemory(scoped_ptr<MemorySnapshot> extra_memory) {
+    extra_memory_.push_back(extra_memory.release());
+  }
 
   // ExceptionSnapshot:
 
@@ -66,6 +71,7 @@ class TestExceptionSnapshot final : public ExceptionSnapshot {
   uint32_t ExceptionInfo() const override;
   uint64_t ExceptionAddress() const override;
   const std::vector<uint64_t>& Codes() const override;
+  std::vector<const MemorySnapshot*> ExtraMemory() const override;
 
  private:
   union {
@@ -78,6 +84,7 @@ class TestExceptionSnapshot final : public ExceptionSnapshot {
   uint32_t exception_info_;
   uint64_t exception_address_;
   std::vector<uint64_t> codes_;
+  PointerVector<MemorySnapshot> extra_memory_;
 
   DISALLOW_COPY_AND_ASSIGN(TestExceptionSnapshot);
 };

snapshot/win/capture_context_memory.cc

@@ -0,0 +1,103 @@
+// Copyright 2016 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,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "snapshot/win/capture_context_memory.h"
+
+#include <stdint.h>
+
+#include <limits>
+
+#include "snapshot/win/memory_snapshot_win.h"
+#include "snapshot/win/process_reader_win.h"
+
+namespace crashpad {
+namespace internal {
+
+namespace {
+
+void MaybeCaptureMemoryAround(ProcessReaderWin* process_reader,
+                              WinVMAddress address,
+                              PointerVector<MemorySnapshotWin>* into) {
+  const WinVMAddress non_address_offset = 0x10000;
+  if (address < non_address_offset)
+    return;
+  if (process_reader->Is64Bit()) {
+    if (address >= std::numeric_limits<uint64_t>::max() - non_address_offset)
+      return;
+  } else {
+    if (address >= std::numeric_limits<uint32_t>::max() - non_address_offset)
+      return;
+  }
+
+  const WinVMSize kRegisterByteOffset = 32;
+  const WinVMAddress target = address - kRegisterByteOffset;
+  const WinVMSize size = 128;
+  auto ranges = process_reader->GetProcessInfo().GetReadableRanges(
+      CheckedRange<WinVMAddress, WinVMSize>(target, size));
+  for (const auto& range : ranges) {
+    internal::MemorySnapshotWin* snapshot = new internal::MemorySnapshotWin();
+    snapshot->Initialize(process_reader, range.base(), range.size());
+    into->push_back(snapshot);
+  }
+}
+
+}  // namespace
+
+void CaptureMemoryPointedToByContext(const CPUContext& context,
+                                     ProcessReaderWin* process_reader,
+                                     const ProcessReaderWin::Thread& thread,
+                                     PointerVector<MemorySnapshotWin>* into) {
+#if defined(ARCH_CPU_X86_64)
+  if (context.architecture == kCPUArchitectureX86_64) {
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rax, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rbx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rcx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rdx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rdi, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rsi, into);
+    if (context.x86_64->rbp < thread.stack_region_address ||
+        context.x86_64->rbp >=
+            thread.stack_region_address + thread.stack_region_size) {
+      MaybeCaptureMemoryAround(process_reader, context.x86_64->rbp, into);
+    }
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r8, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r9, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r10, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r11, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r12, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r13, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r14, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->r15, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86_64->rip, into);
+  } else {
+#endif
+    MaybeCaptureMemoryAround(process_reader, context.x86->eax, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86->ebx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86->ecx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86->edx, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86->edi, into);
+    MaybeCaptureMemoryAround(process_reader, context.x86->esi, into);
+    if (context.x86->ebp < thread.stack_region_address ||
+        context.x86->ebp >=
+            thread.stack_region_address + thread.stack_region_size) {
+      MaybeCaptureMemoryAround(process_reader, context.x86->ebp, into);
+    }
+    MaybeCaptureMemoryAround(process_reader, context.x86->eip, into);
+#if defined(ARCH_CPU_X86_64)
+  }
+#endif
+}
+
+}  // namespace internal
+}  // namespace crashpad

snapshot/win/capture_context_memory.h

@@ -0,0 +1,43 @@
+// Copyright 2016 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,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_SNAPSHOT_WIN_CAPTURE_CONTEXT_MEMORY_H_
+#define CRASHPAD_SNAPSHOT_WIN_CAPTURE_CONTEXT_MEMORY_H_
+
+#include "snapshot/cpu_context.h"
+#include "snapshot/win/process_reader_win.h"
+#include "util/stdlib/pointer_container.h"
+
+namespace crashpad {
+namespace internal {
+
+class MemorySnapshotWin;
+
+//! \brief For all registers that appear to be pointer-like in \a context,
+//!     captures a small amount of memory near their pointed to location.
+//!
+//! \param[in] context The context to inspect.
+//! \param[in] process_reader A ProcessReaderWin to read from the target
+//!     process.
+//! \param[in] thread The thread to which the context belongs.
+//! \param[out] into A vector of pointers to append new ranges to.
+void CaptureMemoryPointedToByContext(const CPUContext& context,
+                                     ProcessReaderWin* process_reader,
+                                     const ProcessReaderWin::Thread& thread,
+                                     PointerVector<MemorySnapshotWin>* into);
+
+}  // namespace internal
+}  // namespace crashpad
+
+#endif  // CRASHPAD_SNAPSHOT_WIN_CAPTURE_CONTEXT_MEMORY_H_

snapshot/win/end_to_end_test.py

@@ -247,6 +247,10 @@ def RunTests(cdb_path,
                 r'FreeOwnStackAndBreak.*\nquit:',
             'at correct location, no additional stack entries')
 
+  out = CdbRun(cdb_path, dump_path, '.ecxr; db /c14 edi')
+  out.Check(r'63 62 61 60 5f 5e 5d 5c-5b 5a 59 58 57 56 55 54 53 52 51 50',
+            'data pointed to by registers captured')
+
   if z7_dump_path:
     out = CdbRun(cdb_path, z7_dump_path, '.ecxr;lm')
     out.Check('This dump file has an exception of interest stored in it',

snapshot/win/exception_snapshot_win.cc

@@ -14,7 +14,10 @@
 
 #include "snapshot/win/exception_snapshot_win.h"
 
+#include "snapshot/memory_snapshot.h"
+#include "snapshot/win/capture_context_memory.h"
 #include "snapshot/win/cpu_context_win.h"
+#include "snapshot/win/memory_snapshot_win.h"
 #include "snapshot/win/process_reader_win.h"
 #include "util/win/nt_internals.h"
 
@@ -41,15 +44,15 @@ bool ExceptionSnapshotWin::Initialize(ProcessReaderWin* process_reader,
                                       WinVMAddress exception_pointers_address) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
-  bool found_thread = false;
+  const ProcessReaderWin::Thread* thread = nullptr;
   for (const auto& loop_thread : process_reader->Threads()) {
     if (thread_id == loop_thread.id) {
-      found_thread = true;
+      thread = &loop_thread;
       break;
     }
   }
 
-  if (!found_thread) {
+  if (!thread) {
     LOG(ERROR) << "thread ID " << thread_id << " not found in process";
     return false;
   } else {
@@ -86,6 +89,9 @@ bool ExceptionSnapshotWin::Initialize(ProcessReaderWin* process_reader,
     InitializeX86Context(context_record, context_.x86);
   }
 
+  CaptureMemoryPointedToByContext(
+      context_, process_reader, *thread, &extra_memory_);
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
@@ -120,6 +126,15 @@ const std::vector<uint64_t>& ExceptionSnapshotWin::Codes() const {
   return codes_;
 }
 
+std::vector<const MemorySnapshot*> ExceptionSnapshotWin::ExtraMemory() const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  std::vector<const MemorySnapshot*> result;
+  result.reserve(extra_memory_.size());
+  for (const auto& em : extra_memory_)
+    result.push_back(em);
+  return result;
+}
+
 template <class ExceptionRecordType,
           class ExceptionPointersType,
           class ContextType>

snapshot/win/exception_snapshot_win.h

@@ -23,6 +23,7 @@
 #include "snapshot/cpu_context.h"
 #include "snapshot/exception_snapshot.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/stdlib/pointer_container.h"
 #include "util/win/address_types.h"
 #include "util/win/process_structs.h"
 
@@ -32,6 +33,8 @@ class ProcessReaderWin;
 
 namespace internal {
 
+class MemorySnapshotWin;
+
 class ExceptionSnapshotWin final : public ExceptionSnapshot {
  public:
   ExceptionSnapshotWin();
@@ -60,6 +63,7 @@ class ExceptionSnapshotWin final : public ExceptionSnapshot {
   uint32_t ExceptionInfo() const override;
   uint64_t ExceptionAddress() const override;
   const std::vector<uint64_t>& Codes() const override;
+  std::vector<const MemorySnapshot*> ExtraMemory() const override;
 
  private:
   template <class ExceptionRecordType,
@@ -77,6 +81,7 @@ class ExceptionSnapshotWin final : public ExceptionSnapshot {
 #endif
   CPUContext context_;
   std::vector<uint64_t> codes_;
+  PointerVector<internal::MemorySnapshotWin> extra_memory_;
   uint64_t thread_id_;
   uint64_t exception_address_;
   uint32_t exception_flags_;

snapshot/win/thread_snapshot_win.cc

@@ -17,6 +17,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "snapshot/win/capture_context_memory.h"
 #include "snapshot/win/cpu_context_win.h"
 #include "snapshot/win/process_reader_win.h"
 
@@ -75,6 +76,9 @@ bool ThreadSnapshotWin::Initialize(
   InitializeX86Context(process_reader_thread.context.native, context_.x86);
 #endif  // ARCH_CPU_X86_64
 
+  CaptureMemoryPointedToByContext(
+      context_, process_reader, thread_, &pointed_to_memory_);
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
@@ -111,9 +115,13 @@ uint64_t ThreadSnapshotWin::ThreadSpecificDataAddress() const {
 
 std::vector<const MemorySnapshot*> ThreadSnapshotWin::ExtraMemory() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
-  // TODO(scottmg): Ensure this region is readable, and make sure we don't
-  // discard the entire dump if it isn't. https://crashpad.chromium.org/bug/59
-  return std::vector<const MemorySnapshot*>(1, &teb_);
+  std::vector<const MemorySnapshot*> result;
+  result.reserve(1 + pointed_to_memory_.size());
+  result.push_back(&teb_);
+  std::copy(pointed_to_memory_.begin(),
+            pointed_to_memory_.end(),
+            std::back_inserter(result));
+  return result;
 }
 
 }  // namespace internal

snapshot/win/thread_snapshot_win.h

@@ -76,6 +76,7 @@ class ThreadSnapshotWin final : public ThreadSnapshot {
   internal::MemorySnapshotWin teb_;
   ProcessReaderWin::Thread thread_;
   InitializationStateDcheck initialized_;
+  PointerVector<internal::MemorySnapshotWin> pointed_to_memory_;
 
   DISALLOW_COPY_AND_ASSIGN(ThreadSnapshotWin);
 };