Refactor the bootstrap sandbox process launching integration.

sandbox/mac/bootstrap_sandbox.cc

Index: sandbox/mac/bootstrap_sandbox.cc
diff --git a/sandbox/mac/bootstrap_sandbox.cc b/sandbox/mac/bootstrap_sandbox.cc
index a90f570eb47487524acecdd11e903c8e60eeeb40..973d24a11d4436059e39fba248fe51e8261178ee 100644
--- a/sandbox/mac/bootstrap_sandbox.cc
+++ b/sandbox/mac/bootstrap_sandbox.cc
@@ -10,18 +10,34 @@
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/mach_logging.h"
+#include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
 #include "sandbox/mac/launchd_interception_server.h"
+#include "sandbox/mac/pre_exec_delegate.h"
 
 namespace sandbox {
 
-const int kNotAPolicy = -1;
+namespace {
+
+struct SandboxCheckInRequest {
+  mach_msg_header_t header;
+  mach_msg_body_t body;

[Mark Mentovai, 2015/09/17 19:45:15]

Since requests aren’t complex, you don’t need the body. Doesn’t hurt, but
doesn’t help either.

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+  uint64_t token;
+};
+
+struct SandboxCheckInReply {
+  mach_msg_header_t header;
+  mach_msg_body_t body;
+  mach_msg_port_descriptor_t bootstrap_port;
+};
+
+}  // namespace
 
 // 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()));
+  sandbox->launchd_server_.reset(new LaunchdInterceptionServer(sandbox.get()));
 
   // Check in with launchd to get the receive right for the server that is
   // published in the bootstrap namespace.
@@ -33,17 +49,66 @@ scoped_ptr<BootstrapSandbox> BootstrapSandbox::Create() {
         << "Failed to bootstrap_check_in the sandbox server.";
     return null.Pass();
   }
-  base::mac::ScopedMachReceiveRight scoped_port(port);
+  sandbox->check_in_port_.reset(port);
+
+  BootstrapSandbox* __block sandbox_ptr = sandbox.get();
+  sandbox->check_in_server_.reset(new base::DispatchSourceMach(
+      "org.chromium.sandbox.BootstrapClientManager",
+      sandbox->check_in_port_.get(),
+      ^{ sandbox_ptr->HandleChildCheckIn(); }));
+  sandbox->check_in_server_->Resume();
 
   // Start the sandbox server.
-  if (sandbox->server_->Initialize(scoped_port.get()))
-    ignore_result(scoped_port.release());  // Transferred to server_.
-  else
+  if (!sandbox->launchd_server_->Initialize(MACH_PORT_NULL))
     return null.Pass();
 
   return sandbox.Pass();
 }
 
+// static
+bool BootstrapSandbox::ClientCheckIn(mach_port_t sandbox_server_port,
+                                     uint64_t sandbox_token,
+                                     mach_port_t* new_bootstrap_port) {
+  // Create a reply port for the check in message.
+  mach_port_t reply_port;
+  kern_return_t kr = mach_port_allocate(mach_task_self(),
+                                        MACH_PORT_RIGHT_RECEIVE,
+                                        &reply_port);
+  if (kr != KERN_SUCCESS) {
+    MACH_LOG(ERROR, kr) << "mach_port_allocate";

[Mark Mentovai, 2015/09/17 19:45:15]

This function is called by the post-fork()-pre-exec() delegate. These should be
RAW_LOG.

[Robert Sesek, 2015/09/17 20:27:23]

Done. Left a comment as a warning to future editors.

+    return false;
+  }
+  base::mac::ScopedMachReceiveRight scoped_reply_port(reply_port);
+
+  // Check in with the sandbox server, presenting the |sandbox_token| in
+  // exchange for a new task bootstrap port.
+  struct {
+    union {
+      SandboxCheckInRequest request;
+      SandboxCheckInReply reply;

[Mark Mentovai, 2015/09/17 19:45:15]

You need to write this as union { request, struct { reply, trailer }}.

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+    };
+    mach_msg_audit_trailer_t trailer;

[Mark Mentovai, 2015/09/17 19:45:15]

Since you’re not actually requesting the audit trailer or any other trailer in
particular, just use mach_msg_trailer_t.

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+  } msg = {};
+  msg.request.header.msgh_bits = MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND,
+                                                MACH_MSG_TYPE_MAKE_SEND_ONCE) |
+                                 MACH_MSGH_BITS_COMPLEX;
+  msg.request.header.msgh_remote_port = sandbox_server_port;
+  msg.request.header.msgh_local_port = reply_port;
+  msg.request.header.msgh_size = sizeof(msg);
+  msg.request.token = sandbox_token;
+
+  kr = mach_msg(&msg.request.header, MACH_SEND_MSG | MACH_RCV_MSG,
+                sizeof(msg.request), sizeof(msg), reply_port,
+                100 /*ms*/, MACH_PORT_NULL);

