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..29c9db58ffe44d19fa8475f12ed2210b7ac6f793 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,144 @@ 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";
+    return;

[Mark Mentovai, 2015/09/29 21:57:11]

Don’t bail out here. You still might be able to save the
RTL_USER_PROCESS_PARAMETERS stuff.

[scottmg, 2015/09/30 18:19:40]

Done.

+  }
+
+  // Walk the LDR structure to retrieve its pointed-to data. We don't care too

[Mark Mentovai, 2015/09/29 21:57:10]

Question: does MiniDumpWriteDump() collect these regions too, or are we reaching
the land of beating MiniDumpWriteDump() at its own game? I don’t recall seeing
this many more regions than just stacks in recent-ish minidumps that it
produced.

[scottmg, 2015/09/30 18:19:40]

Doesn't look like MiniDumpWriteDump() grabs the LDR data. I think it could
sometimes be nice to have the module load list here so that it's easily
accessible in windbg as part of !peb.

It shouldn't be a lot of extra memory (< ~100 path strings plus the linked list
structure) but it's not strictly necessary it seems. So I could go either way on
including them.

I thought it was necessary to read these for windbg, but it appears it's not
required, as current crashes don't have the LDR and the rest of the PEB dumps
fine. I'm not sure what was going wrong in the dumps before when it appeared to
be aborting, but I guess I was mistaken. (Updated the comment here.)

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

No real objection to including this, I’m just trying to maintain a mental list
of things we’re doing better than MiniDumpWriteDump(). In the future, if we find
we’re generating bigger minidumps than we want, we might want to make some of
these things optional. Maybe the list should be written down instead of in our
heads.

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

I'll include it for now then as it's nice to have in windbg. Filed
https://code.google.com/p/crashpad/issues/detail?id=60 for the general case.
Might make sense to make some of this configurable.

+  // much about this here as we have the data in other places already, but
+  // without it Windbg's `!peb` gets confused and won't dump other useful data
+  // in the PEB.
+  AddMemorySnapshotForLdrLIST_ENTRY(

[Mark Mentovai, 2015/09/29 21:57:11]

I’ve got a new concern. A lot of the extra memory that we’re adding now isn’t
critical to the minidump, and a lot of these regions are unchecked in the sense
that we don’t really know that we’ll be able to read them when it comes time to
write the minidump. And I think that the behavior of a failed memory read when
the dump is being written will result in the whole dump being thrown out. It
would be a shame for a corrupt pointer in one of these structures to mess
everything up.

Can we make AddMemorySnapshot() ensure that it’s adding a mapped and readable
range? We can have it reject adds of totally unmapped memory. We can have it add
the mapped subset of a requested range, or we can also just have it reject
attempts to add memory when any portion of it is unmapped.

I think it’s enough to just make sure that things look good in the map, since
the process will be suspended when we do this stuff for real. I don’t think that
we need to worry about someone else calling VirtualFreeEx() while we’re chewing
on a suspended process.

By the way, this problem doesn’t just apply to the new ExtraMemory() support, we
might run into similar trouble with stack memory too. The Mac version is immune
to this problem because of how CalculateStackRegion() works, but we’d need to be
aware of it when we start leveraging ExtraMemory() over there too.

Fixing this may be out of scope for this change, and I think that’s fine. But we
will need to fix this.

[scottmg, 2015/09/30 18:19:40]

Yeah, all good points.

In general, I don't like how I need to do some things twice here. That is, a
read here to walk pointers and then throw that away, only to have the minidump
code read it again. It's not likely to be inconsistent because we're suspended,
but still seems like it could be better.

For now, I made AddMemorySnapshot() verify that the requested range is fully
readable, and filed https://code.google.com/p/crashpad/issues/detail?id=59 to
address it more generally.

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

scottmg wrote:
Nobody says that every memory snapshot you hand out has to be the generic
MemorySnapshotWin. You can read the environment block once into an object that
implements MemorySnapshot, and have its Read() just pass the saved data straight
through to the delegate without ever touching the process.

(If you like this, let’s save it for a follow-up.)

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

Good point. It's pretty localized at the moment, so I'll just keep it in mind
for future similar cases to see if it comes up again. For "one deep" structures,
the current setup works well.

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