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..e64d783d5b1ab4ef2b78d7894448b376b1c52b78 100644
--- a/snapshot/win/process_snapshot_win.cc
+++ b/snapshot/win/process_snapshot_win.cc
@@ -48,10 +48,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 +187,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 +216,51 @@ void ProcessSnapshotWin::InitializeModules() {
   }
 }
 
+template <class Traits>
+void ProcessSnapshotWin::InitializePebData() {
+  WinVMAddress peb_address;
+  WinVMSize peb_size;
+  process_reader_.GetProcessInfo().Peb(&peb_address, &peb_size);
+  peb_memory_.push_back(CreateMemorySnapshot(peb_address, peb_size));
+
+  process_types::PEB<Traits> peb_data;
+  if (!process_reader_.ReadMemory(peb_address, peb_size, &peb_data)) {
+    LOG(ERROR) << "ReadMemory PEB";
+    return;
+  }
+  peb_memory_.push_back(CreateMemorySnapshot(
+      peb_data.Ldr, sizeof(process_types::PEB_LDR_DATA<Traits>)));
+
+  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;
+  }
+
+  peb_memory_.push_back(CreateMemorySnapshotForUNICODE_STRING(
+      process_parameters.CurrentDirectory.DosPath));
+  peb_memory_.push_back(
+      CreateMemorySnapshotForUNICODE_STRING(process_parameters.DllPath));

[Mark Mentovai, 2015/09/25 21:10:16]

The “now” section of the CL description seems to indicate that this isn’t
working. Or was this field just empty in your test?

[scottmg, 2015/09/25 22:52:25]

Yes, it's often null in a live process too. It's the search path that the loader
uses to locate dlls, but if it's not overridden then (I guess) it doesn't get
filled in here.

+  peb_memory_.push_back(
+      CreateMemorySnapshotForUNICODE_STRING(process_parameters.ImagePathName));
+  peb_memory_.push_back(
+      CreateMemorySnapshotForUNICODE_STRING(process_parameters.CommandLine));
+  // 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 should be
+  // more than enough.
+  const int kMaxEnvironmentBlockSize = 32768;
+  peb_memory_.push_back(CreateMemorySnapshot(process_parameters.Environment,
+                                             kMaxEnvironmentBlockSize));

[Mark Mentovai, 2015/09/25 21:10:16]

It might be worth trying to work out the actual size of the environment block so
that we don’t over-report, but you can save that for a TODO if it’s rough to
work out.

[scottmg, 2015/09/25 22:52:25]

I don't know of any way other than probing the target process for \0, which is a
bit tedious (and I'm not sure if it's 100% correct either). Added a TODO as
*something* must know how big that block is.

[Mark Mentovai, 2015/09/25 23:11:59]

scottmg wrote:
Well, if windbg’s able to work out where to stop printing environment variables,
there’s got to be some way to do it. I think you just look for a double-NUL.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms682653(v=vs.85).aspx

[scottmg, 2015/09/26 00:07:38]

Yeah, that was my thinking too. But then I was worried that we might lose some
interesting information if the environment block was the thing that had been
futzed with. But maybe that's useful too, because you'd see what the process
would probably be able to retrieve.

... ok, switched to grabbing 32k here to find the \0\0 and then only storing up
to that point.

+}
+
+internal::MemorySnapshotWin* ProcessSnapshotWin::CreateMemorySnapshot(
+    WinVMAddress address,
+    WinVMAddress size) {

[Mark Mentovai, 2015/09/25 21:10:16]

If size is 0, it’s probably worth not creating anything. Instead, return
nullptr, and have the caller not add anything to the peb_memory_ vector.

That may mean that it’s easier for this return void and take the vector as an
argument, appending to it if size is nonzero, instead of returning something for
the caller to append (or not).

[scottmg, 2015/09/25 22:52:25]

Done.

+  internal::MemorySnapshotWin* ret = new internal::MemorySnapshotWin();
+  ret->Initialize(&process_reader_, address, size);
+  return ret;
+}
+
 }  // namespace crashpad