Refactor the bootstrap sandbox process launching integration.

Refactor the bootstrap sandbox process launching integration.

There are three changes:
  - A LaunchOptions::PreExecDelegate is now used to perform the bootstrap port
    replacement in the new child. This removes sandbox-specific knowledge from
    //base.
  - The replacement bootstrap port is no longer registered with launchd.
    Instead, a new sandbox manager port is registered. Clients communicate with
    this server to get the replacement bootstrap port.
  - Using the above port, clients now perform a post-fork-pre-exec handshake
    to check in with the sandbox server. This removes the complicated
    PrepareToFork/FinishedFork interface.

BUG=367863, 388214
R=mark@chromium.org

Committed: https://crrev.com/408d2ee53267204220f2062a97bd7fe7b9a69354
Cr-Commit-Position: refs/heads/master@{#349571}

[Mark Mentovai, 2015-09-17 19:45:16]

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:24: mach_msg_body_t body;
Since requests aren’t complex, you don’t need the body. Doesn’t hurt, but
doesn’t help either.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:78: MACH_LOG(ERROR, kr) <<
"mach_port_allocate";
This function is called by the post-fork()-pre-exec() delegate. These should be
RAW_LOG.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:88: SandboxCheckInReply reply;
You need to write this as union { request, struct { reply, trailer }}.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:90: mach_msg_audit_trailer_t trailer;
Since you’re not actually requesting the audit trailer or any other trailer in
particular, just use mach_msg_trailer_t.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:102: 100 /*ms*/, MACH_PORT_NULL);
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.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:130: while (true) {
Good. I was hoping to see this!

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:179:
MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |
MACH_MSG_TRAILER_FORMAT_0

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:191: NULL, NULL, NULL, NULL, NULL, &client_pid,
NULL, NULL);
nullptr

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:218: sizeof(reply), 0, MACH_PORT_NULL, 100
/*ms*/, MACH_PORT_NULL);
I do see why you might want a timeout on this one.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:227: // at mach_msg() send.
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.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:228: return;
This is sort of an early return but not really.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
File sandbox/mac/bootstrap_sandbox.h (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.h:44: // clean up any mappings in this class.
Or you could make a new receive right for each client and monitor it for a
no-senders notification. The receive rights would all go into a port set.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/pre_exec_delega...
File sandbox/mac/pre_exec_delegate.cc (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/pre_exec_delega...
sandbox/mac/pre_exec_delegate.cc:50: kr = mach_ports_register(mach_task_self(),
&new_bootstrap_port, 1);
Any reason to do this pre-10.10? It gives the child an unexpected extra send
right reference. If the child legitimately wanted to set its bootstrap port to
something even more restrictive, it’d unknowingly hang on to a reference to this
one.

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

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:24: mach_msg_body_t body;
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> Since requests aren’t complex, you don’t need the body. Doesn’t hurt, but
> doesn’t help either.

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:78: MACH_LOG(ERROR, kr) <<
"mach_port_allocate";
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> This function is called by the post-fork()-pre-exec() delegate. These should
be
> RAW_LOG.

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

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:88: SandboxCheckInReply reply;
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> You need to write this as union { request, struct { reply, trailer }}.

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:90: mach_msg_audit_trailer_t trailer;
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> Since you’re not actually requesting the audit trailer or any other trailer in
> particular, just use mach_msg_trailer_t.

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:102: 100 /*ms*/, MACH_PORT_NULL);
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> 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.

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

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:179:
MACH_RCV_TRAILER_TYPE(MACH_RCV_TRAILER_AUDIT) |
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> MACH_MSG_TRAILER_FORMAT_0

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:191: NULL, NULL, NULL, NULL, NULL, &client_pid,
NULL, NULL);
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> nullptr

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:227: // at mach_msg() send.
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> 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.

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.cc:228: return;
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> This is sort of an early return but not really.

