linux: Enable brokered memory reading

This change:
1. Updates the broker's memory reading protocol to enable short reads.
2. Updates Ptracer to allow short reads.
3. Updates the broker to allow reading from a memory file.
4. Updates the broker's default file root to be "/proc/[pid]/".
5. Adds PtraceConnection::Memory() to produce a suitable memory reader
for a connection type.

Bug: crashpad:30
Change-Id: I8c004016065d981acd1fa74ad1b8e51ce07c7c85
Reviewed-on: https://chromium-review.googlesource.com/991455
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>

snapshot/linux/process_reader_linux.cc

@@ -182,7 +182,6 @@ ProcessReaderLinux::ProcessReaderLinux()
       threads_(),
       modules_(),
       elf_readers_(),
-      process_memory_(),
       is_64_bit_(false),
       initialized_threads_(false),
       initialized_modules_(false),
@@ -203,11 +202,6 @@ bool ProcessReaderLinux::Initialize(PtraceConnection* connection) {
     return false;
   }
 
-  pid_t pid = connection->GetProcessID();
-  if (!process_memory_.Initialize(pid)) {
-    return false;
-  }
-
   is_64_bit_ = process_info_.Is64Bit();
 
   INITIALIZATION_STATE_SET_VALID(initialized_);

snapshot/linux/process_reader_linux.h

@@ -32,7 +32,6 @@
 #include "util/misc/initialization_state_dcheck.h"
 #include "util/posix/process_info.h"
 #include "util/process/process_memory.h"
