win: Add more memory regions to gathering of PEB

snapshot/win/process_reader_win.cc

 // Copyright 2015 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,
@@ skipping 195 matching lines @@
   process_ = process;
   suspension_state_ = suspension_state;
   process_info_.Initialize(process);
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
 
 bool ProcessReaderWin::ReadMemory(WinVMAddress at,
                                   WinVMSize num_bytes,
                                   void* into) const {

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

Possibly this and ReadAvailableMemory() should both return early without failing
if num_bytes is 0. Mac’s TaskMemory::ReadMapped() is precedent for this.

ReadProcessMemory() might do the early return thing on its own, so it might be a
no-op and thus unnecessary for ReadMemory(). But for ReadAvailableMemory(), if
you add the logging for the “it wasn’t mapped” cases, that would also log for
the num_bytes = 0 case, since the new logic we agreed on is for a 0-sized range
to not overlap any other, and we’d hit ranges.empty().

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

Done.

   SIZE_T bytes_read;
   if (!ReadProcessMemory(process_,
                          reinterpret_cast<void*>(at),
                          into,
                          base::checked_cast<SIZE_T>(num_bytes),
                          &bytes_read) ||
       num_bytes != bytes_read) {
     PLOG(ERROR) << "ReadMemory at 0x" << std::hex << at << std::dec << " of "
                 << num_bytes << " bytes failed";
     return false;
   }
   return true;
 }
 
+WinVMSize ProcessReaderWin::ReadAvailableMemory(WinVMAddress at,
+                                                WinVMSize num_bytes,
+                                                void* into) const {
+  auto ranges = process_info_.GetReadableRanges(
+      CheckedRange<WinVMAddress, WinVMSize>(at, num_bytes));
+  // We only read up until the first unavailable block, so we only read from the

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

Put some blank lines before these comments that apply to the things that follow
them but not the things that precede them.

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

Done.

+  // first range.
+  if (ranges.empty())
+    return 0;

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

LOG something on the return-0s that don’t pass through ReadMemory()?

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

I was thinking since we're using this in a trial-and-error way for probing the
environment block, it would be better if it weren't noisy. But I guess it is
useful to know that not even the first byte was accessible because that probably
means the parent structure pointing here is broken.

[Mark Mentovai, 2015/10/01 22:00:52]

scottmg wrote:
It’s probably reasonable to expect there to be something present where we’re
told the environment block is, and to flag it as an oddity if there isn’t
anything there. It’s good now in the latest patch set.

+  // If the start address was adjusted, we couldn't read even the first
+  // requested byte.
+  if (ranges.front().base() != at)
+    return 0;
+  // If we fail on a normal read, then something went very wrong.
+  if (!ReadMemory(ranges.front().base(), ranges.front().size(), into))

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

It’s not strictly necessary because the tests should cover GetReadableRanges’
behavior well enough that this shouldn’t ever happen, but consider:

  DCHECK_LE(ranges.front.size(), num_bytes);

before calling ReadMemory().

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

Done.

+    return 0;
+  return ranges.front().size();
+}
+
 bool ProcessReaderWin::StartTime(timeval* start_time) const {
   FILETIME creation, exit, kernel, user;
   if (!GetProcessTimes(process_, &creation, &exit, &kernel, &user)) {
     PLOG(ERROR) << "GetProcessTimes";
     return false;
   }
   *start_time = FiletimeToTimevalEpoch(creation);
   return true;
 }
 
@@ skipping 121 matching lines @@
         thread.stack_region_size = 0;
       } else {
         thread.stack_region_size = base - limit;
       }
     }
     threads_.push_back(thread);
   }
 }
 
 }  // namespace crashpad