Add a MappedMemory interface to TaskMemory and use it in MachOImageSymbolTableReader

util/mach/task_memory.cc

Index: util/mach/task_memory.cc
diff --git a/util/mach/task_memory.cc b/util/mach/task_memory.cc
index 9597165f99cac323f3461d7bf0cd4ea0ca74a5df..718f61bbad1bfddf719a2dfc9405db6b1e0d1c6b 100644
--- a/util/mach/task_memory.cc
+++ b/util/mach/task_memory.cc
@@ -21,17 +21,66 @@
 
 #include "base/logging.h"
 #include "base/mac/mach_logging.h"
-#include "base/mac/scoped_mach_vm.h"
 #include "base/strings/stringprintf.h"
+#include "util/stdlib/strnlen.h"
 
 namespace crashpad {
 
+TaskMemory::MappedMemory::MappedMemory(vm_address_t vm_address,
+                                       size_t vm_size,
+                                       size_t user_offset,
+                                       size_t user_size)
+    : vm_(vm_address, vm_size),
+      data_(reinterpret_cast<const void*>(vm_address + user_offset)),
+      user_size_(user_size) {
+  vm_address_t vm_end = vm_address + vm_size;
+  vm_address_t user_address = reinterpret_cast<vm_address_t>(data_);
+  vm_address_t user_end = user_address + user_size;
+  DCHECK_GE(user_address, vm_address);

[Robert Sesek, 2014/09/10 21:37:40]

DCHECK_EQ(0, vm_address % PAGE_SIZE) ?

[Mark Mentovai, 2014/09/10 22:16:56]

rsesek wrote:
That’s redundant. ScopedMachVM already does it, and its DCHECK will happen in
the initializer before it would be able to run here.

+  DCHECK_LE(user_address, vm_end);
+  DCHECK_GE(user_end, vm_address);
+  DCHECK_LE(user_end, vm_end);
+}
+
+TaskMemory::MappedMemory::~MappedMemory() {
+}
+
+bool TaskMemory::MappedMemory::ReadCString(
+    size_t offset, std::string* string) const {
+  if (offset >= user_size_) {
+    LOG(WARNING) << "offset out of range";
+    return false;
+  }
+
+  const char* string_base = reinterpret_cast<const char*>(data_) + offset;
+  size_t max_length = user_size_ - offset;
+  size_t string_length = strnlen(string_base, max_length);
+  if (string_length == max_length) {
+    LOG(WARNING) << "unterminated string";
+    return false;
+  }
+
+  string->assign(string_base, string_length);
+  return true;
+}
+
 TaskMemory::TaskMemory(mach_port_t task) : task_(task) {
 }
 
 bool TaskMemory::Read(mach_vm_address_t address, size_t size, void* buffer) {
+  scoped_ptr<MappedMemory> memory = ReadMapped(address, size);
+  if (!memory) {
+    return false;
+  }
+
+  memcpy(buffer, memory->data(), size);
+  return true;
+}
+
+scoped_ptr<TaskMemory::MappedMemory> TaskMemory::ReadMapped(
+    mach_vm_address_t address, size_t size) {
   if (size == 0) {
-    return true;
+    return scoped_ptr<MappedMemory>(new MappedMemory(0, 0, 0, 0));
   }
 
   mach_vm_address_t region_address = mach_vm_trunc_page(address);
@@ -45,16 +94,12 @@ bool TaskMemory::Read(mach_vm_address_t address, size_t size, void* buffer) {
   if (kr != KERN_SUCCESS) {
     MACH_LOG(WARNING, kr) << base::StringPrintf(
         "mach_vm_read(0x%llx, 0x%llx)", region_address, region_size);
-    return false;
+    return scoped_ptr<MappedMemory>();
   }
 
   DCHECK_EQ(region_count, region_size);
-  base::mac::ScopedMachVM region_owner(region, region_count);
-
-  const char* region_base = reinterpret_cast<const char*>(region);
-  memcpy(buffer, &region_base[address - region_address], size);
-
-  return true;
+  return scoped_ptr<MappedMemory>(

[Robert Sesek, 2014/09/10 21:37:40]

You don't need .Pass() when returning scoped_ptr<>?

[Mark Mentovai, 2014/09/10 22:16:56]

rsesek wrote:
I don’t think it’s necessary. Look at the comment at scoped_ptr.h:48.

+      new MappedMemory(region, region_size, address - region_address, size));
 }
 
 bool TaskMemory::ReadCString(mach_vm_address_t address, std::string* string) {
@@ -85,16 +130,17 @@ bool TaskMemory::ReadCStringInternal(mach_vm_address_t address,
   do {
     mach_vm_size_t read_length =
         std::min(size, PAGE_SIZE - (read_address % PAGE_SIZE));
-    std::string read_string(read_length, '\0');
-    if (!Read(read_address, read_length, &read_string[0])) {
+    scoped_ptr<MappedMemory> read_region =
+        ReadMapped(read_address, read_length);
+    if (!read_region) {
       return false;
     }
 
-    size_t terminator = read_string.find_first_of('\0');
-    if (terminator == std::string::npos) {
-      local_string.append(read_string);
-    } else {
-      local_string.append(read_string, 0, terminator);
+    const char* read_region_data =
+        reinterpret_cast<const char*>(read_region->data());
+    size_t read_region_data_length = strnlen(read_region_data, read_length);
+    local_string.append(read_region_data, read_region_data_length);
+    if (read_region_data_length < read_length) {
       string->swap(local_string);
       return true;
     }