mac: Don’t leak send rights from ExceptionPorts::GetExceptionPorts()

ExceptionPorts::GetExceptionPorts() returned a
std::vector<ExceptionPorts::ExceptionHandler>, which contained send
rights to Mach ports. The interface required callers to assume ownership
of each send right contained within the vector. This was cumbersome and
error-prone, and despite the care taken in Crashpad, port right leaks
did occur:

 - SimulateCrash() didn’t make any attempt to release these resources at
   all.
 - Neither did crashpad_util_test ExceptionPorts.HostExceptionPorts,
   which also reused a vector.

This replaces the vector with the interface-compatible (as far as
necessary) ExceptionPorts::ExceptionHandlerVector, which deallocates
collected port rights on destruction or clear().

R=rsesek@chromium.org

Review URL: https://codereview.chromium.org/1381023007 .

client/simulate_crash_mac.cc

@@ -16,8 +16,6 @@
 
 #include <string.h>
 
-#include <vector>
-
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/mac/mach_logging.h"
@@ -216,7 +214,7 @@ void SimulateCrash(const NativeCPUContext& cpu_context) {
   for (size_t target_type_index = 0;
        !success && target_type_index < arraysize(kTargetTypes);
        ++target_type_index) {
-    std::vector<ExceptionPorts::ExceptionHandler> handlers;
+    ExceptionPorts::ExceptionHandlerVector handlers;
     ExceptionPorts exception_ports(kTargetTypes[target_type_index],
                                    MACH_PORT_NULL);
     if (exception_ports.GetExceptionPorts(EXC_MASK_CRASH, &handlers)) {

tools/mac/exception_port_tool.cc

@@ -74,13 +74,20 @@ class MachSendRightPool {
   //! \brief Adds a send right to the pool.
   //!
   //! \param[in] send_right The send right to be added. The pool object takes
-  //!     ownership of the send right, which remains valid until the pool object
+  //!     its own reference to the send right, which remains valid until the
-  //!     is destroyed.
+  //!     pool object is destroyed. The caller remains responsible for its
+  //!     reference to the send right.
   //!
   //! It is possible and in fact likely that one pool will wind up owning the
   //! same send right multiple times. This is acceptable, because send rights
   //! are reference-counted.
   void AddSendRight(mach_port_t send_right) {
+    kern_return_t kr = mach_port_mod_refs(mach_task_self(),
+                                          send_right,
+                                          MACH_PORT_RIGHT_SEND,
+                                          1);
+    MACH_CHECK(kr == KERN_SUCCESS, kr) << "mach_port_mod_refs";
+
     send_rights_.push_back(send_right);
   }
 
@@ -188,9 +195,9 @@ void ShowBootstrapService(const std::string& service_name,
     return;
   }
 
-  printf("service %s %#x\n", service_name.c_str(), service_port.get());
+  mach_send_right_pool->AddSendRight(service_port);
 
-  mach_send_right_pool->AddSendRight(service_port.release());
+  printf("service %s %#x\n", service_name.c_str(), service_port.get());
 }
 
 // Prints information about all exception ports known for |exception_ports|. If
@@ -206,14 +213,14 @@ void ShowExceptionPorts(const ExceptionPorts& exception_ports,
                         MachSendRightPool* mach_send_right_pool) {
   const char* target_name = exception_ports.TargetTypeName();
 
-  std::vector<ExceptionPorts::ExceptionHandler> handlers;
+  ExceptionPorts::ExceptionHandlerVector handlers;
   if (!exception_ports.GetExceptionPorts(ExcMaskValid(), &handlers)) {
     return;
   }
 
   const char* age_name = is_new ? "new " : "";
 
-  if (handlers.size() == 0) {
+  if (handlers.empty()) {
     printf("no %s%s exception ports\n", age_name, target_name);
   }
 

util/mach/exception_ports.cc

@@ -19,6 +19,29 @@
 
 namespace crashpad {
 
+ExceptionPorts::ExceptionHandlerVector::ExceptionHandlerVector()
+    : vector_() {
+}
+
+ExceptionPorts::ExceptionHandlerVector::~ExceptionHandlerVector() {
+  Deallocate();
+}
+
+void ExceptionPorts::ExceptionHandlerVector::clear() {
+  Deallocate();
+  vector_.clear();
+}
+
+void ExceptionPorts::ExceptionHandlerVector::Deallocate() {
+  for (ExceptionHandler& exception_handler : vector_) {
+    if (exception_handler.port != MACH_PORT_NULL) {
+      kern_return_t kr =
+          mach_port_deallocate(mach_task_self(), exception_handler.port);
+      MACH_LOG_IF(ERROR, kr != KERN_SUCCESS, kr) << "mach_port_deallocate";
+    }
+  }
+}
+
 ExceptionPorts::ExceptionPorts(TargetType target_type, mach_port_t target_port)
     : target_port_(target_port), dealloc_target_port_(false) {
   switch (target_type) {
@@ -69,9 +92,8 @@ ExceptionPorts::~ExceptionPorts() {
   }
 }
 
-bool ExceptionPorts::GetExceptionPorts(
+bool ExceptionPorts::GetExceptionPorts(exception_mask_t mask,
-    exception_mask_t mask,
+                                       ExceptionHandlerVector* handlers) const {
-    std::vector<ExceptionHandler>* handlers) const {
   // <mach/mach_types.defs> says that these arrays have room for 32 elements,
   // despite EXC_TYPES_COUNT only being as low as 11 (in the 10.6 SDK), and
   // later operating system versions have defined more exception types. The

util/mach/exception_ports.h

@@ -85,6 +85,38 @@ class ExceptionPorts {
     thread_state_flavor_t flavor;
   };
 
+  //! \brief Wraps `std::vector<ExceptionHandler>`, providing proper cleanup of
+  //!     the send rights contained in each element’s ExceptionHandler::port.
+  //!
+  //! Upon destruction or clear(), an object of this class will deallocate all
+  //! send rights it contains. Otherwise, it is an interface-compatible drop-in
+  //! replacement for `std::vector<ExceptionHandler>`. Note that non-`const`
+  //! mutators are not provided to avoid accidental Mach right leaks.
+  class ExceptionHandlerVector {
+   public:
+    using VectorType = std::vector<ExceptionHandler>;
+
+    ExceptionHandlerVector();
+    ~ExceptionHandlerVector();
+
+    VectorType::const_iterator begin() const { return vector_.begin(); }
+    VectorType::const_iterator end() const { return vector_.end(); }
+    VectorType::size_type size() const { return vector_.size(); }
+    bool empty() const { return vector_.empty(); }
+    VectorType::const_reference operator[](VectorType::size_type index) const {
+      return vector_[index];
+    }
+    void push_back(VectorType::value_type& value) { vector_.push_back(value); }
+    void clear();
+
+   private:
+    void Deallocate();
+
+    VectorType vector_;
+
+    DISALLOW_COPY_AND_ASSIGN(ExceptionHandlerVector);
+  };
+
   //! \brief Constructs an interface object to get or set exception ports on a
   //!     host, task, or thread port.
   //!
@@ -111,19 +143,18 @@ class ExceptionPorts {
   //! \param[in] mask The exception mask, containing the `EXC_MASK_*` values to
   //!     be looked up and returned in \a handlers.
   //! \param[out] handlers The exception handlers registered for \a target_port
-  //!     to handle exceptions indicated in \a mask. The caller must take
+  //!     to handle exceptions indicated in \a mask. If no execption port is
-  //!     ownership of the \a port members of the returned ExceptionHandler
+  //!     registered for a bit in \a mask, \a handlers will not contain an entry
-  //!     objects. If no execption port is registered for a bit in \a mask, \a
+  //!     corresponding to that bit. This is a departure from the
-  //!     handlers will not contain an entry corresponding to that bit. This is
+  //!     `*_get_exception_ports()` functions, which may return a handler whose
-  //!     a departure from the `*_get_exception_ports()` functions, which may
+  //!     port is set to `EXCEPTION_PORT_NULL` in this case. On failure, this
-  //!     return a handler whose port is set to `EXCEPTION_PORT_NULL` in this
+  //!     argument is untouched.
-  //!     case. On failure, this argument is untouched.
   //!
   //! \return `true` if `*_get_exception_ports()` returned `KERN_SUCCESS`, with
   //!     \a handlers set appropriately. `false` otherwise, with an appropriate
   //!     message logged.
   bool GetExceptionPorts(exception_mask_t mask,
-                         std::vector<ExceptionHandler>* handlers) const;
+                         ExceptionHandlerVector* handlers) const;
 
   //! \brief Calls `*_set_exception_ports()` on the target.
   //!

util/mach/exception_ports_test.cc

@@ -64,13 +64,12 @@ void TestGetExceptionPorts(const ExceptionPorts& exception_ports,
                                             ? THREAD_STATE_NONE
                                             : MACHINE_THREAD_STATE;
 
-  std::vector<ExceptionPorts::ExceptionHandler> crash_handler;
+  ExceptionPorts::ExceptionHandlerVector crash_handler;
   ASSERT_TRUE(
       exception_ports.GetExceptionPorts(kExceptionMask, &crash_handler));
 
   if (expect_port != MACH_PORT_NULL) {
     ASSERT_EQ(1u, crash_handler.size());
-    base::mac::ScopedMachSendRight port_owner(crash_handler[0].port);
 
     EXPECT_EQ(kExceptionMask, crash_handler[0].mask);
     EXPECT_EQ(expect_port, crash_handler[0].port);
@@ -80,15 +79,13 @@ void TestGetExceptionPorts(const ExceptionPorts& exception_ports,
     EXPECT_TRUE(crash_handler.empty());
   }
 
-  std::vector<ExceptionPorts::ExceptionHandler> handlers;
+  ExceptionPorts::ExceptionHandlerVector handlers;
   ASSERT_TRUE(exception_ports.GetExceptionPorts(ExcMaskValid(), &handlers));
 
   EXPECT_GE(handlers.size(), crash_handler.size());
   bool found = false;
   for (const ExceptionPorts::ExceptionHandler& handler : handlers) {
     if ((handler.mask & kExceptionMask) != 0) {
-      base::mac::ScopedMachSendRight port_owner(handler.port);
-
       EXPECT_FALSE(found);
       found = true;
       EXPECT_EQ(expect_port, handler.port);
@@ -586,28 +583,27 @@ TEST(ExceptionPorts, HostExceptionPorts) {
   // host_set_exception_ports() is not tested, because if the test were running
   // as root and the call succeeded, it would have global effects.
 
+  const bool expect_success = geteuid() == 0;
+
   base::mac::ScopedMachSendRight host(mach_host_self());
   ExceptionPorts explicit_host_ports(ExceptionPorts::kTargetTypeHost, host);
   EXPECT_STREQ("host", explicit_host_ports.TargetTypeName());
 
-  std::vector<ExceptionPorts::ExceptionHandler> handlers;
+  ExceptionPorts::ExceptionHandlerVector explicit_handlers;
-  bool rv = explicit_host_ports.GetExceptionPorts(ExcMaskValid(), &handlers);
+  bool rv =
-  if (geteuid() == 0) {
+      explicit_host_ports.GetExceptionPorts(ExcMaskValid(), &explicit_handlers);
-    EXPECT_TRUE(rv);
+  EXPECT_EQ(expect_success, rv);
-  } else {
-    EXPECT_FALSE(rv);
-  }
 
   ExceptionPorts implicit_host_ports(ExceptionPorts::kTargetTypeHost,
                                      HOST_NULL);
   EXPECT_STREQ("host", implicit_host_ports.TargetTypeName());
 
-  rv = implicit_host_ports.GetExceptionPorts(ExcMaskValid(), &handlers);
+  ExceptionPorts::ExceptionHandlerVector implicit_handlers;
-  if (geteuid() == 0) {
+  rv =
-    EXPECT_TRUE(rv);
+      implicit_host_ports.GetExceptionPorts(ExcMaskValid(), &implicit_handlers);
-  } else {
+  EXPECT_EQ(expect_success, rv);
-    EXPECT_FALSE(rv);
+
-  }
+  EXPECT_EQ(explicit_handlers.size(), implicit_handlers.size());
 }
 
 }  // namespace