win: Save contents of TEBs allowing !teb and !gle to work in windbg

crashy_program's log looks something like this now:

0:000> .ecxr
eax=00000007 ebx=7f24e000 ecx=7f24d000 edx=00000000 esi=00497ec8 edi=00d39ca0
eip=00cf5d12 esp=001ffcd8 ebp=001ffcdc iopl=0         nv up ei ng nz ac po cy
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010293
crashy_program+0x5d12:
00cf5d12 ??              ???
0:000> !teb
TEB at 7f24d000
    ExceptionList:        001ff548
    StackBase:            00200000
    StackLimit:           001fd000
    SubSystemTib:         00000000
    FiberData:            00001e00
    ArbitraryUserPointer: 00000000
    Self:                 7f24d000
    EnvironmentPointer:   00000000
    ClientId:             00003658 . 00004630
    RpcHandle:            00000000
    Tls Storage:          7f24d02c
    PEB Address:          7f24e000
    LastErrorValue:       2
    LastStatusValue:      c000000f
    Count Owned Locks:    0
    HardErrorMode:        0
0:000> !gle
LastErrorValue: (Win32) 0x2 (2) - The system cannot find the file specified.
LastStatusValue: (NTSTATUS) 0xc000000f - {File Not Found}  The file %hs does not exist.

R=mark@chromium.org
BUG=crashpad:46

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

handler/win/crashy_test_program.cc

@@ -12,15 +12,35 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "client/crashpad_client.h"
+#include <windows.h>
+#include <winternl.h>
+
+// ntstatus.h conflicts with windows.h so define this locally.
+#ifndef STATUS_NO_SUCH_FILE
+#define STATUS_NO_SUCH_FILE static_cast<NTSTATUS>(0xC000000F)
+#endif
 
 #include "base/logging.h"
+#include "client/crashpad_client.h"
 #include "tools/tool_support.h"
 
 namespace crashpad {
 namespace {
 
+ULONG RtlNtStatusToDosError(NTSTATUS status) {
+  static decltype(::RtlNtStatusToDosError)* rtl_nt_status_to_dos_error =
+      reinterpret_cast<decltype(::RtlNtStatusToDosError)*>(
+          GetProcAddress(LoadLibrary(L"ntdll.dll"), "RtlNtStatusToDosError"));
+  DCHECK(rtl_nt_status_to_dos_error);
+  return rtl_nt_status_to_dos_error(status);
+}
+
 void SomeCrashyFunction() {
+  // SetLastError and NTSTATUS so that we have something to view in !gle in
+  // windbg. RtlNtStatusToDosError() stores STATUS_NO_SUCH_FILE into the
+  // 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));
   volatile int* foo = reinterpret_cast<volatile int*>(7);
   *foo = 42;
 }

minidump/minidump_thread_writer.cc

@@ -150,6 +150,11 @@ void MinidumpThreadListWriter::InitializeFromSnapshot(
     thread->InitializeFromSnapshot(thread_snapshot, thread_id_map);
     AddThread(thread.Pass());
   }
+
+  // Do this in a separate loop to keep the thread stacks earlier in the dump,
+  // and together.
+  for (const ThreadSnapshot* thread_snapshot : thread_snapshots)
+    memory_list_writer_->AddFromSnapshot(thread_snapshot->ExtraMemory());
 }
 
 void MinidumpThreadListWriter::SetMemoryListWriter(

minidump/minidump_thread_writer_test.cc

@@ -18,6 +18,8 @@
 #include <dbghelp.h>
 #include <sys/types.h>
 
+#include <string>
+
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/strings/stringprintf.h"
@@ -527,6 +529,9 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
   uint64_t thread_ids[arraysize(expect_threads)] = {};
   uint8_t memory_values[arraysize(expect_threads)] = {};
   uint32_t context_seeds[arraysize(expect_threads)] = {};
+  MINIDUMP_MEMORY_DESCRIPTOR tebs[arraysize(expect_threads)] = {};
+
+  const size_t kTebSize = 1024;
 
   expect_threads[0].ThreadId = 1;
   expect_threads[0].SuspendCount = 2;
@@ -537,6 +542,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
   expect_threads[0].ThreadContext.DataSize = sizeof(MinidumpContextType);
   memory_values[0] = 'A';
   context_seeds[0] = 0x80000000;
+  tebs[0].StartOfMemoryRange = expect_threads[0].Teb;
+  tebs[0].Memory.DataSize = kTebSize;
 
   // The thread at index 1 has no stack.
   expect_threads[1].ThreadId = 11;
@@ -545,6 +552,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
   expect_threads[1].Teb = 0xfedcba9876543210;
   expect_threads[1].ThreadContext.DataSize = sizeof(MinidumpContextType);
   context_seeds[1] = 0x40000001;
+  tebs[1].StartOfMemoryRange = expect_threads[1].Teb;
+  tebs[1].Memory.DataSize = kTebSize;
 
   expect_threads[2].ThreadId = 21;
   expect_threads[2].SuspendCount = 22;
@@ -555,6 +564,8 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
   expect_threads[2].ThreadContext.DataSize = sizeof(MinidumpContextType);
   memory_values[2] = 'd';
   context_seeds[2] = 0x20000002;
+  tebs[2].StartOfMemoryRange = expect_threads[2].Teb;
+  tebs[2].Memory.DataSize = kTebSize;
 
   if (thread_id_collision) {
     thread_ids[0] = 0x0123456700000001;
@@ -595,6 +606,12 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
     Traits::InitializeCPUContext(thread_snapshot->MutableContext(),
                                  context_seeds[index]);
 
+    auto teb_snapshot = make_scoped_ptr(new TestMemorySnapshot());
+    teb_snapshot->SetAddress(expect_threads[index].Teb);
+    teb_snapshot->SetSize(kTebSize);
+    teb_snapshot->SetValue(static_cast<char>('t' + index));
+    thread_snapshot->AddExtraMemory(teb_snapshot.Pass());
+
     thread_snapshots.push_back(thread_snapshot);
   }
 
@@ -617,7 +634,7 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
       GetThreadListStream(string_file.string(), &thread_list, &memory_list));
 
   ASSERT_EQ(3u, thread_list->NumberOfThreads);
-  ASSERT_EQ(2u, memory_list->NumberOfMemoryRanges);
+  ASSERT_EQ(5u, memory_list->NumberOfMemoryRanges);
 
   size_t memory_index = 0;
   for (size_t index = 0; index < thread_list->NumberOfThreads; ++index) {
@@ -643,7 +660,7 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
           observed_stack,
           string_file.string(),
           memory_values[index],
-          index == thread_list->NumberOfThreads - 1));
+          false));
 
       ASSERT_NO_FATAL_FAILURE(ExpectMinidumpMemoryDescriptor(
           observed_stack, &memory_list->MemoryRanges[memory_index]));
