Rewrite ScopedMachPort in terms of ScopedGeneric, making its usage more explicit and clear.

ScopedMachPort was only properly handling send rights, accidentally leaking
receive rights in some places. This adds an additional scoper for handling
receive rights.

BUG=none
R=mark@chromium.org

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

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

5 files changed, 31 insertions, 26 deletions

diff --git a/sandbox/mac/bootstrap_sandbox.cc b/sandbox/mac/bootstrap_sandbox.cc
index 8692c7e..b90d8d1 100644
--- a/sandbox/mac/bootstrap_sandbox.cc
+++ b/sandbox/mac/bootstrap_sandbox.cc
@@ -15,26 +15,36 @@ const int kNotAPolicy = -1;
 
 // static
 scoped_ptr<BootstrapSandbox> BootstrapSandbox::Create() {
+  scoped_ptr<BootstrapSandbox> null;  // Used for early returns.
   scoped_ptr<BootstrapSandbox> sandbox(new BootstrapSandbox());
   sandbox->server_.reset(new LaunchdInterceptionServer(sandbox.get()));
 
-  if (!sandbox->server_->Initialize()) {
-    sandbox.reset();
-  } else {
-    // Note that the extern global bootstrap_port (in bootstrap.h) will not
-    // be changed here. The parent only has its bootstrap port replaced
-    // permanently because changing it repeatedly in a multi-threaded program
-    // could lead to unsafe access patterns. In a single-threaded program,
-    // the port would be restored after fork(). See the design document for
-    // a larger discussion.
-    //
-    // By not changing the global bootstrap_port, users of the bootstrap port
-    // in the parent can potentially skip an unnecessary indirection through
-    // the sandbox server.
-    kern_return_t kr = task_set_special_port(mach_task_self(),
-        TASK_BOOTSTRAP_PORT, sandbox->server_->server_port());
-    if (kr != KERN_SUCCESS)
-      sandbox.reset();
+  if (!sandbox->server_->Initialize())
+    return null.Pass();
+
+  mach_port_t port = sandbox->server_->server_port();
+  kern_return_t kr = mach_port_insert_right(mach_task_self(), port, port,
+      MACH_MSG_TYPE_MAKE_SEND);
+  if (kr != KERN_SUCCESS) {
+    MACH_LOG(ERROR, kr) << "Failed to insert send right on bootstrap port.";
+    return null.Pass();
+  }
+  base::mac::ScopedMachSendRight scoped_right(port);
+
+  // Note that the extern global bootstrap_port (in bootstrap.h) will not
+  // be changed here. The parent only has its bootstrap port replaced
+  // permanently because changing it repeatedly in a multi-threaded program
+  // could lead to unsafe access patterns. In a single-threaded program,
+  // the port would be restored after fork(). See the design document for
+  // a larger discussion.
+  //
+  // By not changing the global bootstrap_port, users of the bootstrap port
+  // in the parent can potentially skip an unnecessary indirection through
+  // the sandbox server.
+  kr = task_set_special_port(mach_task_self(), TASK_BOOTSTRAP_PORT, port);
+  if (kr != KERN_SUCCESS) {
+    MACH_LOG(ERROR, kr) << "Failed to set new bootstrap port.";
+    return null.Pass();
   }
 
   return sandbox.Pass();
diff --git a/sandbox/mac/bootstrap_sandbox.h b/sandbox/mac/bootstrap_sandbox.h
index 335c481..53fc54f 100644
--- a/sandbox/mac/bootstrap_sandbox.h
+++ b/sandbox/mac/bootstrap_sandbox.h
@@ -88,7 +88,7 @@ class SANDBOX_EXPORT BootstrapSandbox {
 
   // The original bootstrap port of the process, which is connected to the
   // real launchd server.
-  base::mac::ScopedMachPort real_bootstrap_port_;
+  base::mac::ScopedMachSendRight real_bootstrap_port_;
 
   // The |lock_| protects all the following variables.
   mutable base::Lock lock_;
diff --git a/sandbox/mac/bootstrap_sandbox_unittest.mm b/sandbox/mac/bootstrap_sandbox_unittest.mm
index dbea76e..0b1e279 100644
--- a/sandbox/mac/bootstrap_sandbox_unittest.mm
+++ b/sandbox/mac/bootstrap_sandbox_unittest.mm
@@ -228,7 +228,7 @@ TEST_F(BootstrapSandboxTest, PolicySubstitutePort) {
   mach_port_t port;
   ASSERT_EQ(KERN_SUCCESS, mach_port_allocate(mach_task_self(),
       MACH_PORT_RIGHT_RECEIVE, &port));
-  base::mac::ScopedMachPort scoped_port(port);
+  base::mac::ScopedMachReceiveRight scoped_port(port);
 
   BootstrapSandboxPolicy policy(BaselinePolicy());
   policy[kTestServer] = Rule(port);
diff --git a/sandbox/mac/launchd_interception_server.cc b/sandbox/mac/launchd_interception_server.cc
index 6c20181..d1495dfe 100644
--- a/sandbox/mac/launchd_interception_server.cc
+++ b/sandbox/mac/launchd_interception_server.cc
@@ -47,11 +47,6 @@ bool LaunchdInterceptionServer::Initialize() {
     MACH_LOG(ERROR, kr) << "Failed to allocate new bootstrap port.";
     return false;
   }
-  if ((kr = mach_port_insert_right(task, port, port, MACH_MSG_TYPE_MAKE_SEND))
-          != KERN_SUCCESS) {
-    MACH_LOG(ERROR, kr) << "Failed to insert send right on bootstrap port.";
-    return false;
-  }
   server_port_.reset(port);
 
   // Allocate the message request and reply buffers.
diff --git a/sandbox/mac/launchd_interception_server.h b/sandbox/mac/launchd_interception_server.h
index c7e2a74..8125d00 100644
--- a/sandbox/mac/launchd_interception_server.h
+++ b/sandbox/mac/launchd_interception_server.h
@@ -70,7 +70,7 @@ class LaunchdInterceptionServer {
   const BootstrapSandbox* sandbox_;
 
   // The Mach port on which the server is receiving requests.
-  base::mac::ScopedMachPort server_port_;
+  base::mac::ScopedMachReceiveRight server_port_;
 
   // The dispatch queue used to service the server_source_.
   dispatch_queue_t server_queue_;
@@ -84,7 +84,7 @@ class LaunchdInterceptionServer {
 
   // 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::ScopedMachPort sandbox_port_;
+  base::mac::ScopedMachReceiveRight sandbox_port_;
 
   // The compatibility shim that handles differences in message header IDs and
   // request/reply structures between different OS X versions.