win: Add more memory regions to gathering of PEB

snapshot/win/process_snapshot_win.cc

Index: snapshot/win/process_snapshot_win.cc
diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc
index b9df62c0ab4c011109ab9054f5b556141958d658..e0df8e7c4cf7655542978062d7befaff2fe75818 100644
--- a/snapshot/win/process_snapshot_win.cc
+++ b/snapshot/win/process_snapshot_win.cc
@@ -14,6 +14,8 @@
 
 #include "snapshot/win/process_snapshot_win.h"
 
+#include <algorithm>
+
 #include "base/logging.h"
 #include "snapshot/win/module_snapshot_win.h"
 #include "util/win/registration_protocol_win.h"
@@ -48,10 +50,11 @@ bool ProcessSnapshotWin::Initialize(HANDLE process,
     return false;
 
   system_.Initialize(&process_reader_);
-  WinVMAddress peb_address;
-  WinVMSize peb_size;
-  process_reader_.GetProcessInfo().Peb(&peb_address, &peb_size);
-  peb_.Initialize(&process_reader_, peb_address, peb_size);
+
+  if (process_reader_.Is64Bit())
+    InitializePebData<process_types::internal::Traits64>();
+  else
+    InitializePebData<process_types::internal::Traits32>();
 
   InitializeThreads();
   InitializeModules();
@@ -186,7 +189,8 @@ const ExceptionSnapshot* ProcessSnapshotWin::Exception() const {
 std::vector<const MemorySnapshot*> ProcessSnapshotWin::ExtraMemory() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   std::vector<const MemorySnapshot*> extra_memory;
-  extra_memory.push_back(&peb_);
+  for (const auto& peb_memory : peb_memory_)
+    extra_memory.push_back(peb_memory);
   return extra_memory;
 }
 
@@ -214,4 +218,151 @@ void ProcessSnapshotWin::InitializeModules() {
   }
 }
 