@@ -651,6 +668,18 @@ void RunInitializeFromSnapshotTest(bool thread_id_collision) {
       ++memory_index;
     }
   }
+
+  for (size_t index = 0; index < thread_list->NumberOfThreads; ++index) {
+    const MINIDUMP_MEMORY_DESCRIPTOR* memory =
+        &memory_list->MemoryRanges[memory_index];
+    ASSERT_NO_FATAL_FAILURE(
+        ExpectMinidumpMemoryDescriptor(&tebs[index], memory));
+    std::string expected_data(kTebSize, static_cast<char>('t' + index));
+    std::string observed_data(&string_file.string()[memory->Memory.Rva],
+                              memory->Memory.DataSize);
+    EXPECT_EQ(expected_data, observed_data);
+    ++memory_index;
+  }
 }
 
 TEST(MinidumpThreadWriter, InitializeFromSnapshot_x86) {

snapshot/test/test_process_snapshot.h

@@ -98,8 +98,9 @@ class TestProcessSnapshot final : public ProcessSnapshot {
 
   //! \brief Add a memory snapshot to be returned by ExtraMemory().
   //!
-  //! \param[in] peb The memory snapshot that will be included in ExtraMemory().
+  //! \param[in] extra_memory The memory snapshot that will be included in
-  //!     The TestProcessSnapshot object takes ownership of \a extra_memory.
+  //!     ExtraMemory(). The TestProcessSnapshot object takes ownership of \a
+  //!     extra_memory.
   void AddExtraMemory(scoped_ptr<MemorySnapshot> extra_memory) {
     extra_memory_.push_back(extra_memory.release());
   }

snapshot/test/test_thread_snapshot.cc

@@ -55,5 +55,12 @@ uint64_t TestThreadSnapshot::ThreadSpecificDataAddress() const {
   return thread_specific_data_address_;
 }
 
+std::vector<const MemorySnapshot*> TestThreadSnapshot::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_thread_snapshot.h

@@ -17,11 +17,14 @@
 
 #include <stdint.h>
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "snapshot/cpu_context.h"
 #include "snapshot/memory_snapshot.h"
 #include "snapshot/thread_snapshot.h"
+#include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
 namespace test {
@@ -61,6 +64,15 @@ class TestThreadSnapshot final : public ThreadSnapshot {
     thread_specific_data_address_ = thread_specific_data_address;
   }
 
+  //! \brief Add a memory snapshot to be returned by ExtraMemory().
+  //!
+  //! \param[in] extra_memory The memory snapshot that will be included in
+  //!     ExtraMemory(). The TestThreadSnapshot object takes ownership of \a
+  //!     extra_memory.
+  void AddExtraMemory(scoped_ptr<MemorySnapshot> extra_memory) {
+    extra_memory_.push_back(extra_memory.release());
+  }
+
   // ThreadSnapshot:
 
   const CPUContext* Context() const override;
@@ -69,6 +81,7 @@ class TestThreadSnapshot final : public ThreadSnapshot {
   int SuspendCount() const override;
   int Priority() const override;
   uint64_t ThreadSpecificDataAddress() const override;
+  std::vector<const MemorySnapshot*> ExtraMemory() const override;
 
  private:
   union {
@@ -81,6 +94,7 @@ class TestThreadSnapshot final : public ThreadSnapshot {
   int suspend_count_;
   int priority_;
   uint64_t thread_specific_data_address_;
+  PointerVector<MemorySnapshot> extra_memory_;
 
   DISALLOW_COPY_AND_ASSIGN(TestThreadSnapshot);
 };

snapshot/thread_snapshot.h

@@ -17,6 +17,8 @@
 
 #include <stdint.h>
 
+#include <vector>
+
 namespace crashpad {
 
 struct CPUContext;
@@ -62,6 +64,15 @@ class ThreadSnapshot {
   //! \brief Returns the base address of a region used to store thread-specific
   //!     data.
   virtual uint64_t ThreadSpecificDataAddress() 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/win/process_reader_win.cc

@@ -177,7 +177,8 @@ bool FillThreadContextAndSuspendCount(HANDLE thread_handle,
 ProcessReaderWin::Thread::Thread()
     : context(),
       id(0),
-      teb(0),
+      teb_address(0),
+      teb_size(0),
       stack_region_address(0),
       stack_region_size(0),
       suspend_count(0),
@@ -332,8 +333,9 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) {
     // Read the TIB (Thread Information Block) which is the first element of the
     // TEB, for its stack fields.
     process_types::NT_TIB<Traits> tib;
-    thread.teb = thread_basic_info.TebBaseAddress;
+    thread.teb_address = thread_basic_info.TebBaseAddress;
-    if (ReadMemory(thread.teb, sizeof(tib), &tib)) {
+    thread.teb_size = sizeof(process_types::TEB<Traits>);
+    if (ReadMemory(thread.teb_address, sizeof(tib), &tib)) {
       WinVMAddress base = 0;
       WinVMAddress limit = 0;
       // If we're reading a WOW64 process, then the TIB we just retrieved is the
@@ -341,8 +343,10 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) {
       // https://msdn.microsoft.com/en-us/library/dn424783.aspx
       if (is_64_reading_32) {
         process_types::NT_TIB<process_types::internal::Traits32> tib32;
-        thread.teb = tib.Wow64Teb;
+        thread.teb_address = tib.Wow64Teb;
-        if (ReadMemory(thread.teb, sizeof(tib32), &tib32)) {
+        thread.teb_size =
+            sizeof(process_types::TEB<process_types::internal::Traits32>);
+        if (ReadMemory(thread.teb_address, sizeof(tib32), &tib32)) {
           base = tib32.StackBase;
           limit = tib32.StackLimit;
         }

snapshot/win/process_reader_win.h

@@ -48,10 +48,11 @@ class ProcessReaderWin {
       CONTEXT native;
 #if defined(ARCH_CPU_64_BITS)
       WOW64_CONTEXT wow64;
-#endif;
+#endif
     } context;
     uint64_t id;
-    WinVMAddress teb;
+    WinVMAddress teb_address;
+    WinVMSize teb_size;
     WinVMAddress stack_region_address;
     WinVMSize stack_region_size;
     uint32_t suspend_count;

snapshot/win/thread_snapshot_win.cc

@@ -14,6 +14,8 @@
 
 #include "snapshot/win/thread_snapshot_win.h"
 
+#include <vector>
+
 #include "base/logging.h"
 #include "snapshot/win/cpu_context_win.h"
 #include "snapshot/win/process_reader_win.h"
@@ -22,7 +24,12 @@ namespace crashpad {
 namespace internal {
 
 ThreadSnapshotWin::ThreadSnapshotWin()
-    : ThreadSnapshot(), context_(), stack_(), thread_(), initialized_() {
+    : ThreadSnapshot(),
+      context_(),
+      stack_(),
+      teb_(),
+      thread_(),
+      initialized_() {
 }
 
 ThreadSnapshotWin::~ThreadSnapshotWin() {
@@ -34,8 +41,11 @@ bool ThreadSnapshotWin::Initialize(
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   thread_ = process_reader_thread;
+  // TODO(scottmg): Ensure these regions are readable
+  // https://code.google.com/p/crashpad/issues/detail?id=59
   stack_.Initialize(
       process_reader, thread_.stack_region_address, thread_.stack_region_size);
+  teb_.Initialize(process_reader, thread_.teb_address, thread_.teb_size);
 
 #if defined(ARCH_CPU_X86_64)
   if (process_reader->Is64Bit()) {
@@ -84,7 +94,15 @@ int ThreadSnapshotWin::Priority() const {
 
 uint64_t ThreadSnapshotWin::ThreadSpecificDataAddress() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
-  return thread_.teb;
+  return thread_.teb_address;
+}
+
+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://code.google.com/p/crashpad/issues/detail?id=59
+  return std::vector<const MemorySnapshot*>(1, &teb_);
 }
 
 }  // namespace internal

snapshot/win/thread_snapshot_win.h

@@ -17,6 +17,8 @@
 
 #include <stdint.h>
 
+#include <vector>
+
 #include "base/basictypes.h"
 #include "snapshot/cpu_context.h"
 #include "snapshot/memory_snapshot.h"
@@ -24,6 +26,7 @@
 #include "snapshot/win/memory_snapshot_win.h"
 #include "snapshot/win/process_reader_win.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/stdlib/pointer_container.h"
 
 namespace crashpad {
 
@@ -58,6 +61,7 @@ class ThreadSnapshotWin final : public ThreadSnapshot {
   int SuspendCount() const override;
   int Priority() const override;
   uint64_t ThreadSpecificDataAddress() const override;
+  std::vector<const MemorySnapshot*> ExtraMemory() const override;
 
  private:
 #if defined(ARCH_CPU_X86_FAMILY)
@@ -68,6 +72,7 @@ class ThreadSnapshotWin final : public ThreadSnapshot {
 #endif
   CPUContext context_;
   MemorySnapshotWin stack_;
+  internal::MemorySnapshotWin teb_;
   ProcessReaderWin::Thread thread_;
   InitializationStateDcheck initialized_;
 

util/win/process_structs.h

@@ -309,12 +309,24 @@ struct CLIENT_ID {
 };
 
 // This is a partial definition of the TEB, as we do not currently use many
-// fields of it. See http://www.nirsoft.net/kernel_struct/vista/TEB.html.
+// fields of it. See http://www.nirsoft.net/kernel_struct/vista/TEB.html, and
+// the (arch-specific) definition of _TEB in winternl.h.
 template <class Traits>
 struct TEB {
   NT_TIB<Traits> NtTib;
-  typename Traits::Pointer EnvironmentPointer;
+  typename Traits::Pointer ProcessEnvironmentBlock;
   CLIENT_ID<Traits> ClientId;
+
+  // Not identical to Reserved2 in winternl's _TEB because we define ClientId.
+  typename Traits::Pointer RemainderOfReserved2[397];
+
+  BYTE Reserved3[1952];
+  typename Traits::Pointer TlsSlots[64];
+  BYTE Reserved4[8];
+  typename Traits::Pointer Reserved5[26];
+  typename Traits::Pointer ReservedForOle;
+  typename Traits::Pointer Reserved6[4];
+  typename Traits::Pointer TlsExpansionSlots;
 };
 
 // See https://msdn.microsoft.com/en-us/library/gg750724.aspx.