Use the new ScopedMachVM class and the MACH_LOG family of logging macros

content/browser/mach_broker_mac.mm

Index: content/browser/mach_broker_mac.mm
diff --git a/content/browser/mach_broker_mac.mm b/content/browser/mach_broker_mac.mm
index 24a0e2498134e941fab023ab22c307b156454beb..9261d3c749c33c4b609d7e9a36065aa11db44059 100644
--- a/content/browser/mach_broker_mac.mm
+++ b/content/browser/mach_broker_mac.mm
@@ -12,6 +12,7 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
+#include "base/mac/mach_logging.h"
 #include "base/mac/scoped_mach_port.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -28,11 +29,6 @@ namespace content {
 
 namespace {
 
-// Prints a string representation of a Mach error code.
-std::string MachErrorCode(kern_return_t err) {
-  return base::StringPrintf("0x%x %s", err, mach_error_string(err));
-}
-
 // Mach message structure used in the child as a sending message.
 struct MachBroker_ChildSendMsg {
   mach_msg_header_t header;
@@ -64,8 +60,7 @@ class MachListenerThreadDelegate : public base::PlatformThread::Delegate {
                                           MACH_PORT_RIGHT_RECEIVE,
                                           &port);
     if (kr != KERN_SUCCESS) {
-      LOG(ERROR) << "Failed to allocate MachBroker server port: "
-                 << MachErrorCode(kr);
+      MACH_LOG(ERROR, kr) << "mach_port_allocate";
       return false;
     }
 
@@ -73,8 +68,7 @@ class MachListenerThreadDelegate : public base::PlatformThread::Delegate {
     kr = mach_port_insert_right(
         mach_task_self(), port, port, MACH_MSG_TYPE_MAKE_SEND);
     if (kr != KERN_SUCCESS) {
-      LOG(ERROR) << "Failed to insert send right for MachBroker server port: "
-                 << MachErrorCode(kr);
+      MACH_LOG(ERROR, kr) << "mach_port_insert_right";
       return false;
     }
 
@@ -96,33 +90,31 @@ class MachListenerThreadDelegate : public base::PlatformThread::Delegate {
     msg.header.msgh_size = sizeof(msg);
     msg.header.msgh_local_port = server_port_.get();
 
+    const mach_msg_option_t options = MACH_RCV_MSG |
+        MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |
+        MACH_RCV_TRAILER_ELEMENTS(MACH_RCV_TRAILER_AUDIT);
+
     kern_return_t kr;
-    do {
+    while ((kr = mach_msg(&msg.header, options, 0, sizeof(msg), server_port_,

[Mark Mentovai, 2014/05/09 19:06:20]

It occurs to me that this sucks. A compromised child can send an
intentionally-large message to this port to force this mach_msg to return
MACH_RCV_TOO_LARGE, which will terminate this loop and thread.

Do you know how the libdispatch-based Mach message receiver deals with this kind
of thing?

[Robert Sesek, 2014/05/09 20:40:01]

Switching this to libdispatch would be better. Dispatch just has an
EVFILT_MACHPORT and calls out to the event handler block. If the message failed
to be received, then returning out of the block would be fine and the server
would keep servicing its port.

I have an item on my todo list to switch this from a thread to a queue. Then it
can actually be joinable (if we want).

+                          MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL)) ==
+           KERN_SUCCESS) {

[Robert Sesek, 2014/05/09 20:40:01]

nit? should this be indented +4

[Mark Mentovai, 2014/05/09 21:09:35]

rsesek wrote:
I don’t think so, but I struggled with this too. I made it tall instead of wide,
how ’bout that?

       // Use the kernel audit information to make sure this message is from
       // a task that this process spawned. The kernel audit token contains the
       // unspoofable pid of the task that sent the message.
-      mach_msg_option_t options = MACH_RCV_MSG |
-          MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |
-          MACH_RCV_TRAILER_ELEMENTS(MACH_RCV_TRAILER_AUDIT);
-
-      kr = mach_msg(&msg.header, options, 0, sizeof(msg), server_port_,
-          MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
-      if (kr == KERN_SUCCESS) {
-        // TODO(rsesek): In the 10.7 SDK, there's audit_token_to_pid().
-        pid_t child_pid;
-        audit_token_to_au32(msg.trailer.msgh_audit,
-            NULL, NULL, NULL, NULL, NULL, &child_pid, NULL, NULL);
-
-        mach_port_t child_task_port = msg.child_task_port.name;
-
-        // Take the lock and update the broker information.
-        base::AutoLock lock(broker_->GetLock());
-        broker_->FinalizePid(child_pid, child_task_port);
-      }
-    } while (kr == KERN_SUCCESS);
-
-    LOG(ERROR) << "MachBroker thread exiting; mach_msg() likely failed: "
-               << MachErrorCode(kr);
+      //
+      // TODO(rsesek): In the 10.7 SDK, there's audit_token_to_pid().
+      pid_t child_pid;
+      audit_token_to_au32(msg.trailer.msgh_audit,
+          NULL, NULL, NULL, NULL, NULL, &child_pid, NULL, NULL);
+
+      mach_port_t child_task_port = msg.child_task_port.name;
+
+      // Take the lock and update the broker information.
+      base::AutoLock lock(broker_->GetLock());
+      broker_->FinalizePid(child_pid, child_task_port);
+    }
+
+    MACH_LOG(ERROR, kr) << "mach_msg";
   }
 
  private:
@@ -141,7 +133,7 @@ bool MachBroker::ChildSendTaskPortToParent() {
   mach_port_t bootstrap_port;
   kern_return_t kr = task_get_bootstrap_port(mach_task_self(), &bootstrap_port);
   if (kr != KERN_SUCCESS) {
-    LOG(ERROR) << "Failed to look up bootstrap port: " << MachErrorCode(kr);
+    MACH_LOG(ERROR, kr) << "task_get_bootstrap_port";
     return false;
   }
 
@@ -149,7 +141,7 @@ bool MachBroker::ChildSendTaskPortToParent() {
   kr = bootstrap_look_up(bootstrap_port,
       const_cast<char*>(GetMachPortName().c_str()), &parent_port);
   if (kr != KERN_SUCCESS) {
-    LOG(ERROR) << "Failed to look up named parent port: " << MachErrorCode(kr);
+    BOOTSTRAP_LOG(ERROR, kr) << "bootstrap_look_up";
     return false;
   }
 
@@ -169,7 +161,7 @@ bool MachBroker::ChildSendTaskPortToParent() {
   kr = mach_msg(&msg.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT, sizeof(msg),
       0, MACH_PORT_NULL, 100 /*milliseconds*/, MACH_PORT_NULL);
   if (kr != KERN_SUCCESS) {
-    LOG(ERROR) << "Failed to send task port to parent: " << MachErrorCode(kr);
+    MACH_LOG(ERROR, kr) << "mach_msg";
     return false;
   }
 
@@ -279,9 +271,7 @@ void MachBroker::InvalidatePid(base::ProcessHandle pid) {
 
   kern_return_t kr = mach_port_deallocate(mach_task_self(),
                                           it->second);
-  LOG_IF(WARNING, kr != KERN_SUCCESS)
-     << "Failed to mach_port_deallocate mach task " << it->second
-     << ", error " << MachErrorCode(kr);
+  MACH_LOG_IF(WARNING, kr != KERN_SUCCESS, kr) << "mach_port_deallocate";
   mach_map_.erase(it);
 }