Refactor ProcessReaderWin to use ProcessMemoryWin

Remove ProcessReaderWin's ReadMemory() and ReadAvailableMemory() methods
and replace their uses with a new method that exposes an instance of
ProcessMemoryWin instead.

BUG=crashpad:262

Change-Id: Ief5b660b0504d7a740ee53c7cd2fa7672ae56249
Reviewed-on: https://chromium-review.googlesource.com/c/1278830
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Vlad Tsyrklevich <vtsyrklevich@chromium.org>

snapshot/win/capture_memory_delegate_win.cc

@@ -39,7 +39,8 @@ bool CaptureMemoryDelegateWin::Is64Bit() const {
 bool CaptureMemoryDelegateWin::ReadMemory(uint64_t at,
                                           uint64_t num_bytes,
                                           void* into) const {
-  return process_reader_->ReadMemory(at, num_bytes, into);
+  return process_reader_->Memory()->Read(
+      at, base::checked_cast<size_t>(num_bytes), into);
 }
 
 std::vector<CheckedRange<uint64_t>> CaptureMemoryDelegateWin::GetReadableRanges(

snapshot/win/exception_snapshot_win.cc

@@ -176,9 +176,9 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
                                   CPUContext* context,
                                   CPUContextUnion* context_union)) {
   ExceptionPointersType exception_pointers;
-  if (!process_reader->ReadMemory(exception_pointers_address,
+  if (!process_reader->Memory()->Read(exception_pointers_address,
-                                  sizeof(exception_pointers),
+                                      sizeof(exception_pointers),
-                                  &exception_pointers)) {
+                                      &exception_pointers)) {
     LOG(ERROR) << "EXCEPTION_POINTERS read failed";
     return false;
   }
@@ -188,7 +188,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
   }
 
   ExceptionRecordType first_record;
-  if (!process_reader->ReadMemory(
+  if (!process_reader->Memory()->Read(
           static_cast<WinVMAddress>(exception_pointers.ExceptionRecord),
           sizeof(first_record),
           &first_record)) {
@@ -240,7 +240,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers(
     }
 
     ContextType context_record;
-    if (!process_reader->ReadMemory(
+    if (!process_reader->Memory()->Read(
             static_cast<WinVMAddress>(exception_pointers.ContextRecord),
             sizeof(context_record),
             &context_record)) {

snapshot/win/memory_snapshot_win.cc

@@ -60,7 +60,7 @@ bool MemorySnapshotWin::Read(Delegate* delegate) const {
   }
 
   std::unique_ptr<uint8_t[]> buffer(new uint8_t[size_]);
-  if (!process_reader_->ReadMemory(address_, size_, buffer.get())) {
+  if (!process_reader_->Memory()->Read(address_, size_, buffer.get())) {
     return false;
   }
   return delegate->MemorySnapshotDelegateRead(buffer.get(), size_);

snapshot/win/module_snapshot_win.cc

@@ -278,7 +278,7 @@ void ModuleSnapshotWin::GetCrashpadExtraMemoryRanges(
 
   std::vector<SimpleAddressRangeBag::Entry> simple_ranges(
       SimpleAddressRangeBag::num_entries);
-  if (!process_reader_->ReadMemory(
+  if (!process_reader_->Memory()->Read(
           crashpad_info.extra_address_ranges,
           simple_ranges.size() * sizeof(simple_ranges[0]),
           &simple_ranges[0])) {
@@ -304,8 +304,8 @@ void ModuleSnapshotWin::GetCrashpadUserMinidumpStreams(
 
   for (uint64_t cur = crashpad_info.user_data_minidump_stream_head; cur;) {
     internal::UserDataMinidumpStreamListEntry list_entry;
-    if (!process_reader_->ReadMemory(
+    if (!process_reader_->Memory()->Read(
-          cur, sizeof(list_entry), &list_entry)) {
+            cur, sizeof(list_entry), &list_entry)) {
       LOG(WARNING) << "could not read user data stream entry from "
                    << base::UTF16ToUTF8(name_);
       return;

snapshot/win/pe_image_annotations_reader.cc

@@ -92,7 +92,7 @@ void PEImageAnnotationsReader::ReadCrashpadSimpleAnnotations(
 
   std::vector<SimpleStringDictionary::Entry>
       simple_annotations(SimpleStringDictionary::num_entries);
-  if (!process_reader_->ReadMemory(
+  if (!process_reader_->Memory()->Read(
           crashpad_info.simple_annotations,
           simple_annotations.size() * sizeof(simple_annotations[0]),
           &simple_annotations[0])) {
@@ -127,9 +127,9 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList(
   }
 
   process_types::AnnotationList<Traits> annotation_list_object;
-  if (!process_reader_->ReadMemory(crashpad_info.annotations_list,
+  if (!process_reader_->Memory()->Read(crashpad_info.annotations_list,
-                                   sizeof(annotation_list_object),
+                                       sizeof(annotation_list_object),
-                                   &annotation_list_object)) {
+                                       &annotation_list_object)) {
     LOG(WARNING) << "could not read annotations list object in "
                  << base::UTF16ToUTF8(name_);
     return;
@@ -140,7 +140,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList(
        current.link_node != annotation_list_object.tail_pointer &&
        index < kMaxNumberOfAnnotations;
        ++index) {
-    if (!process_reader_->ReadMemory(
+    if (!process_reader_->Memory()->Read(
             current.link_node, sizeof(current), &current)) {
       LOG(WARNING) << "could not read annotation at index " << index << " in "
                    << base::UTF16ToUTF8(name_);
@@ -155,7 +155,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList(
     snapshot.type = current.type;
 
     char name[Annotation::kNameMaxLength];
-    if (!process_reader_->ReadMemory(current.name, arraysize(name), name)) {
+    if (!process_reader_->Memory()->Read(current.name, arraysize(name), name)) {
       LOG(WARNING) << "could not read annotation name at index " << index
                    << " in " << base::UTF16ToUTF8(name_);
       continue;
@@ -167,7 +167,7 @@ void PEImageAnnotationsReader::ReadCrashpadAnnotationsList(
     size_t value_length =
         std::min(static_cast<size_t>(current.size), Annotation::kValueMaxSize);
     snapshot.value.resize(value_length);
-    if (!process_reader_->ReadMemory(
+    if (!process_reader_->Memory()->Read(
             current.value, value_length, snapshot.value.data())) {
       LOG(WARNING) << "could not read annotation value at index " << index
                    << " in " << base::UTF16ToUTF8(name_);

snapshot/win/process_reader_win.cc

@@ -204,6 +204,7 @@ ProcessReaderWin::Thread::Thread()
 ProcessReaderWin::ProcessReaderWin()
     : process_(INVALID_HANDLE_VALUE),
       process_info_(),
+      process_memory_(),
       threads_(),
       modules_(),
       suspension_state_(),
@@ -220,67 +221,15 @@ bool ProcessReaderWin::Initialize(HANDLE process,
 
   process_ = process;
   suspension_state_ = suspension_state;
-  process_info_.Initialize(process);
+  if (!process_info_.Initialize(process))
+    return false;
+  if (!process_memory_.Initialize(process))
+    return false;
 
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
 
-bool ProcessReaderWin::ReadMemory(WinVMAddress at,
-                                  WinVMSize num_bytes,
-                                  void* into) const {
-  if (num_bytes == 0)
-    return 0;
-
-  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 {
-  if (num_bytes == 0)
-    return 0;
-
-  auto ranges = process_info_.GetReadableRanges(
-      CheckedRange<WinVMAddress, WinVMSize>(at, num_bytes));
-
-  // We only read up until the first unavailable byte, so we only read from the
-  // first range. If we have no ranges, then no bytes were accessible anywhere
-  // in the range.
-  if (ranges.empty()) {
-    LOG(ERROR) << base::StringPrintf(
-        "range at 0x%llx, size 0x%llx completely inaccessible", at, num_bytes);
-    return 0;
-  }
-
-  // If the start address was adjusted, we couldn't read even the first
-  // requested byte.
-  if (ranges.front().base() != at) {
-    LOG(ERROR) << base::StringPrintf(
-        "start of range at 0x%llx, size 0x%llx inaccessible", at, num_bytes);
-    return 0;
-  }
-
-  DCHECK_LE(ranges.front().size(), num_bytes);
-
-  // If we fail on a normal read, then something went very wrong.
-  if (!ReadMemory(ranges.front().base(), ranges.front().size(), into))
-    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)) {
@@ -398,7 +347,7 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) {
     process_types::NT_TIB<Traits> tib;
     thread.teb_address = thread_basic_info.TebBaseAddress;
     thread.teb_size = sizeof(process_types::TEB<Traits>);
-    if (ReadMemory(thread.teb_address, sizeof(tib), &tib)) {
+    if (process_memory_.Read(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
@@ -409,7 +358,7 @@ void ProcessReaderWin::ReadThreadData(bool is_64_reading_32) {
         thread.teb_address = tib.Wow64Teb;
         thread.teb_size =
             sizeof(process_types::TEB<process_types::internal::Traits32>);
-        if (ReadMemory(thread.teb_address, sizeof(tib32), &tib32)) {
+        if (process_memory_.Read(thread.teb_address, sizeof(tib32), &tib32)) {
           base = tib32.StackBase;
           limit = tib32.StackLimit;
         }

snapshot/win/process_reader_win.h

@@ -23,6 +23,7 @@
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/process/process_memory_win.h"
 #include "util/win/address_types.h"
 #include "util/win/process_info.h"
 
@@ -84,25 +85,8 @@ class ProcessReaderWin {
   //! \return `true` if the target task is a 64-bit process.
   bool Is64Bit() const { return process_info_.Is64Bit(); }
 
-  //! \brief Attempts to read \a num_bytes bytes from the target process
+  //! \brief Return a memory reader for the target process.
-  //!     starting at address \a at into \a into.
+  const ProcessMemoryWin* Memory() const { return &process_memory_; }
-  //!
-  //! \return `true` if the entire region could be read, or `false` with an
-  //!     error logged.
-  //!
-  //! \sa ReadAvailableMemory
-  bool ReadMemory(WinVMAddress at, WinVMSize num_bytes, void* into) const;
-
-  //! \brief Attempts to read \a num_bytes bytes from the target process
-  //!     starting at address \a at into \a into. If some of the specified range
-  //!     is not accessible, reads up to the first inaccessible byte.
-  //!
-  //! \return The actual number of bytes read.
-  //!
-  //! \sa ReadMemory
-  WinVMSize ReadAvailableMemory(WinVMAddress at,
-                                WinVMSize num_bytes,
-                                void* into) const;
 
   //! \brief Determines the target process' start time.
   //!
@@ -145,6 +129,7 @@ class ProcessReaderWin {
 
   HANDLE process_;
   ProcessInfo process_info_;
+  ProcessMemoryWin process_memory_;
   std::vector<Thread> threads_;
   std::vector<ProcessInfo::Module> modules_;
   ProcessSuspensionState suspension_state_;

snapshot/win/process_reader_win_test.cc

@@ -43,7 +43,7 @@ TEST(ProcessReaderWin, SelfBasic) {
 
   static constexpr char kTestMemory[] = "Some test memory";
   char buffer[arraysize(kTestMemory)];
-  ASSERT_TRUE(process_reader.ReadMemory(
+  ASSERT_TRUE(process_reader.Memory()->Read(
       reinterpret_cast<uintptr_t>(kTestMemory), sizeof(kTestMemory), &buffer));
   EXPECT_STREQ(kTestMemory, buffer);
 }
@@ -72,7 +72,7 @@ class ProcessReaderChild final : public WinMultiprocess {
 
     char buffer[sizeof(kTestMemory)];
     ASSERT_TRUE(
-        process_reader.ReadMemory(address, sizeof(kTestMemory), &buffer));
+        process_reader.Memory()->Read(address, sizeof(kTestMemory), &buffer));
     EXPECT_EQ(strcmp(kTestMemory, buffer), 0);
   }
 

snapshot/win/process_snapshot_win.cc

@@ -21,7 +21,7 @@
 #include <utility>
 
 #include "base/logging.h"
-#include "base/strings/stringprintf.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "util/misc/from_pointer_cast.h"
 #include "util/misc/time.h"
@@ -63,9 +63,9 @@ bool ProcessSnapshotWin::Initialize(
 
   if (exception_information_address != 0) {
     ExceptionInformation exception_information = {};
-    if (!process_reader_.ReadMemory(exception_information_address,
+    if (!process_reader_.Memory()->Read(exception_information_address,
-                                    sizeof(exception_information),
+                                        sizeof(exception_information),
-                                    &exception_information)) {
+                                        &exception_information)) {
       LOG(WARNING) << "ReadMemory ExceptionInformation failed";
       return false;
     }
@@ -298,9 +298,9 @@ void ProcessSnapshotWin::InitializeUnloadedModules() {
       FromPointerCast<WinVMAddress>(event_trace_address);
 
   Traits::Pointer pointer_to_array;
-  if (!process_reader_.ReadMemory(address_in_target_process,
+  if (!process_reader_.Memory()->Read(address_in_target_process,
-                                  sizeof(pointer_to_array),
+                                      sizeof(pointer_to_array),
-                                  &pointer_to_array)) {
+                                      &pointer_to_array)) {
     LOG(ERROR) << "failed to read target address";
     return;
   }
@@ -311,7 +311,7 @@ void ProcessSnapshotWin::InitializeUnloadedModules() {
 
   const size_t data_size = *element_size * *element_count;
   std::vector<uint8_t> data(data_size);
-  if (!process_reader_.ReadMemory(pointer_to_array, data_size, &data[0])) {
+  if (!process_reader_.Memory()->Read(pointer_to_array, data_size, &data[0])) {
     LOG(ERROR) << "failed to read unloaded module data";
     return;
   }
@@ -377,14 +377,15 @@ void ProcessSnapshotWin::InitializePebData(
   AddMemorySnapshot(peb_address, peb_size, &extra_memory_);
 
   process_types::PEB<Traits> peb_data;
-  if (!process_reader_.ReadMemory(peb_address, peb_size, &peb_data)) {
+  if (!process_reader_.Memory()->Read(
+          peb_address, base::checked_cast<size_t>(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), &extra_memory_);
-  if (!process_reader_.ReadMemory(
+  if (!process_reader_.Memory()->Read(
           peb_data.Ldr, sizeof(peb_ldr_data), &peb_ldr_data)) {
     LOG(ERROR) << "ReadMemory PEB_LDR_DATA";
   } else {
@@ -406,9 +407,9 @@ void ProcessSnapshotWin::InitializePebData(
   }
 
   process_types::RTL_USER_PROCESS_PARAMETERS<Traits> process_parameters;
-  if (!process_reader_.ReadMemory(peb_data.ProcessParameters,
+  if (!process_reader_.Memory()->Read(peb_data.ProcessParameters,
-                                  sizeof(process_parameters),
+                                      sizeof(process_parameters),
-                                  &process_parameters)) {
+                                      &process_parameters)) {
     LOG(ERROR) << "ReadMemory RTL_USER_PROCESS_PARAMETERS";
     return;
   }
@@ -496,7 +497,7 @@ void ProcessSnapshotWin::AddMemorySnapshotForLdrLIST_ENTRY(
   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(
+    if (!process_reader_.Memory()->Read(
             cur - offset_of_member, sizeof(entry), &entry)) {
       return;
     }
@@ -520,7 +521,7 @@ WinVMSize ProcessSnapshotWin::DetermineSizeOfEnvironmentBlock(
   // be more than enough.
   std::wstring env_block;
   env_block.resize(32768);
-  WinVMSize bytes_read = process_reader_.ReadAvailableMemory(
+  size_t bytes_read = process_reader_.Memory()->ReadAvailableMemory(
       start_of_environment_block,
       env_block.size() * sizeof(env_block[0]),
       &env_block[0]);
@@ -543,7 +544,7 @@ void ProcessSnapshotWin::ReadLock(
   // RTL_CRITICAL_SECTION_DEBUG.
 
   process_types::RTL_CRITICAL_SECTION<Traits> critical_section;
-  if (!process_reader_.ReadMemory(
+  if (!process_reader_.Memory()->Read(
           start, sizeof(critical_section), &critical_section)) {
     LOG(ERROR) << "failed to read RTL_CRITICAL_SECTION";
     return;

snapshot/win/process_subrange_reader.cc

@@ -15,6 +15,7 @@
 #include "snapshot/win/process_subrange_reader.h"
 
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "snapshot/win/process_reader_win.h"
 
 namespace crashpad {
@@ -82,7 +83,8 @@ bool ProcessSubrangeReader::ReadMemory(WinVMAddress address,
     return false;
   }
 
-  return process_reader_->ReadMemory(address, size, into);
+  return process_reader_->Memory()->Read(
+      address, base::checked_cast<size_t>(size), into);
 }
 
 bool ProcessSubrangeReader::InitializeInternal(ProcessReaderWin* process_reader,