[Mark Mentovai, 2015/09/17 19:45:15]

Why a timeout at all?

Anyway, you’re not even getting a timeout because you haven’t specified
MACH_SEND_TIMEOUT or MACH_RCV_TIMEOUT.

[Robert Sesek, 2015/09/17 20:27:23]

Agreed no timeout necessary, just forgot to clean up the arg.

+  if (kr == KERN_SUCCESS) {
+    *new_bootstrap_port = msg.reply.bootstrap_port.name;
+    return true;
+  } else {
+    MACH_LOG(ERROR, kr) << "mach_msg";
+    return false;
+  }
+}
+
 BootstrapSandbox::~BootstrapSandbox() {
 }
 
@@ -51,48 +116,29 @@ void BootstrapSandbox::RegisterSandboxPolicy(
     int sandbox_policy_id,
     const BootstrapSandboxPolicy& policy) {
   CHECK(IsPolicyValid(policy));
-  CHECK_GT(sandbox_policy_id, kNotAPolicy);
   base::AutoLock lock(lock_);
   DCHECK(policies_.find(sandbox_policy_id) == policies_.end());
   policies_.insert(std::make_pair(sandbox_policy_id, policy));
 }
 
-void BootstrapSandbox::PrepareToForkWithPolicy(int sandbox_policy_id) {
-  base::AutoLock lock(lock_);
-
-  // Verify that this is a real policy.
-  CHECK(policies_.find(sandbox_policy_id) != policies_.end());
-  CHECK_EQ(kNotAPolicy, effective_policy_id_)
-      << "Cannot nest calls to PrepareToForkWithPolicy()";
-
-  // Store the policy for the process we're about to create.
-  effective_policy_id_ = sandbox_policy_id;
-}
-
-// TODO(rsesek): The |lock_| needs to be taken twice because
-// base::LaunchProcess handles both fork+exec, and holding the lock for the
-// duration would block servicing of other bootstrap messages. If a better
-// LaunchProcess existed (do arbitrary work without layering violations), this
-// could be avoided.
-
-void BootstrapSandbox::FinishedFork(base::ProcessHandle handle) {
+scoped_ptr<PreExecDelegate> BootstrapSandbox::NewClient(int sandbox_policy_id) {
   base::AutoLock lock(lock_);
 
-  CHECK_NE(kNotAPolicy, effective_policy_id_)
-      << "Must PrepareToForkWithPolicy() before FinishedFork()";
+  DCHECK(policies_.find(sandbox_policy_id) != policies_.end());
 
-  // Apply the policy to the new process.
-  if (handle != base::kNullProcessHandle) {
-    const auto& existing_process = sandboxed_processes_.find(handle);
-    CHECK(existing_process == sandboxed_processes_.end());
-    sandboxed_processes_.insert(std::make_pair(handle, effective_policy_id_));
-    VLOG(3) << "Bootstrap sandbox enforced for pid " << handle;
+  uint64_t token;
+  while (true) {

[Mark Mentovai, 2015/09/17 19:45:15]

Good. I was hoping to see this!

+    token = base::RandUint64();
+    if (awaiting_processes_.find(token) == awaiting_processes_.end())
+      break;
   }
 
-  effective_policy_id_ = kNotAPolicy;
+  awaiting_processes_[token] = sandbox_policy_id;
+
+  return make_scoped_ptr(new PreExecDelegate(server_bootstrap_name_, token));
 }
 
-void BootstrapSandbox::ChildDied(base::ProcessHandle handle) {
+void BootstrapSandbox::InvalidateClient(base::ProcessHandle handle) {
   base::AutoLock lock(lock_);
   const auto& it = sandboxed_processes_.find(handle);
   if (it != sandboxed_processes_.end())
@@ -103,26 +149,18 @@ const BootstrapSandboxPolicy* BootstrapSandbox::PolicyForProcess(
     pid_t pid) const {
   base::AutoLock lock(lock_);
   const auto& process = sandboxed_processes_.find(pid);
-
-  // The new child could send bootstrap requests before the parent calls
-  // FinishedFork().
-  int policy_id = effective_policy_id_;
   if (process != sandboxed_processes_.end()) {
-    policy_id = process->second;
+    return &policies_.find(process->second)->second;
   }
 
-  if (policy_id == kNotAPolicy)
-    return NULL;
-
-  return &policies_.find(policy_id)->second;
+  return nullptr;
 }
 
 BootstrapSandbox::BootstrapSandbox()
     : server_bootstrap_name_(
           base::StringPrintf("%s.sandbox.%d", base::mac::BaseBundleID(),
               getpid())),
-      real_bootstrap_port_(MACH_PORT_NULL),
-      effective_policy_id_(kNotAPolicy) {
+      real_bootstrap_port_(MACH_PORT_NULL) {
   mach_port_t port = MACH_PORT_NULL;
   kern_return_t kr = task_get_special_port(
       mach_task_self(), TASK_BOOTSTRAP_PORT, &port);
@@ -130,4 +168,65 @@ BootstrapSandbox::BootstrapSandbox()
   real_bootstrap_port_.reset(port);
 }
 
+void BootstrapSandbox::HandleChildCheckIn() {
+  struct {
+    SandboxCheckInRequest request;
+    mach_msg_audit_trailer_t trailer;
+  } msg = {};
+  msg.request.header.msgh_local_port = check_in_port_.get();
+  msg.request.header.msgh_size = sizeof(msg.request);
+  const mach_msg_option_t kOptions = MACH_RCV_MSG |
+      MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |

[Mark Mentovai, 2015/09/17 19:45:15]

MACH_MSG_TRAILER_FORMAT_0

[Robert Sesek, 2015/09/17 20:27:24]

Done.

+      MACH_RCV_TRAILER_ELEMENTS(MACH_RCV_TRAILER_AUDIT);
+  kern_return_t kr = mach_msg(&msg.request.header, kOptions, 0,
+                              sizeof(msg), check_in_port_.get(),
+                              MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
+  if (kr != KERN_SUCCESS) {
+    MACH_LOG(ERROR, kr) << "HandleChildCheckIn mach_msg MACH_RCV_MSG";
+    return;
+  }
+
+  pid_t client_pid;
+  audit_token_to_au32(msg.trailer.msgh_audit,
+      NULL, NULL, NULL, NULL, NULL, &client_pid, NULL, NULL);

[Mark Mentovai, 2015/09/17 19:45:15]

nullptr

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+
+  {
+    base::AutoLock lock(lock_);
+
+    auto awaiting_it = awaiting_processes_.find(msg.request.token);
+    if (awaiting_it == awaiting_processes_.end()) {
+      LOG(ERROR) << "Received sandbox check-in message from unknown client.";
+      mach_msg_destroy(&msg.request);
+      return;
+    }
+
+    sandboxed_processes_[client_pid] = awaiting_it->second;
+    awaiting_processes_.erase(awaiting_it);
+  }
+
+  SandboxCheckInReply reply = {};
+  reply.header.msgh_bits = MACH_MSGH_BITS_REMOTE(msg.request.header.msgh_bits) |
+                           MACH_MSGH_BITS_COMPLEX;
+  reply.header.msgh_remote_port = msg.request.header.msgh_remote_port;
+  reply.header.msgh_size = sizeof(reply);
+  reply.body.msgh_descriptor_count = 1;
+  reply.bootstrap_port.name = launchd_server_->server_port();
+  reply.bootstrap_port.disposition = MACH_MSG_TYPE_MAKE_SEND;
+  reply.bootstrap_port.type = MACH_MSG_PORT_DESCRIPTOR;
+
+  kr = mach_msg(&reply.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT,
+                sizeof(reply), 0, MACH_PORT_NULL, 100 /*ms*/, MACH_PORT_NULL);

[Mark Mentovai, 2015/09/17 19:45:15]

I do see why you might want a timeout on this one.

+  if (kr != KERN_SUCCESS) {
+    {
+      base::AutoLock lock(lock_);
+      sandboxed_processes_.erase(client_pid);
+    }
+    MACH_LOG(ERROR, kr) << "HandleChildCheckIn mach_msg MACH_SEND_MSG";
+    mach_msg_destroy(&msg.request);
+    // msg.reply does not need to be destroyed as the right is created
+    // at mach_msg() send.

[Mark Mentovai, 2015/09/17 19:45:15]

Say that the destroy is just to kill the send-once right for the reply. Also,
consider using a scoper to call mach_msg_destroy(). This function already has
one early return above.

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+    return;

[Mark Mentovai, 2015/09/17 19:45:15]

This is sort of an early return but not really.

[Robert Sesek, 2015/09/17 20:27:23]

Done.

+  }
+}
+
 }  // namespace sandbox