linux: Use mmap for attachments in PtraceBroker

The broker attempts to use sbrk() to allocate memory to track ptrace
attachments. If the process failed due to an OOM, this system call might
fail, the broker falls back to saving attachments on the stack, and then
overruns the stack.

This change updates the broker to use sys_mmap() instead of sbrk(),
which is expected to work at least as well. If sys_mmap() fails or
the first mapped page is exhausted, further attachments fail without
attempting to save them to the stack.

Bug: chromium:1128441
Change-Id: Ibffaa986403adaf3178ee77e6d210053fbf60f26
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/2488280
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>

util/linux/ptrace_broker.cc

@@ -17,6 +17,7 @@
 #include <fcntl.h>
 #include <limits.h>
 #include <string.h>
+#include <sys/mman.h>
 #include <syscall.h>
 #include <unistd.h>
 
@@ -24,7 +25,11 @@
 
 #include "base/check_op.h"
 #include "base/posix/eintr_wrapper.h"
+#include "base/process/process_metrics.h"
+#include "third_party/lss/lss.h"
+#include "util/linux/scoped_ptrace_attach.h"
 #include "util/misc/memory_sanitizer.h"
+#include "util/posix/scoped_mmap.h"
 
 namespace crashpad {
 
@@ -52,18 +57,58 @@ size_t FormatPID(char* buffer, pid_t pid) {
 
 }  // namespace
 
+class PtraceBroker::AttachmentsArray {
+ public:
+  AttachmentsArray() : allocation_(false), attach_count_(0) {}
+
+  ~AttachmentsArray() {
+    for (size_t index = 0; index < attach_count_; ++index) {
+      PtraceDetach(Attachments()[index], false);
+    }
+  }
+
+  bool Initialize() {
+    return allocation_.ResetMmap(nullptr,
+                                 base::GetPageSize(),
+                                 PROT_READ | PROT_WRITE,
+                                 MAP_PRIVATE | MAP_ANONYMOUS,
+                                 -1,
+                                 0);
+  }
+
+  bool Attach(pid_t pid) {
+    pid_t* attach = AllocateAttachment();
+    if (!attach || !PtraceAttach(pid, false)) {
+      return false;
+    }
+
+    *attach = pid;
+    return true;
+  }
+
+ private:
+  pid_t* AllocateAttachment() {
+    if (attach_count_ >= (allocation_.len() / sizeof(pid_t))) {
+      return nullptr;
+    }
+    return &Attachments()[attach_count_++];
+  }
+
+  pid_t* Attachments() { return allocation_.addr_as<pid_t*>(); }
+
+  ScopedMmap allocation_;
+  size_t attach_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(AttachmentsArray);
+};
+
 PtraceBroker::PtraceBroker(int sock, pid_t pid, bool is_64_bit)
     : ptracer_(is_64_bit, /* can_log= */ false),
       file_root_(file_root_buffer_),
-      attachments_(nullptr),
-      attach_count_(0),
-      attach_capacity_(0),
       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);
@@ -88,35 +133,12 @@ void PtraceBroker::SetFileRoot(const char* new_root) {
 }
 
 int PtraceBroker::Run() {
-  int result = RunImpl();
-  ReleaseAttachments();
-  return result;
+  AttachmentsArray attachments;
+  attachments.Initialize();
+  return RunImpl(&attachments);
 }
 
