From 5f7eda87a6fa32b8881464950dacb006180a4edb Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 6 Oct 2015 16:14:29 -0400 Subject: [PATCH] =?UTF-8?q?mac:=20Don=E2=80=99t=20leak=20send=20rights=20f?= =?UTF-8?q?rom=20ExceptionPorts::GetExceptionPorts()?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExceptionPorts::GetExceptionPorts() returned a std::vector, 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 | 4 +-- tools/mac/exception_port_tool.cc | 19 +++++++++---- util/mach/exception_ports.cc | 28 ++++++++++++++++-- util/mach/exception_ports.h | 47 +++++++++++++++++++++++++------ util/mach/exception_ports_test.cc | 32 +++++++++------------ 5 files changed, 92 insertions(+), 38 deletions(-) diff --git a/client/simulate_crash_mac.cc b/client/simulate_crash_mac.cc index 1884262f..cda6b76d 100644 --- a/client/simulate_crash_mac.cc +++ b/client/simulate_crash_mac.cc @@ -16,8 +16,6 @@ #include -#include - #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 handlers; + ExceptionPorts::ExceptionHandlerVector handlers; ExceptionPorts exception_ports(kTargetTypes[target_type_index], MACH_PORT_NULL); if (exception_ports.GetExceptionPorts(EXC_MASK_CRASH, &handlers)) { diff --git a/tools/mac/exception_port_tool.cc b/tools/mac/exception_port_tool.cc index 818ad446..e26c1bb0 100644 --- a/tools/mac/exception_port_tool.cc +++ b/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 - //! is destroyed. + //! its own reference to the send right, which remains valid until the + //! 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 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); } diff --git a/util/mach/exception_ports.cc b/util/mach/exception_ports.cc index 2d85c1e6..ef535b4c 100644 --- a/util/mach/exception_ports.cc +++ b/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( - exception_mask_t mask, - std::vector* handlers) const { +bool ExceptionPorts::GetExceptionPorts(exception_mask_t mask, + ExceptionHandlerVector* handlers) const { // 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 diff --git a/util/mach/exception_ports.h b/util/mach/exception_ports.h index d82ac2dc..7f23c5e5 100644 --- a/util/mach/exception_ports.h +++ b/util/mach/exception_ports.h @@ -85,6 +85,38 @@ class ExceptionPorts { thread_state_flavor_t flavor; }; + //! \brief Wraps `std::vector`, 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`. Note that non-`const` + //! mutators are not provided to avoid accidental Mach right leaks. + class ExceptionHandlerVector { + public: + using VectorType = std::vector; + + 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 - //! ownership of the \a port members of the returned ExceptionHandler - //! objects. If no execption port is registered for a bit in \a mask, \a - //! handlers will not contain an entry corresponding to that bit. This is - //! a departure from the `*_get_exception_ports()` functions, which may - //! return a handler whose port is set to `EXCEPTION_PORT_NULL` in this - //! case. On failure, this argument is untouched. + //! to handle exceptions indicated in \a mask. If no execption port is + //! registered for a bit in \a mask, \a handlers will not contain an entry + //! corresponding to that bit. This is a departure from the + //! `*_get_exception_ports()` functions, which may return a handler whose + //! port is set to `EXCEPTION_PORT_NULL` in this 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* handlers) const; + ExceptionHandlerVector* handlers) const; //! \brief Calls `*_set_exception_ports()` on the target. //! diff --git a/util/mach/exception_ports_test.cc b/util/mach/exception_ports_test.cc index 478dfe21..7384c022 100644 --- a/util/mach/exception_ports_test.cc +++ b/util/mach/exception_ports_test.cc @@ -64,13 +64,12 @@ void TestGetExceptionPorts(const ExceptionPorts& exception_ports, ? THREAD_STATE_NONE : MACHINE_THREAD_STATE; - std::vector 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 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 handlers; - bool rv = explicit_host_ports.GetExceptionPorts(ExcMaskValid(), &handlers); - if (geteuid() == 0) { - EXPECT_TRUE(rv); - } else { - EXPECT_FALSE(rv); - } + ExceptionPorts::ExceptionHandlerVector explicit_handlers; + bool rv = + explicit_host_ports.GetExceptionPorts(ExcMaskValid(), &explicit_handlers); + EXPECT_EQ(expect_success, rv); ExceptionPorts implicit_host_ports(ExceptionPorts::kTargetTypeHost, HOST_NULL); EXPECT_STREQ("host", implicit_host_ports.TargetTypeName()); - rv = implicit_host_ports.GetExceptionPorts(ExcMaskValid(), &handlers); - if (geteuid() == 0) { - EXPECT_TRUE(rv); - } else { - EXPECT_FALSE(rv); - } + ExceptionPorts::ExceptionHandlerVector implicit_handlers; + rv = + implicit_host_ports.GetExceptionPorts(ExcMaskValid(), &implicit_handlers); + EXPECT_EQ(expect_success, rv); + + EXPECT_EQ(explicit_handlers.size(), implicit_handlers.size()); } } // namespace