Done.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
File sandbox/mac/bootstrap_sandbox.h (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/bootstrap_sandb...
sandbox/mac/bootstrap_sandbox.h:44: // clean up any mappings in this class.
On 2015/09/17 19:45:15, Mark Mentovai - August is over wrote:
> Or you could make a new receive right for each client and monitor it for a
> no-senders notification. The receive rights would all go into a port set.

That's what I want(ed) to do, but because a task's ports get reset on exec()
there'd have to be a two-phase check in. I'm still considering it, though.

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/pre_exec_delega...
File sandbox/mac/pre_exec_delegate.cc (right):

https://codereview.chromium.org/1346923006/diff/1/sandbox/mac/pre_exec_delega...
sandbox/mac/pre_exec_delegate.cc:50: kr = mach_ports_register(mach_task_self(),
&new_bootstrap_port, 1);
On 2015/09/17 19:45:16, Mark Mentovai - August is over wrote:
> Any reason to do this pre-10.10? It gives the child an unexpected extra send
> right reference. If the child legitimately wanted to set its bootstrap port to
> something even more restrictive, it’d unknowingly hang on to a reference to
this
> one.

Done.

[Mark Mentovai, 2015-09-17 21:39:57]

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:81: if
(!sandbox->launchd_server_->Initialize(MACH_PORT_NULL))
Is Initialize ever called with an argument other than MACH_PORT_NULL now? You
may be able to get rid of the argument.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:87: // static
I think that the static comment should come right before the bool.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
If you never get a check-in, this entry will persist in the map forever.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
If you process the check-in late, after the child has died a relatively early
death and someone else has already called InvalidateClient() with no effect,
this entry will persist in the map forever.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:224: sandboxed_processes_[client_pid] =
awaiting_it->second;
Verify that client_pid isn’t already in this map.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.h (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.h:52: // For use in newly created child processes.
Checks in with the bootstrap
newly-created

[Robert Sesek, 2015-09-17 22:04:44]

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:81: if
(!sandbox->launchd_server_->Initialize(MACH_PORT_NULL))
On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> Is Initialize ever called with an argument other than MACH_PORT_NULL now? You
> may be able to get rid of the argument.

Acknowledged.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:87: // static
On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> I think that the static comment should come right before the bool.

Done.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> If you process the check-in late, after the child has died a relatively early
> death and someone else has already called InvalidateClient() with no effect,
> this entry will persist in the map forever.

Right, but AFAICT there's nothing that can be done about this.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> If you never get a check-in, this entry will persist in the map forever.

Yup. Do you have a suggestion?

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:224: sandboxed_processes_[client_pid] =
awaiting_it->second;
On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> Verify that client_pid isn’t already in this map.

Done. That seems CHECK-worthy.

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.h (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.h:52: // For use in newly created child processes.
Checks in with the bootstrap
On 2015/09/17 21:39:57, Mark Mentovai - August is over wrote:
> newly-created

Newly is an adverb and so shouldn't be hyphenated with the verb it modifies.

[Mark Mentovai, 2015-09-17 22:10:53]

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
Robert Sesek wrote:
> On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> > If you never get a check-in, this entry will persist in the map forever.
> 
> Yup. Do you have a suggestion?

Some kind of post-fork-in-parent delegate to tie things to the PID once it’s
known? Gets a little racy but I bet we can work it out.

[Robert Sesek, 2015-09-17 22:21:54]

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
File sandbox/mac/bootstrap_sandbox.cc (right):

https://codereview.chromium.org/1346923006/diff/20001/sandbox/mac/bootstrap_s...
sandbox/mac/bootstrap_sandbox.cc:156: awaiting_processes_[token] =
sandbox_policy_id;
On 2015/09/17 22:10:53, Mark Mentovai - August is over wrote:
> Robert Sesek wrote:
> > On 2015/09/17 21:39:56, Mark Mentovai - August is over wrote:
> > > If you never get a check-in, this entry will persist in the map forever.
> > 
> > Yup. Do you have a suggestion?
> 
> Some kind of post-fork-in-parent delegate to tie things to the PID once it’s
> known? Gets a little racy but I bet we can work it out.

How about this? Anything else seems like the complexity cost outweighs the
benefit.

[Mark Mentovai, 2015-09-17 22:26:04]

LGTM

[Robert Sesek, 2015-09-17 22:30:45]

rsesek@chromium.org changed reviewers:
+ avi@chromium.org

[Robert Sesek, 2015-09-17 22:30:46]

+avi for content

[Avi (use Gerrit), 2015-09-17 23:54:20]

content lgtm

[Robert Sesek, 2015-09-18 00:47:55]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2015-09-18 00:49:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1346923006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1346923006/60001

[commit-bot: I haz the power, 2015-09-18 01:18:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-18 01:18:49]

Patchset 4 (id:??) landed as
https://crrev.com/408d2ee53267204220f2062a97bd7fe7b9a69354
Cr-Commit-Position: refs/heads/master@{#349571}