-bool PtraceBroker::AllocateAttachments() {
-  constexpr size_t page_size = 4096;
-  constexpr size_t alloc_size =
-      (sizeof(ScopedPtraceAttach) + page_size - 1) & ~(page_size - 1);
-  void* alloc = sbrk(alloc_size);
-  if (reinterpret_cast<intptr_t>(alloc) == -1) {
-    return false;
-  }
-
-  if (attachments_ == nullptr) {
-    attachments_ = reinterpret_cast<ScopedPtraceAttach*>(alloc);
-  }
-
-  attach_capacity_ += alloc_size / sizeof(ScopedPtraceAttach);
-  return true;
-}
-
-void PtraceBroker::ReleaseAttachments() {
-  for (size_t index = 0; index < attach_count_; ++index) {
-    attachments_[index].Reset();
-  }
-}
-
-int PtraceBroker::RunImpl() {
+int PtraceBroker::RunImpl(AttachmentsArray* attachments) {
   while (true) {
     Request request = {};
     if (!ReadFileExactly(sock_, &request, sizeof(request))) {
@@ -129,25 +151,10 @@ int PtraceBroker::RunImpl() {
 
     switch (request.type) {
       case Request::kTypeAttach: {
-        ScopedPtraceAttach* attach;
-        ScopedPtraceAttach stack_attach;
-        bool attach_on_stack = false;
-
-        if (attach_capacity_ > attach_count_ || AllocateAttachments()) {
-          attach = new (&attachments_[attach_count_]) ScopedPtraceAttach;
-        } else {
-          attach = &stack_attach;
-          attach_on_stack = true;
-        }
-
         ExceptionHandlerProtocol::Bool status =
-            ExceptionHandlerProtocol::kBoolFalse;
-        if (attach->ResetAttach(request.tid)) {
-          status = ExceptionHandlerProtocol::kBoolTrue;
-          if (!attach_on_stack) {
-            ++attach_count_;
-          }
-        }
+            attachments->Attach(request.tid)
+                ? ExceptionHandlerProtocol::kBoolTrue
+                : ExceptionHandlerProtocol::kBoolFalse;
 
         if (!WriteFile(sock_, &status, sizeof(status))) {
           return errno;
@@ -160,9 +167,6 @@ int PtraceBroker::RunImpl() {
           }
         }
 
-        if (attach_on_stack && status == ExceptionHandlerProtocol::kBoolTrue) {
-          return RunImpl();
-        }
         continue;
       }
 

util/linux/ptrace_broker.h

@@ -24,7 +24,6 @@
 #include "util/linux/exception_handler_protocol.h"
 #include "util/linux/ptrace_connection.h"
 #include "util/linux/ptracer.h"
-#include "util/linux/scoped_ptrace_attach.h"
 #include "util/linux/thread_info.h"
 #include "util/misc/address_types.h"
 
@@ -186,16 +185,13 @@ class PtraceBroker {
   //! This method returns when a PtraceBrokerRequest with type kTypeExit is
   //! received or an error is encountered on the socket.
   //!
-  //! This method calls `sbrk`, which may break other memory management tools,
-  //! such as `malloc`.
-  //!
   //! \return 0 if Run() exited due to an exit request. Otherwise an error code.
   int Run();
 
  private:
-  bool AllocateAttachments();
-  void ReleaseAttachments();
-  int RunImpl();
+  class AttachmentsArray;
+
+  int RunImpl(AttachmentsArray*);
   int SendError(ExceptionHandlerProtocol::Errno err);
   int SendReadError(ReadError err);
   int SendOpenResult(OpenResult result);
@@ -210,9 +206,6 @@ class PtraceBroker {
   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_;

util/linux/scoped_ptrace_attach.cc

@@ -22,6 +22,32 @@
 
 namespace crashpad {
 
+bool PtraceAttach(pid_t pid, bool can_log) {
+  if (ptrace(PTRACE_ATTACH, pid, nullptr, nullptr) != 0) {
+    PLOG_IF(ERROR, can_log) << "ptrace";
+    return false;
+  }
+
+  int status;
+  if (HANDLE_EINTR(waitpid(pid, &status, __WALL)) < 0) {
+    PLOG_IF(ERROR, can_log) << "waitpid";
+    return false;
+  }
+  if (!WIFSTOPPED(status)) {
+    LOG_IF(ERROR, can_log) << "process not stopped";
+    return false;
+  }
+  return true;
+}
+
+bool PtraceDetach(pid_t pid, bool can_log) {
+  if (pid >= 0 && ptrace(PTRACE_DETACH, pid, nullptr, nullptr) != 0) {
+    PLOG_IF(ERROR, can_log) << "ptrace";
+    return false;
+  }
+  return true;
+}
+
 ScopedPtraceAttach::ScopedPtraceAttach()
     : pid_(-1) {}
 
@@ -30,8 +56,7 @@ ScopedPtraceAttach::~ScopedPtraceAttach() {
 }
 
 bool ScopedPtraceAttach::Reset() {
-  if (pid_ >= 0 && ptrace(PTRACE_DETACH, pid_, nullptr, nullptr) != 0) {
-    PLOG(ERROR) << "ptrace";
+  if (!PtraceDetach(pid_, true)) {
     return false;
   }
   pid_ = -1;
@@ -41,21 +66,11 @@ bool ScopedPtraceAttach::Reset() {
 bool ScopedPtraceAttach::ResetAttach(pid_t pid) {
   Reset();
 
-  if (ptrace(PTRACE_ATTACH, pid, nullptr, nullptr) != 0) {
-    PLOG(ERROR) << "ptrace";
+  if (!PtraceAttach(pid, true)) {
     return false;
   }
-  pid_ = pid;
 
-  int status;
-  if (HANDLE_EINTR(waitpid(pid_, &status, __WALL)) < 0) {
-    PLOG(ERROR) << "waitpid";
-    return false;
-  }
-  if (!WIFSTOPPED(status)) {
-    LOG(ERROR) << "process not stopped";
-    return false;
-  }
+  pid_ = pid;
   return true;
 }
 

util/linux/scoped_ptrace_attach.h

@@ -21,6 +21,24 @@
 
 namespace crashpad {
 
+//! \brief Attaches to the process with process ID \a pid and blocks until the
+//!     target process has stopped by calling `waitpid()`.
+//!
+//! \param pid The process ID of the process to attach to.
+//! \param can_log Whether this function may log messages on failure.
+//! \return `true` on success. `false` on failure with a message logged if \a
+//!     can_log is `true`.
+bool PtraceAttach(pid_t pid, bool can_log = true);
+
+//! \brief Detaches the process  with process ID \a pid. The process must
+//!     already be ptrace attached.
+//!
+//! \param pid The process ID of the process to detach.
+//! \param can_log Whether this function may log messages on failure.
+//! \return `true` on success. `false` on failure with a message logged if \a
+//!     ca_log is `true `true`
+bool PtraceDetach(pid_t pid, bool can_log = true);
+
 //! \brief Maintains a `ptrace()` attachment to a process.
 //!
 //! On destruction, the process will be detached.

util/posix/scoped_mmap.cc

@@ -22,12 +22,54 @@
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/numerics/safe_math.h"
+#include "base/process/process_metrics.h"
+#include "build/build_config.h"
+
+#if defined(OS_LINUX)
+#include "third_party/lss/lss.h"
+#endif
 
 namespace {
 
-bool Munmap(uintptr_t addr, size_t len) {
-  if (munmap(reinterpret_cast<void*>(addr), len) != 0) {
-    PLOG(ERROR) << "munmap";
+#if defined(OS_LINUX)
+void* CallMmap(void* addr,
+               size_t len,
+               int prot,
+               int flags,
+               int fd,
+               off_t offset) {
+  return sys_mmap(addr, len, prot, flags, fd, offset);
+}
+
+int CallMunmap(void* addr, size_t len) {
+  return sys_munmap(addr, len);
+}
+
+int CallMprotect(void* addr, size_t len, int prot) {
+  return sys_mprotect(addr, len, prot);
+}
+#else
+void* CallMmap(void* addr,
+               size_t len,
+               int prot,
+               int flags,
+               int fd,
+               off_t offset) {
+  return mmap(addr, len, prot, flags, fd, offset);
+}
+
+int CallMunmap(void* addr, size_t len) {
+  return munmap(addr, len);
+}
+
+int CallMprotect(void* addr, size_t len, int prot) {
+  return mprotect(addr, len, prot);
+}
+#endif
+
+bool LoggingMunmap(uintptr_t addr, size_t len, bool can_log) {
+  if (CallMunmap(reinterpret_cast<void*>(addr), len) != 0) {
+    PLOG_IF(ERROR, can_log) << "munmap";
     return false;
   }
 
@@ -35,7 +77,7 @@ bool Munmap(uintptr_t addr, size_t len) {
 }
 
 size_t RoundPage(size_t size) {
-  const size_t kPageMask = base::checked_cast<size_t>(getpagesize()) - 1;
+  const size_t kPageMask = base::checked_cast<size_t>(base::GetPageSize()) - 1;
   return (size + kPageMask) & ~kPageMask;
 }
 
@@ -43,11 +85,12 @@ size_t RoundPage(size_t size) {
 
 namespace crashpad {
 
-ScopedMmap::ScopedMmap() {}
+ScopedMmap::ScopedMmap(bool can_log) : can_log_(can_log) {}
 
 ScopedMmap::~ScopedMmap() {
   if (is_valid()) {
-    Munmap(reinterpret_cast<uintptr_t>(addr_), RoundPage(len_));
+    LoggingMunmap(
+        reinterpret_cast<uintptr_t>(addr_), RoundPage(len_), can_log_);
   }
 }
 
@@ -63,7 +106,7 @@ bool ScopedMmap::ResetAddrLen(void* addr, size_t len) {
     DCHECK_EQ(len, 0u);
   } else {
     DCHECK_NE(len, 0u);
-    DCHECK_EQ(new_addr % getpagesize(), 0u);
+    DCHECK_EQ(new_addr % base::GetPageSize(), 0u);
     DCHECK((base::CheckedNumeric<uintptr_t>(new_addr) + (new_len_round - 1))
                .IsValid());
   }
@@ -74,11 +117,13 @@ bool ScopedMmap::ResetAddrLen(void* addr, size_t len) {
     const uintptr_t old_addr = reinterpret_cast<uintptr_t>(addr_);
     const size_t old_len_round = RoundPage(len_);
     if (old_addr < new_addr) {
-      result &= Munmap(old_addr, std::min(old_len_round, new_addr - old_addr));
+      result &= LoggingMunmap(
+          old_addr, std::min(old_len_round, new_addr - old_addr), can_log_);
     }
     if (old_addr + old_len_round > new_addr + new_len_round) {
       uintptr_t unmap_start = std::max(old_addr, new_addr + new_len_round);
-      result &= Munmap(unmap_start, old_addr + old_len_round - unmap_start);
+      result &= LoggingMunmap(
+          unmap_start, old_addr + old_len_round - unmap_start, can_log_);
     }
   }
 
@@ -100,9 +145,9 @@ bool ScopedMmap::ResetMmap(void* addr,
   // consider the return value from Reset().
   Reset();
 
-  void* new_addr = mmap(addr, len, prot, flags, fd, offset);
+  void* new_addr = CallMmap(addr, len, prot, flags, fd, offset);
   if (new_addr == MAP_FAILED) {
-    PLOG(ERROR) << "mmap";
+    PLOG_IF(ERROR, can_log_) << "mmap";
     return false;
   }
 
@@ -113,8 +158,8 @@ bool ScopedMmap::ResetMmap(void* addr,
 }
 
 bool ScopedMmap::Mprotect(int prot) {
-  if (mprotect(addr_, RoundPage(len_), prot) < 0) {
-    PLOG(ERROR) << "mprotect";
+  if (CallMprotect(addr_, RoundPage(len_), prot) < 0) {
+    PLOG_IF(ERROR, can_log_) << "mprotect";
     return false;
   }
 

util/posix/scoped_mmap.h

@@ -30,7 +30,10 @@ namespace crashpad {
 //! will be released by calling `munmap()`.
 class ScopedMmap {
  public:
-  ScopedMmap();
+  //! \brief Constructs this object.
+  //!
+  //! \param can_log `true` if methods of this class may log messages.
+  explicit ScopedMmap(bool can_log = true);
   ~ScopedMmap();
 
   //! \brief Releases the memory-mapped region by calling `munmap()`.
@@ -105,6 +108,7 @@ class ScopedMmap {
  private:
   void* addr_ = MAP_FAILED;
   size_t len_ = 0;
+  bool can_log_;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedMmap);
 };