+template <class Traits>
+void ProcessSnapshotWin::InitializePebData() {
+  WinVMAddress peb_address;
+  WinVMSize peb_size;
+  process_reader_.GetProcessInfo().Peb(&peb_address, &peb_size);
+  AddMemorySnapshot(peb_address, peb_size, &peb_memory_);
+
+  process_types::PEB<Traits> peb_data;
+  if (!process_reader_.ReadMemory(peb_address, peb_size, &peb_data)) {
+    LOG(ERROR) << "ReadMemory PEB";
+    return;
+  }
+
+  process_types::PEB_LDR_DATA<Traits> peb_ldr_data;
+  AddMemorySnapshot(peb_data.Ldr, sizeof(peb_ldr_data), &peb_memory_);
+  if (!process_reader_.ReadMemory(
+          peb_data.Ldr, sizeof(peb_ldr_data), &peb_ldr_data)) {
+    LOG(ERROR) << "ReadMemory PEB_LDR_DATA";
+  } else {
+    // Walk the LDR structure to retrieve its pointed-to data.
+    AddMemorySnapshotForLdrLIST_ENTRY(

[Mark Mentovai, 2015/10/01 19:05:47]

Do the elements in the three lists point to the same string data? If so, we’re
going to wind up adding duplicate regions for their UNICODE_STRINGs. As
discussed, I don’t think we should do that.

[scottmg, 2015/10/01 20:17:01]

It seems I'm writing more and more of the coalescing in here... but anyway, did
a simple search for identical pre-existing ranges in AddMemorySnapshot().

On the simple crashy_program, this reduced the minidump size from 28,848 bytes
to 27,056 bytes, which is a decent amount considering it only has 4 modules.

+        peb_ldr_data.InLoadOrderModuleList,
+        offsetof(process_types::LDR_DATA_TABLE_ENTRY<Traits>, InLoadOrderLinks),
+        &peb_memory_);
+    AddMemorySnapshotForLdrLIST_ENTRY(
+        peb_ldr_data.InMemoryOrderModuleList,
+        offsetof(process_types::LDR_DATA_TABLE_ENTRY<Traits>,
+                 InMemoryOrderLinks),
+        &peb_memory_);
+    AddMemorySnapshotForLdrLIST_ENTRY(
+        peb_ldr_data.InInitializationOrderModuleList,
+        offsetof(process_types::LDR_DATA_TABLE_ENTRY<Traits>,
+                 InInitializationOrderLinks),
+        &peb_memory_);
+  }
+
+  process_types::RTL_USER_PROCESS_PARAMETERS<Traits> process_parameters;
+  if (!process_reader_.ReadMemory(peb_data.ProcessParameters,
+                                  sizeof(process_parameters),
+                                  &process_parameters)) {
+    LOG(ERROR) << "ReadMemory RTL_USER_PROCESS_PARAMETERS";
+    return;
+  }
+  AddMemorySnapshot(
+      peb_data.ProcessParameters, sizeof(process_parameters), &peb_memory_);
+
+  AddMemorySnapshotForUNICODE_STRING(
+      process_parameters.CurrentDirectory.DosPath, &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.DllPath, &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.ImagePathName,
+                                     &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.CommandLine,
+                                     &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.WindowTitle,
+                                     &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.DesktopInfo,
+                                     &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.ShellInfo,
+                                     &peb_memory_);
+  AddMemorySnapshotForUNICODE_STRING(process_parameters.RuntimeData,
+                                     &peb_memory_);
+  AddMemorySnapshot(
+      process_parameters.Environment,
+      DetermineSizeOfEnvironmentBlock(process_parameters.Environment),
+      &peb_memory_);
+}
+
+void ProcessSnapshotWin::AddMemorySnapshot(
+    WinVMAddress address,
+    WinVMSize size,
+    PointerVector<internal::MemorySnapshotWin>* into) {
+  if (size == 0)
+    return;
+
+  // Ensure that the entire range is readable. TODO(scottmg): Consider
+  // generalizing this as part of
+  // https://code.google.com/p/crashpad/issues/detail?id=59.

[Mark Mentovai, 2015/10/01 19:05:47]

I read this bug and liked the terminology you used, “auxiliary memory”. I’m
kinda keen on renaming ExtraMemory() in the Snapshot family to that. There’s
some naming confusion in the minidump family, where MinidumpMemoryListWriter has
its own thing called “extra memory”. (For a follow-up, if at all.)

[scottmg, 2015/10/01 20:17:01]

Yeah, I confused myself for a bit with ExtraMemory(). I'll rename these to
AuxiliaryMemory() in a follow-up then.

+  auto ranges = process_reader_.GetProcessInfo().GetReadableRanges(
+      CheckedRange<WinVMAddress, WinVMSize>(address, size));
+  if (ranges.size() != 1)
+    return;

[Mark Mentovai, 2015/10/01 19:05:47]

Maybe these early returns should log something. When we get the log messages
encountered during snapshotting hooked up to be carried in the minidump, it
might be nice to see “oh, we’re missing something we tried to collect.”

[scottmg, 2015/10/01 20:17:01]

Done.

+  if (ranges[0].base() != address || ranges[0].size() != size)
+    return;
+
+  internal::MemorySnapshotWin* memory_snapshot =
+      new internal::MemorySnapshotWin();
+  memory_snapshot->Initialize(&process_reader_, address, size);
+  into->push_back(memory_snapshot);
+}
+
+template <class Traits>
+void ProcessSnapshotWin::AddMemorySnapshotForUNICODE_STRING(
+    const process_types::UNICODE_STRING<Traits>& us,
+    PointerVector<internal::MemorySnapshotWin>* into) {
+  AddMemorySnapshot(us.Buffer, us.Length, into);
+}
+
+template <class Traits>
+void ProcessSnapshotWin::AddMemorySnapshotForLdrLIST_ENTRY(
+      const process_types::LIST_ENTRY<Traits>& le, size_t offset_of_member,
+      PointerVector<internal::MemorySnapshotWin>* into) {
+  // Walk the doubly-linked list of entries, adding the list memory itself, as
+  // well as pointed-to strings.
+  Traits::Pointer last = le.Blink;
+  process_types::LDR_DATA_TABLE_ENTRY<Traits> entry;
+  Traits::Pointer cur = le.Flink;
+  for (;;) {
+    // |cur| is the pointer to LIST_ENTRY embedded in the LDR_DATA_TABLE_ENTRY.
+    // So we need to offset back to the beginning of the structure.
+    if (!process_reader_.ReadMemory(
+            cur - offset_of_member, sizeof(entry), &entry)) {
+      return;
+    }
+    AddMemorySnapshot(cur - offset_of_member, sizeof(entry), into);
+    AddMemorySnapshotForUNICODE_STRING(entry.FullDllName, into);
+    AddMemorySnapshotForUNICODE_STRING(entry.BaseDllName, into);
+
+    process_types::LIST_ENTRY<Traits>* links =
+        reinterpret_cast<process_types::LIST_ENTRY<Traits>*>(
+            reinterpret_cast<unsigned char*>(&entry) + offset_of_member);
+    cur = links->Flink;
+    if (cur == last)
+      break;
+  }
+}
+
+WinVMSize ProcessSnapshotWin::DetermineSizeOfEnvironmentBlock(
+    WinVMAddress start_of_environment_block) {
+  // http://blogs.msdn.com/b/oldnewthing/archive/2010/02/03/9957320.aspx On
+  // newer OSs there's no stated limit, but in practice grabbing 32k characters
+  // should be more than enough.
+  std::wstring env_block;
+  env_block.resize(32768);
+  WinVMSize bytes_read = process_reader_.ReadAvailableMemory(
+      start_of_environment_block,
+      env_block.size() * sizeof(env_block[0]),
+      &env_block[0]);
+  env_block.resize(
+      static_cast<unsigned int>(bytes_read / sizeof(env_block[0])));
+  const wchar_t terminator[] = { 0, 0 };
+  size_t at = env_block.find(std::wstring(terminator, arraysize(terminator)));
+  if (at != std::wstring::npos)
+    env_block.resize(at + arraysize(terminator));
+
+  return env_block.size() * sizeof(env_block[0]);
+}
+
 }  // namespace crashpad