-#include "util/process/process_memory_linux.h"
 
 namespace crashpad {
 
@@ -103,7 +102,7 @@ class ProcessReaderLinux {
   pid_t ParentProcessID() const { return process_info_.ParentProcessID(); }
 
   //! \brief Return a memory reader for the target process.
-  ProcessMemory* Memory() { return &process_memory_; }
+  ProcessMemory* Memory() { return connection_->Memory(); }
 
   //! \brief Return a memory map of the target process.
   MemoryMap* GetMemoryMap() { return &memory_map_; }
@@ -146,7 +145,6 @@ class ProcessReaderLinux {
   std::vector<Thread> threads_;
   std::vector<Module> modules_;
   std::vector<std::unique_ptr<ElfImageReader>> elf_readers_;
-  ProcessMemoryLinux process_memory_;
   bool is_64_bit_;
   bool initialized_threads_;
   bool initialized_modules_;

test/linux/fake_ptrace_connection.cc

@@ -14,6 +14,8 @@
 
 #include "test/linux/fake_ptrace_connection.h"
 
+#include <utility>
+
 #include "build/build_config.h"
 #include "gtest/gtest.h"
 #include "util/file/file_io.h"
@@ -77,5 +79,18 @@ bool FakePtraceConnection::ReadFileContents(const base::FilePath& path,
   return LoggingReadEntireFile(path, contents);
 }
 
+ProcessMemory* FakePtraceConnection::Memory() {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  if (!memory_) {
+    auto mem = std::make_unique<ProcessMemoryLinux>();
+    if (mem->Initialize(pid_)) {
+      memory_ = std::move(mem);
+    } else {
+      ADD_FAILURE();
+    }
+  }
+  return memory_.get();
+}
+
 }  // namespace test
 }  // namespace crashpad

test/linux/fake_ptrace_connection.h

@@ -17,11 +17,13 @@
 
 #include <sys/types.h>
 
+#include <memory>
 #include <set>
 
 #include "base/macros.h"
 #include "util/linux/ptrace_connection.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/process/process_memory_linux.h"
 
 namespace crashpad {
 namespace test {
@@ -56,8 +58,13 @@ class FakePtraceConnection : public PtraceConnection {
   bool ReadFileContents(const base::FilePath& path,
                         std::string* contents) override;
 
+  //! \brief Attempts to create a ProcessMemory when called, calling
+  //!     ADD_FAILURE() and returning `nullptr` on failure.
+  ProcessMemory* Memory() override;
+
  private:
   std::set<pid_t> attachments_;
+  std::unique_ptr<ProcessMemoryLinux> memory_;
   pid_t pid_;
   bool is_64_bit_;
   InitializationStateDcheck initialized_;

util/linux/direct_ptrace_connection.cc

@@ -23,6 +23,7 @@ namespace crashpad {
 DirectPtraceConnection::DirectPtraceConnection()
     : PtraceConnection(),
       attachments_(),
+      memory_(),
       pid_(-1),
       ptracer_(/* can_log= */ true),
       initialized_() {}
@@ -37,6 +38,10 @@ bool DirectPtraceConnection::Initialize(pid_t pid) {
   }
   pid_ = pid;
 
+  if (!memory_.Initialize(pid)) {
+    return false;
+  }
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
@@ -71,4 +76,9 @@ bool DirectPtraceConnection::ReadFileContents(const base::FilePath& path,
   return LoggingReadEntireFile(path, contents);
 }
 
+ProcessMemory* DirectPtraceConnection::Memory() {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  return &memory_;
+}
+
 }  // namespace crashpad

util/linux/direct_ptrace_connection.h

@@ -25,6 +25,7 @@
 #include "util/linux/ptracer.h"
 #include "util/linux/scoped_ptrace_attach.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/process/process_memory_linux.h"
 
 namespace crashpad {
 
@@ -54,9 +55,11 @@ class DirectPtraceConnection : public PtraceConnection {
   bool GetThreadInfo(pid_t tid, ThreadInfo* info) override;
   bool ReadFileContents(const base::FilePath& path,
                         std::string* contents) override;
+  ProcessMemory* Memory() override;
 
  private:
   std::vector<std::unique_ptr<ScopedPtraceAttach>> attachments_;
+  ProcessMemoryLinux memory_;
   pid_t pid_;
   Ptracer ptracer_;
   InitializationStateDcheck initialized_;

util/linux/exception_handler_client.cc

@@ -130,7 +130,7 @@ int ExceptionHandlerClient::WaitForCrashDumpComplete() {
           constexpr bool am_64_bit = false;
 #endif  // ARCH_CPU_64_BITS
 
-          PtraceBroker broker(server_sock_, am_64_bit);
+          PtraceBroker broker(server_sock_, getppid(), am_64_bit);
           _exit(broker.Run());
         }
 

util/linux/ptrace_broker.cc

@@ -23,24 +23,65 @@
 
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
-#include "util/file/file_io.h"
 
 namespace crashpad {
 
-PtraceBroker::PtraceBroker(int sock, bool is_64_bit)
+namespace {
+
+size_t FormatPID(char* buffer, pid_t pid) {
+  DCHECK_GE(pid, 0);
+
+  char pid_buf[16];
+  size_t length = 0;
+  do {
+    DCHECK_LT(length, sizeof(pid_buf));
+
+    pid_buf[length] = '0' + pid % 10;
+    pid /= 10;
+    ++length;
+  } while (pid > 0);
+
+  for (size_t index = 0; index < length; ++index) {
+    buffer[index] = pid_buf[length - index - 1];
+  }
+
+  return length;
+}
+
+}  // namespace
+
+PtraceBroker::PtraceBroker(int sock, pid_t pid, bool is_64_bit)
     : ptracer_(is_64_bit, /* can_log= */ false),
-      file_root_("/proc/"),
+      file_root_(file_root_buffer_),
       attachments_(nullptr),
       attach_count_(0),
       attach_capacity_(0),
-      sock_(sock) {
+      memory_file_(),
+      sock_(sock),
+      memory_pid_(pid),
+      tried_opening_mem_file_(false) {
   AllocateAttachments();
+
+  static constexpr char kProc[] = "/proc/";
+  size_t root_length = strlen(kProc);
+  memcpy(file_root_buffer_, kProc, root_length);
+
+  if (pid >= 0) {
+    root_length += FormatPID(file_root_buffer_ + root_length, pid);
+    DCHECK_LT(root_length, sizeof(file_root_buffer_));
+    file_root_buffer_[root_length] = '/';
+    ++root_length;
+  }
+
+  DCHECK_LT(root_length, sizeof(file_root_buffer_));
+  file_root_buffer_[root_length] = '\0';
 }
 
 PtraceBroker::~PtraceBroker() = default;
 
 void PtraceBroker::SetFileRoot(const char* new_root) {
   DCHECK_EQ(new_root[strlen(new_root) - 1], '/');
+  memory_pid_ = -1;
   file_root_ = new_root;
 }
 
@@ -189,12 +230,12 @@ int PtraceBroker::SendError(Errno err) {
   return WriteFile(sock_, &err, sizeof(err)) ? 0 : errno;
 }
 
-int PtraceBroker::SendReadError(Errno err) {
+int PtraceBroker::SendReadError(ReadError error) {
   int32_t rv = -1;
-  if (!WriteFile(sock_, &rv, sizeof(rv))) {
+  return WriteFile(sock_, &rv, sizeof(rv)) &&
-    return errno;
+                 WriteFile(sock_, &error, sizeof(error))
-  }
+             ? 0
-  return SendError(err);
+             : errno;
 }
 
 int PtraceBroker::SendOpenResult(OpenResult result) {
@@ -208,7 +249,7 @@ int PtraceBroker::SendFileContents(FileHandle handle) {
     rv = ReadFile(handle, buffer, sizeof(buffer));
 
     if (rv < 0) {
-      return SendReadError(errno);
+      return SendReadError(static_cast<ReadError>(errno));
     }
 
     if (!WriteFile(sock_, &rv, sizeof(rv))) {
@@ -225,25 +266,59 @@ int PtraceBroker::SendFileContents(FileHandle handle) {
   return 0;
 }
 
+void PtraceBroker::TryOpeningMemFile() {
+  if (tried_opening_mem_file_) {
+    return;
+  }
+  tried_opening_mem_file_ = true;
+
+  if (memory_pid_ < 0) {
+    return;
+  }
+
+  char mem_path[32];
+  size_t root_length = strlen(file_root_buffer_);
+  static constexpr char kMem[] = "mem";
+
+  DCHECK_LT(root_length + strlen(kMem) + 1, sizeof(mem_path));
+  memcpy(mem_path, file_root_buffer_, root_length);
+  // Include the trailing NUL.
+  memcpy(mem_path + root_length, kMem, strlen(kMem) + 1);
+  memory_file_.reset(
+      HANDLE_EINTR(open(mem_path, O_RDONLY | O_CLOEXEC | O_NOCTTY)));
+}
+
 int PtraceBroker::SendMemory(pid_t pid, VMAddress address, VMSize size) {
+  if (memory_pid_ >= 0 && pid != memory_pid_) {
+    return SendReadError(kReadErrorAccessDenied);
+  }
+
+  TryOpeningMemFile();
+  auto read_memory = [this, pid](VMAddress address, size_t size, char* buffer) {
+    return this->memory_file_.is_valid()
+               ? HANDLE_EINTR(
+                     pread64(this->memory_file_.get(), buffer, size, address))
+               : this->ptracer_.ReadUpTo(pid, address, size, buffer);
+  };
+
   char buffer[4096];
   while (size > 0) {
-    VMSize bytes_read = std::min(size, VMSize{sizeof(buffer)});
+    size_t to_read = std::min(size, VMSize{sizeof(buffer)});
 
-    if (!ptracer_.ReadMemory(pid, address, bytes_read, buffer)) {
+    int32_t bytes_read = read_memory(address, to_read, buffer);
-      bytes_read = 0;
+
-      Errno error = errno;
+    if (bytes_read < 0) {
-      if (!WriteFile(sock_, &bytes_read, sizeof(bytes_read)) ||
+      return SendReadError(static_cast<ReadError>(errno));
-          !WriteFile(sock_, &error, sizeof(error))) {
-        return errno;
-      }
-      return 0;
     }
 
     if (!WriteFile(sock_, &bytes_read, sizeof(bytes_read))) {
       return errno;
     }
 
+    if (bytes_read == 0) {
+      return 0;
+    }
+
     if (!WriteFile(sock_, buffer, bytes_read)) {
       return errno;
     }

util/linux/ptrace_broker.h

@@ -62,12 +62,9 @@ class PtraceBroker {
       kTypeGetThreadInfo,
 
       //! \brief Reads memory from the attached process. The data is returned in
-      //!     a series of messages. Each message begins with a VMSize indicating
+      //!     a series of messages. Each message begins with an int32_t
-      //!     the number of bytes being returned in this message, followed by
+      //!     indicating the number of bytes read, 0 for end-of-file, or -1 for
-      //!     the requested bytes. The broker continues to send messages until
+      //!     errors, followed by a ReadError. On success the bytes read follow.
-      //!     either all of the requested memory has been sent or an error
-      //!     occurs, in which case it sends a message containing a VMSize equal
-      //!     to zero, followed by an Errno.
       kTypeReadMemory,
 
       //! \brief Read a file's contents. The data is returned in a series of
@@ -98,7 +95,7 @@ class PtraceBroker {
         VMSize size;
       } iov;
 
-      // \brief Specifies the file path to read for a kTypeReadFile request.
+      //! \brief Specifies the file path to read for a kTypeReadFile request.
       struct {
         //! \brief The number of bytes in #path. The path should not include a
         //!     `NUL`-terminator.
@@ -124,6 +121,14 @@ class PtraceBroker {
     kOpenResultSuccess = 0,
   };
 
+  //! \brief A result used in operations that read from memory or files.
+  //!
+  //! Positive values of this enum are reserved for sending errno values.
+  enum ReadError : int32_t {
+    //! \brief Access to this data is denied.
+    kReadErrorAccessDenied = -1,
+  };
+
   //! \brief The response sent for a Request with type kTypeGetThreadInfo.
   struct GetThreadInfoResponse {
     //! \brief Information about the specified thread. Only valid if #success
@@ -139,9 +144,15 @@ class PtraceBroker {
   //!
   //! \param[in] sock A socket on which to read requests from a connected
   //!     PtraceClient. Does not take ownership of the socket.
+  //! \param[in] pid The process ID of the process the broker is expected to
+  //!     trace. Setting this value exends the default file root to
+  //!     "/proc/[pid]/" and enables memory reading via /proc/[pid]/mem. The
+  //!     broker will deny any requests to read memory from processes whose
+  //!     processID is not \a pid. If pid is -1, the broker will serve requests
+  //!     to read memory from any process it is able to via `ptrace PEEKDATA`.
   //! \param[in] is_64_bit Whether this broker should be configured to trace a
   //!     64-bit process.
-  PtraceBroker(int sock, bool is_64_bit);
+  PtraceBroker(int sock, pid_t pid, bool is_64_bit);
 
   ~PtraceBroker();
 
@@ -149,7 +160,10 @@ class PtraceBroker {
   //!     root.
   //!
   //! If this method is not called, the broker defaults to only serving files
-  //! under "/proc/".
+  //! under "/proc/" or "/proc/[pid]/" if a pid was set.
+  //!
+  //! Calling this function disables reading from a memory file if one has not
+  //! already been opened.
   //!
   //! \param[in] root A NUL-terminated c-string containing the path to the new
   //!     root. \a root must not be `nullptr`, must end in a '/', and the caller
@@ -173,18 +187,23 @@ class PtraceBroker {
   void ReleaseAttachments();
   int RunImpl();
   int SendError(Errno err);
-  int SendReadError(Errno err);
+  int SendReadError(ReadError err);
   int SendOpenResult(OpenResult result);
   int SendFileContents(FileHandle handle);
+  void TryOpeningMemFile();
   int SendMemory(pid_t pid, VMAddress address, VMSize size);
   int ReceiveAndOpenFilePath(VMSize path_length, ScopedFileHandle* handle);
 
+  char file_root_buffer_[32];
   Ptracer ptracer_;
   const char* file_root_;
   ScopedPtraceAttach* attachments_;
   size_t attach_count_;
   size_t attach_capacity_;
+  ScopedFileHandle memory_file_;
   int sock_;
+  pid_t memory_pid_;
+  bool tried_opening_mem_file_;
 
   DISALLOW_COPY_AND_ASSIGN(PtraceBroker);
 };

util/linux/ptrace_broker_test.cc

@@ -127,6 +127,85 @@ class SameBitnessTest : public Multiprocess {
   }
 
  private:
+  void BrokerTests(bool set_broker_pid,
+                   LinuxVMAddress child1_tls,
+                   LinuxVMAddress child2_tls,
+                   pid_t child2_tid,
+                   const base::FilePath& file_dir,
+                   const base::FilePath& test_file,
+                   const std::string& expected_file_contents) {
+    int socks[2];
+    ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, socks), 0);
+    ScopedFileHandle broker_sock(socks[0]);
+    ScopedFileHandle client_sock(socks[1]);
+
+#if defined(ARCH_CPU_64_BITS)
+    constexpr bool am_64_bit = true;
+#else
+    constexpr bool am_64_bit = false;
+#endif  // ARCH_CPU_64_BITS
+
+    PtraceBroker broker(
+        broker_sock.get(), set_broker_pid ? ChildPID() : -1, am_64_bit);
+    RunBrokerThread broker_thread(&broker);
+    broker_thread.Start();
+
+    PtraceClient client;
+    ASSERT_TRUE(client.Initialize(
+        client_sock.get(), ChildPID(), /* try_direct_memory= */ false));
+
+    EXPECT_EQ(client.GetProcessID(), ChildPID());
+    EXPECT_TRUE(client.Attach(child2_tid));
+    EXPECT_EQ(client.Is64Bit(), am_64_bit);
+
+    ThreadInfo info1;
+    ASSERT_TRUE(client.GetThreadInfo(ChildPID(), &info1));
+    EXPECT_EQ(info1.thread_specific_data_address, child1_tls);
+
+    ThreadInfo info2;
+    ASSERT_TRUE(client.GetThreadInfo(child2_tid, &info2));
+    EXPECT_EQ(info2.thread_specific_data_address, child2_tls);
+
+    ProcessMemory* memory = client.Memory();
+    ASSERT_TRUE(memory);
+
+    auto buffer = std::make_unique<char[]>(mapping_.len());
+    ASSERT_TRUE(memory->Read(
+        mapping_.addr_as<VMAddress>(), mapping_.len(), buffer.get()));
+    auto expected_buffer = mapping_.addr_as<char*>();
+    for (size_t index = 0; index < mapping_.len(); ++index) {
+      EXPECT_EQ(buffer[index], expected_buffer[index]);
+    }
+
+    char first;
+    ASSERT_TRUE(
+        memory->Read(mapping_.addr_as<VMAddress>(), sizeof(first), &first));
+    EXPECT_EQ(first, expected_buffer[0]);
+
+    char last;
+    ASSERT_TRUE(memory->Read(mapping_.addr_as<VMAddress>() + mapping_.len() - 1,
+                             sizeof(last),
+                             &last));
+    EXPECT_EQ(last, expected_buffer[mapping_.len() - 1]);
+
+    char unmapped;
+    EXPECT_FALSE(memory->Read(mapping_.addr_as<VMAddress>() + mapping_.len(),
+                              sizeof(unmapped),
+                              &unmapped));
+
+    std::string file_root = file_dir.value() + '/';
+    broker.SetFileRoot(file_root.c_str());
+
+    std::string file_contents;
+    ASSERT_TRUE(client.ReadFileContents(test_file, &file_contents));
+    EXPECT_EQ(file_contents, expected_file_contents);
+
+    ScopedTempDir temp_dir2;
+    base::FilePath test_file2(temp_dir2.path().Append("test_file2"));
+    ASSERT_TRUE(CreateFile(test_file2));
+    EXPECT_FALSE(client.ReadFileContents(test_file2, &file_contents));
+  }
+
   void MultiprocessParent() override {
     LinuxVMAddress child1_tls;
     ASSERT_TRUE(LoggingReadFileExactly(
@@ -140,11 +219,6 @@ class SameBitnessTest : public Multiprocess {
     ASSERT_TRUE(LoggingReadFileExactly(
         ReadPipeHandle(), &child2_tls, sizeof(child2_tls)));
 
-    int socks[2];
-    ASSERT_EQ(socketpair(AF_UNIX, SOCK_STREAM, 0, socks), 0);
-    ScopedFileHandle broker_sock(socks[0]);
-    ScopedFileHandle client_sock(socks[1]);
-
     ScopedTempDir temp_dir;
     base::FilePath file_path(temp_dir.path().Append("test_file"));
     std::string expected_file_contents;
@@ -162,67 +236,20 @@ class SameBitnessTest : public Multiprocess {
                                    expected_file_contents.size()));
     }
 
-#if defined(ARCH_CPU_64_BITS)
+    BrokerTests(true,
-    constexpr bool am_64_bit = true;
+                child1_tls,
-#else
+                child2_tls,
-    constexpr bool am_64_bit = false;
+                child2_tid,
-#endif  // ARCH_CPU_64_BITS
+                temp_dir.path(),
-    PtraceBroker broker(broker_sock.get(), am_64_bit);
+                file_path,
-    RunBrokerThread broker_thread(&broker);
+                expected_file_contents);
-    broker_thread.Start();
+    BrokerTests(false,
-
+                child1_tls,
-    {
+                child2_tls,
-      PtraceClient client;
+                child2_tid,
-      ASSERT_TRUE(client.Initialize(client_sock.get(), ChildPID()));
+                temp_dir.path(),
-
+                file_path,
-      EXPECT_EQ(client.GetProcessID(), ChildPID());
+                expected_file_contents);
-      EXPECT_TRUE(client.Attach(child2_tid));
-      EXPECT_EQ(client.Is64Bit(), am_64_bit);
-
-      ThreadInfo info1;
-      ASSERT_TRUE(client.GetThreadInfo(ChildPID(), &info1));
-      EXPECT_EQ(info1.thread_specific_data_address, child1_tls);
-
-      ThreadInfo info2;
-      ASSERT_TRUE(client.GetThreadInfo(child2_tid, &info2));
-      EXPECT_EQ(info2.thread_specific_data_address, child2_tls);
-
-      auto buffer = std::make_unique<char[]>(mapping_.len());
-      ASSERT_TRUE(client.Read(
-          mapping_.addr_as<VMAddress>(), mapping_.len(), buffer.get()));
-      auto expected_buffer = mapping_.addr_as<char*>();
-      for (size_t index = 0; index < mapping_.len(); ++index) {
-        EXPECT_EQ(buffer[index], expected_buffer[index]);
-      }
-
-      char first;
-      ASSERT_TRUE(
-          client.Read(mapping_.addr_as<VMAddress>(), sizeof(first), &first));
-      EXPECT_EQ(first, expected_buffer[0]);
-
-      char last;
-      ASSERT_TRUE(
-          client.Read(mapping_.addr_as<VMAddress>() + mapping_.len() - 1,
-                      sizeof(last),
-                      &last));
-      EXPECT_EQ(last, expected_buffer[mapping_.len() - 1]);
-
-      char unmapped;
-      EXPECT_FALSE(client.Read(mapping_.addr_as<VMAddress>() + mapping_.len(),
-                               sizeof(unmapped),
-                               &unmapped));
-
-      std::string file_root = temp_dir.path().value() + '/';
-      broker.SetFileRoot(file_root.c_str());
-      std::string file_contents;
-      ASSERT_TRUE(client.ReadFileContents(file_path, &file_contents));
-      EXPECT_EQ(file_contents, expected_file_contents);
-
-      ScopedTempDir temp_dir2;
-      base::FilePath test_file2(temp_dir2.path().Append("test_file2"));
-      ASSERT_TRUE(CreateFile(test_file2));
-      EXPECT_FALSE(client.ReadFileContents(test_file2, &file_contents));
-    }
   }
 
   void MultiprocessChild() override {

util/linux/ptrace_client.cc

@@ -15,12 +15,14 @@
 #include "util/linux/ptrace_client.h"
 
 #include <errno.h>
+#include <stdio.h>
 
 #include <string>
 
 #include "base/logging.h"
 #include "util/file/file_io.h"
 #include "util/linux/ptrace_broker.h"
+#include "util/process/process_memory_linux.h"
 
 namespace crashpad {
 
@@ -36,6 +38,27 @@ bool ReceiveAndLogError(int sock, const std::string& operation) {
   return true;
 }
 
+bool ReceiveAndLogReadError(int sock, const std::string& operation) {
+  PtraceBroker::ReadError err;
+  if (!LoggingReadFileExactly(sock, &err, sizeof(err))) {
+    return false;
+  }
+  switch (err) {
+    case PtraceBroker::kReadErrorAccessDenied:
+      LOG(ERROR) << operation << " access denied";
+      return true;
+    default:
+      if (err <= 0) {
+        LOG(ERROR) << operation << " invalid error " << err;
+        DCHECK(false);
+        return false;
+      }
+      errno = err;
+      PLOG(ERROR) << operation;
+      return true;
+  }
+}
+
 bool AttachImpl(int sock, pid_t tid) {
   PtraceBroker::Request request;
   request.type = PtraceBroker::Request::kTypeAttach;
@@ -61,6 +84,7 @@ bool AttachImpl(int sock, pid_t tid) {
 
 PtraceClient::PtraceClient()
     : PtraceConnection(),
+      memory_(),
       sock_(kInvalidFileHandle),
       pid_(-1),
       is_64_bit_(false),
@@ -74,7 +98,7 @@ PtraceClient::~PtraceClient() {
   }
 }
 
-bool PtraceClient::Initialize(int sock, pid_t pid) {
+bool PtraceClient::Initialize(int sock, pid_t pid, bool try_direct_memory) {
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
   sock_ = sock;
   pid_ = pid;
@@ -97,46 +121,20 @@ bool PtraceClient::Initialize(int sock, pid_t pid) {
   }
   is_64_bit_ = is_64_bit == kBoolTrue;
 
+  if (try_direct_memory) {
+    auto direct_mem = std::make_unique<ProcessMemoryLinux>();
+    if (direct_mem->Initialize(pid)) {
+      memory_.reset(direct_mem.release());
+    }
+  }
+  if (!memory_) {
+    memory_ = std::make_unique<BrokeredMemory>(this);
+  }
+
   INITIALIZATION_STATE_SET_VALID(initialized_);
   return true;
 }
 
-bool PtraceClient::Read(VMAddress address, size_t size, void* buffer) {
-  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
-  char* buffer_c = reinterpret_cast<char*>(buffer);
-
-  PtraceBroker::Request request;
-  request.type = PtraceBroker::Request::kTypeReadMemory;
-  request.tid = pid_;
-  request.iov.base = address;
-  request.iov.size = size;
-
-  if (!LoggingWriteFile(sock_, &request, sizeof(request))) {
-    return false;
-  }
-
-  while (size > 0) {
-    VMSize bytes_read;
-    if (!LoggingReadFileExactly(sock_, &bytes_read, sizeof(bytes_read))) {
-      return false;
-    }
-
-    if (!bytes_read) {
-      ReceiveAndLogError(sock_, "PtraceBroker ReadMemory");
-      return false;
-    }
-
-    if (!LoggingReadFileExactly(sock_, buffer_c, bytes_read)) {
-      return false;
-    }
-
-    size -= bytes_read;
-    buffer_c += bytes_read;
-  }
-
-  return true;
-}
-
 pid_t PtraceClient::GetProcessID() {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   return pid_;
@@ -197,7 +195,7 @@ bool PtraceClient::ReadFileContents(const base::FilePath& path,
     }
 
     if (read_result < 0) {
-      ReceiveAndLogError(sock_, "ReadFileContents");
+      ReceiveAndLogReadError(sock_, "ReadFileContents");
       return false;
     }
 
@@ -215,6 +213,66 @@ bool PtraceClient::ReadFileContents(const base::FilePath& path,
   return true;
 }
 
+ProcessMemory* PtraceClient::Memory() {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  return memory_.get();
+}
+
+PtraceClient::BrokeredMemory::BrokeredMemory(PtraceClient* client)
+    : ProcessMemory(), client_(client) {}
+
+PtraceClient::BrokeredMemory::~BrokeredMemory() = default;
+
+ssize_t PtraceClient::BrokeredMemory::ReadUpTo(VMAddress address,
+                                               size_t size,
+                                               void* buffer) const {
+  return client_->ReadUpTo(address, size, buffer);
+}
+
+ssize_t PtraceClient::ReadUpTo(VMAddress address,
+                               size_t size,
+                               void* buffer) const {
+  INITIALIZATION_STATE_DCHECK_VALID(initialized_);
+  char* buffer_c = reinterpret_cast<char*>(buffer);
+
+  PtraceBroker::Request request;
+  request.type = PtraceBroker::Request::kTypeReadMemory;
+  request.tid = pid_;
+  request.iov.base = address;
+  request.iov.size = size;
+
+  if (!LoggingWriteFile(sock_, &request, sizeof(request))) {
+    return false;
+  }
+
+  ssize_t total_read = 0;
+  while (size > 0) {
+    int32_t bytes_read;
+    if (!LoggingReadFileExactly(sock_, &bytes_read, sizeof(bytes_read))) {
+      return -1;
+    }
+
+    if (bytes_read < 0) {
+      ReceiveAndLogReadError(sock_, "PtraceBroker ReadMemory");
+      return -1;
+    }
+
+    if (bytes_read == 0) {
+      return total_read;
+    }
+
+    if (!LoggingReadFileExactly(sock_, buffer_c, bytes_read)) {
+      return -1;
+    }
+
+    size -= bytes_read;
+    buffer_c += bytes_read;
+    total_read += bytes_read;
+  }
+
+  return total_read;
+}
+
 bool PtraceClient::SendFilePath(const char* path, size_t length) {
   if (!LoggingWriteFile(sock_, path, length)) {
     return false;

util/linux/ptrace_client.h

@@ -17,10 +17,13 @@
 
 #include <sys/types.h>
 
+#include <memory>
+
 #include "base/macros.h"
 #include "util/linux/ptrace_connection.h"
 #include "util/misc/address_types.h"
 #include "util/misc/initialization_state_dcheck.h"
+#include "util/process/process_memory.h"
 
 namespace crashpad {
 
@@ -43,26 +46,11 @@ class PtraceClient : public PtraceConnection {
   //!     ownership of the socket.
   //! \param[in] pid The process ID of the process to form a PtraceConnection
   //!     with.
+  //! \param[in] try_direct_memory If `true` the client will attempt to support
+  //!     memory reading operations by directly acessing the target process'
+  //!     /proc/[pid]/mem file.
   //! \return `true` on success. `false` on failure with a message logged.
-  bool Initialize(int sock, pid_t pid);
+  bool Initialize(int sock, pid_t pid, bool try_direct_memory = true);
-
-  //! \brief Copies memory from the target process into a caller-provided buffer
-  //!     in the current process.
-  //!
-  //! TODO(jperaza): In order for this to be usable, PtraceConnection will need
-  //! to surface it, possibly by inheriting from ProcessMemory, or providing a
-  //! method to return a ProcessMemory*.
-  //!
-  //! \param[in] address The address, in the target process' address space, of
-  //!     the memory region to copy.
-  //! \param[in] size The size, in bytes, of the memory region to copy.
-  //!     \a buffer must be at least this size.
-  //! \param[out] buffer The buffer into which the contents of the other
-  //!     process' memory will be copied.
-  //!
-  //! \return `true` on success, with \a buffer filled appropriately. `false` on
-  //!     failure, with a message logged.
-  bool Read(VMAddress address, size_t size, void* buffer);
 
   // PtraceConnection:
 
@@ -72,10 +60,28 @@ class PtraceClient : public PtraceConnection {
   bool GetThreadInfo(pid_t tid, ThreadInfo* info) override;
   bool ReadFileContents(const base::FilePath& path,
                         std::string* contents) override;
+  ProcessMemory* Memory() override;
 
  private:
+  class BrokeredMemory : public ProcessMemory {
+   public:
+    explicit BrokeredMemory(PtraceClient* client);
+    ~BrokeredMemory();
+
+    ssize_t ReadUpTo(VMAddress address,
+                     size_t size,
+                     void* buffer) const override;
+
+   private:
+    PtraceClient* client_;
+
+    DISALLOW_COPY_AND_ASSIGN(BrokeredMemory);
+  };
+
+  ssize_t ReadUpTo(VMAddress address, size_t size, void* buffer) const;
   bool SendFilePath(const char* path, size_t length);
 
+  std::unique_ptr<ProcessMemory> memory_;
   int sock_;
   pid_t pid_;
   bool is_64_bit_;

util/linux/ptrace_connection.h

@@ -21,6 +21,7 @@
 
 #include "base/files/file_path.h"
 #include "util/linux/thread_info.h"
+#include "util/process/process_memory.h"
 
 namespace crashpad {
 
@@ -57,6 +58,12 @@ class PtraceConnection {
   //! \return `true` on success. `false` on failure with a message logged.
   virtual bool ReadFileContents(const base::FilePath& path,
                                 std::string* contents) = 0;
+
+  //! \brief Returns a memory reader for the connected process.
+  //!
+  //! The caller does not take ownership of the reader. The reader is valid for
+  //! the lifetime of the PtraceConnection that created it.
+  virtual ProcessMemory* Memory() = 0;
 };
 
 }  // namespace crashpad

util/linux/ptracer.cc

@@ -375,10 +375,11 @@ bool Ptracer::GetThreadInfo(pid_t tid, ThreadInfo* info) {
                          can_log_);
 }
 
-bool Ptracer::ReadMemory(pid_t pid,
+ssize_t Ptracer::ReadUpTo(pid_t pid,
-                         LinuxVMAddress address,
+                          LinuxVMAddress address,
-                         size_t size,
+                          size_t size,
-                         char* buffer) {
+                          char* buffer) {
+  size_t bytes_read = 0;
   while (size > 0) {
     errno = 0;
 
@@ -386,46 +387,66 @@ bool Ptracer::ReadMemory(pid_t pid,
       *reinterpret_cast<long*>(buffer) =
           ptrace(PTRACE_PEEKDATA, pid, address, nullptr);
 
+      if (errno == EIO) {
+        ssize_t last_bytes = ReadLastBytes(pid, address, size, buffer);
+        return last_bytes >= 0 ? bytes_read + last_bytes : -1;
+      }
+
       if (errno != 0) {
         PLOG_IF(ERROR, can_log_) << "ptrace";
-        return false;
+        return -1;
       }
 
       size -= sizeof(long);
       buffer += sizeof(long);
       address += sizeof(long);
+      bytes_read += sizeof(long);
     } else {
       long word = ptrace(PTRACE_PEEKDATA, pid, address, nullptr);
 
       if (errno == 0) {
         memcpy(buffer, reinterpret_cast<char*>(&word), size);
-        return true;
+        return bytes_read + size;
       }
 
-      if (errno != EIO) {
+      if (errno == EIO) {
-        PLOG_IF(ERROR, can_log_);
+        ssize_t last_bytes = ReadLastBytes(pid, address, size, buffer);
-        return false;
+        return last_bytes >= 0 ? bytes_read + last_bytes : -1;
-      }
-
-      // A read smaller than a word at the end of a mapping might spill over
-      // into unmapped memory. Try aligning the read so that the requested
-      // data is at the end of the word instead.
-      errno = 0;
-      word =
-          ptrace(PTRACE_PEEKDATA, pid, address - sizeof(word) + size, nullptr);
-
-      if (errno == 0) {
-        memcpy(
-            buffer, reinterpret_cast<char*>(&word) + sizeof(word) - size, size);
-        return true;
       }
 
       PLOG_IF(ERROR, can_log_);
-      return false;
+      return -1;
     }
   }
 
-  return true;
+  return bytes_read;
+}
+
+// Handles an EIO by reading at most size bytes from address into buffer if
+// address was within a word of a possible page boundary, by aligning to read
+// the last word of the page and extracting the desired bytes.
+ssize_t Ptracer::ReadLastBytes(pid_t pid,
+                               LinuxVMAddress address,
+                               size_t size,
+                               char* buffer) {
+  LinuxVMAddress aligned = ((address + 4095) & ~4095) - sizeof(long);
+  if (aligned >= address || aligned == address - sizeof(long)) {
+    PLOG_IF(ERROR, can_log_) << "ptrace";
+    return -1;
+  }
+  DCHECK_GT(aligned, address - sizeof(long));
+
+  errno = 0;
+  long word = ptrace(PTRACE_PEEKDATA, pid, aligned, nullptr);
+  if (errno != 0) {
+    PLOG_IF(ERROR, can_log_) << "ptrace";
+    return -1;
+  }
+
+  size_t bytes_read = address - aligned;
+  size_t last_bytes = std::min(sizeof(long) - bytes_read, size);
+  memcpy(buffer, reinterpret_cast<char*>(&word) + bytes_read, last_bytes);
+  return last_bytes;
 }
 
 }  // namespace crashpad

util/linux/ptracer.h

@@ -72,20 +72,29 @@ class Ptracer {
   bool GetThreadInfo(pid_t tid, ThreadInfo* info);
 
   //! \brief Uses `ptrace` to read memory from the process with process ID \a
-  //!     pid.
+  //!     pid, up to a maximum number of bytes.
   //!
   //! The target process should already be attached before calling this method.
   //! \see ScopedPtraceAttach
   //!
   //! \param[in] pid The process ID whose memory to read.
   //! \param[in] address The base address of the region to read.
-  //! \param[in] size The size of the memory region to read.
+  //! \param[in] size The size of the memory region to read. \a buffer must be
+  //!     at least this size.
   //! \param[out] buffer The buffer to fill with the data read.
-  //! \return `true` on success. `false` on failure with a message logged, if
+  //! \return the number of bytes read, 0 if there are no more bytes to read, or
-  //!     enabled.
+  //!     -1 on failure with a message logged if logging is enabled.
-  bool ReadMemory(pid_t pid, LinuxVMAddress address, size_t size, char* buffer);
+  ssize_t ReadUpTo(pid_t pid,
+                   LinuxVMAddress address,
+                   size_t size,
+                   char* buffer);
 
  private:
+  ssize_t ReadLastBytes(pid_t pid,
+                        LinuxVMAddress address,
+                        size_t size,
+                        char* buffer);
+
   bool is_64_bit_;
   bool can_log_;
   InitializationStateDcheck initialized_;

util/process/process_memory.h

@@ -78,9 +78,10 @@ class ProcessMemory {
     return ReadCStringInternal(address, true, size, string);
   }
 
+  virtual ~ProcessMemory() = default;
+
  protected:
   ProcessMemory() = default;
-  ~ProcessMemory() = default;
 
  private:
   //! \brief Copies memory from the target process into a caller-provided buffer