Do not destroy Mach messages that are forwarded out of the the bootstrap sandbox.

For the process hosting the sandbox, doing so could result in over-unrefing send
rights. It's not necessary to destroy forwarded messages because any rights
copied into the process will be copied out on send, using move semantics.

BUG=367863
R=mark@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273545 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 65 insertions, 4 deletions

diff --git a/sandbox/mac/bootstrap_sandbox_unittest.mm b/sandbox/mac/bootstrap_sandbox_unittest.mm
index 0b1e279..fb389237 100644
--- a/sandbox/mac/bootstrap_sandbox_unittest.mm
+++ b/sandbox/mac/bootstrap_sandbox_unittest.mm
@@ -266,4 +266,52 @@ MULTIPROCESS_TEST_MAIN(PolicySubstitutePort) {
   return 0;
 }
 
+TEST_F(BootstrapSandboxTest, ForwardMessageInProcess) {
+  mach_port_t task = mach_task_self();
+
+  mach_port_t port;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_allocate(task, MACH_PORT_RIGHT_RECEIVE,
+      &port));
+  base::mac::ScopedMachReceiveRight scoped_port_recv(port);
+
+  mach_port_urefs_t send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(0u, send_rights);
+
+  ASSERT_EQ(KERN_SUCCESS, mach_port_insert_right(task, port, port,
+      MACH_MSG_TYPE_MAKE_SEND));
+  base::mac::ScopedMachSendRight scoped_port_send(port);
+
+  send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(1u, send_rights);
+
+  mach_port_t bp;
+  ASSERT_EQ(KERN_SUCCESS, task_get_bootstrap_port(task, &bp));
+  base::mac::ScopedMachSendRight scoped_bp(bp);
+
+  char service_name[] = "org.chromium.sandbox.test.ForwardMessageInProcess";
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+  kern_return_t kr = bootstrap_register(bp, service_name, port);
+#pragma GCC diagnostic pop
+  EXPECT_EQ(KERN_SUCCESS, kr);
+
+  send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(1u, send_rights);
+
+  mach_port_t service_port;
+  EXPECT_EQ(KERN_SUCCESS, bootstrap_look_up(bp, service_name, &service_port));
+  base::mac::ScopedMachSendRight scoped_service_port(service_port);
+
+  send_rights = 0;
+  ASSERT_EQ(KERN_SUCCESS, mach_port_get_refs(task, port, MACH_PORT_RIGHT_SEND,
+      &send_rights));
+  EXPECT_EQ(2u, send_rights);
+}
+
 }  // namespace sandbox
diff --git a/sandbox/mac/launchd_interception_server.cc b/sandbox/mac/launchd_interception_server.cc
index d1495dfe..919f207 100644
--- a/sandbox/mac/launchd_interception_server.cc
+++ b/sandbox/mac/launchd_interception_server.cc
@@ -25,6 +25,7 @@ LaunchdInterceptionServer::LaunchdInterceptionServer(
       server_port_(MACH_PORT_NULL),
       server_queue_(NULL),
       server_source_(NULL),
+      did_forward_message_(false),
       sandbox_port_(MACH_PORT_NULL),
       compat_shim_(GetLaunchdCompatibilityShim()) {
 }
@@ -102,6 +103,7 @@ void LaunchdInterceptionServer::ReceiveMessage() {
   // Zero out the buffers from handling any previous message.
   bzero(request, kBufferSize);
   bzero(reply, kBufferSize);
+  did_forward_message_ = false;
 
   // A Mach message server-once. The system library to run a message server
   // cannot be used here, because some requests are conditionally forwarded
@@ -125,9 +127,15 @@ void LaunchdInterceptionServer::ReceiveMessage() {
   // Process the message.
   DemuxMessage(request, reply);
 
-  // Free any descriptors in the message body.
-  mach_msg_destroy(request);
-  mach_msg_destroy(reply);
+  // Free any descriptors in the message body. If the message was forwarded,
+  // any descriptors would have been moved out of the process on send. If the
+  // forwarded message was sent from the process hosting this sandbox server,
+  // destroying the message could also destroy rights held outside the scope of
+  // this message server.
+  if (!did_forward_message_) {
+    mach_msg_destroy(request);
+    mach_msg_destroy(reply);
+  }
 }
 
 void LaunchdInterceptionServer::DemuxMessage(mach_msg_header_t* request,
@@ -254,7 +262,9 @@ void LaunchdInterceptionServer::ForwardMessage(mach_msg_header_t* request,
   request->msgh_bits = (request->msgh_bits & ~MACH_MSGH_BITS_PORTS_MASK) |
       MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND, MACH_MSG_TYPE_MOVE_SEND_ONCE);
   kern_return_t kr = mach_msg_send(request);
-  if (kr != KERN_SUCCESS) {
+  if (kr == KERN_SUCCESS) {
+    did_forward_message_ = true;
+  } else {
     MACH_LOG(ERROR, kr) << "Unable to forward message to the real launchd.";
   }
 }
diff --git a/sandbox/mac/launchd_interception_server.h b/sandbox/mac/launchd_interception_server.h
index 8125d00..6b92e9f 100644
--- a/sandbox/mac/launchd_interception_server.h
+++ b/sandbox/mac/launchd_interception_server.h
@@ -82,6 +82,9 @@ class LaunchdInterceptionServer {
   base::mac::ScopedMachVM request_buffer_;
   base::mac::ScopedMachVM reply_buffer_;
 
+  // Whether or not ForwardMessage() was called during ReceiveMessage().
+  bool did_forward_message_;
+
   // The Mach port handed out in reply to denied look up requests. All denied
   // requests share the same port, though nothing reads messages from it.
   base::mac::ScopedMachReceiveRight sandbox_port_;