Linux sandbox: refactor BrokerProcess class

Linux sandbox: refactor BrokerProcess class

The BrokerProcess class is too complex. It both serves as management
of a new broker process, as well as the implementation of an
async-signal-safe IPC mechanism suitable to broker syscall.

We split the BrokerProcess class into 4 simpler classes. This will make it
easier to replace the broker process with a thread or a new Chrome process
type.

BUG=429028
Committed: https://crrev.com/70c42c20bd408c9f1b4d43f5154da43c16529cf8
Cr-Commit-Position: refs/heads/master@{#302370}

[jln (very slow on Chromium), 2014-10-31 03:37:21]

jln@chromium.org changed reviewers:
+ mdempsky@chromium.org

[jln (very slow on Chromium), 2014-10-31 03:40:38]

Matthew: I apologize in advance for an annoying review.

This is a first step towards a refactor of the BrokerProcess class. The body of
pretty much every function is functionally the same as before, but the review
tool will detect nothing as identical.

What's important to review are the new class interfaces (and their
constructors/destructors which are new.

[jln (very slow on Chromium), 2014-10-31 17:44:57]

rickyz or leecam: would you mind doing a first-pass before Matthew?

[leecam, 2014-10-31 17:47:35]

On 2014/10/31 17:44:57, jln (OOO til 29th) wrote:
> rickyz or leecam: would you mind doing a first-pass before Matthew?

I'll take a look now

[leecam, 2014-10-31 21:06:24]

leecam@chromium.org changed reviewers:
+ leecam@chromium.org

[leecam, 2014-10-31 21:06:25]

lgtm - I did my review on ps3 but comments looks good in the new sets

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
File sandbox/linux/syscall_broker/broker_host.cc (right):

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:98:
write_pickle->WriteInt(-access_errno);
OpenFileForIPC just does -errno for consistency we could lose the local
acccess_errno

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:117: return -1;
return false

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:133: break;
Should we return false here as we encountered a bad IPC request? If we don't
we'll just reply with an empty pickle

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:176: // EOF from our parent, or our
parent died, we should die.
Assuming the other end is our parent is an implementation detail that isn't
relevant to this class. Maybe s/parent/client

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:177: _exit(0);
Seems a little harsh :) Maybe return false or an error code (so you can
distinguish from parsing errors) and let the BrokerProcess dictate the action
and process lifetime management. Could wait until the next CL

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_host.cc:180: // The parent should send
exactly one file descriptor, on which we
parent=client

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
File sandbox/linux/syscall_broker/broker_policy.cc (right):

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_policy.cc:103: //
GetFileNameIfAllowedToOpen() fore more explanation.
for

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
File sandbox/linux/syscall_broker/broker_policy.h (right):

https://codereview.chromium.org/688843003/diff/40001/sandbox/linux/syscall_br...
sandbox/linux/syscall_broker/broker_policy.h:28: // GetFileNameIfAllowedToOpen()
fore more explanation.
for

[jln (very slow on Chromium), 2014-10-31 21:43:19]

Thanks for the quick review Lee!

Matthew: PTAL!

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
File sandbox/linux/syscall_broker/broker_host.cc (right):

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:98:
write_pickle->WriteInt(-access_errno);
On 2014/10/31 21:06:24, leecam wrote:
> OpenFileForIPC just does -errno for consistency we could lose the local
> acccess_errno

In OpenFileForIPC, we use errno immediately after it has been modified by
(open). In this case, since we're going through WriteInt I fear it could get
modified.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:117: return -1;
On 2014/10/31 21:06:25, leecam wrote:
> return false

Done.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:133: break;
On 2014/10/31 21:06:25, leecam wrote:
> Should we return false here as we encountered a bad IPC request? If we don't
> we'll just reply with an empty pickle

May be worth considering, but I don't want to risk changing the behavior in this
CL.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:176: // EOF from our parent, or our
parent died, we should die.
On 2014/10/31 21:06:25, leecam wrote:
> Assuming the other end is our parent is an implementation detail that isn't
> relevant to this class. Maybe s/parent/client

Done.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:177: _exit(0);
On 2014/10/31 21:06:25, leecam wrote:
> Seems a little harsh :) Maybe return false or an error code (so you can
> distinguish from parsing errors) and let the BrokerProcess dictate the action
> and process lifetime management. Could wait until the next CL

Yep, for sure! Especially if we embed this in the browser. Definitely something
to do in next CL.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_host.cc:180: // The parent should send
exactly one file descriptor, on which we
On 2014/10/31 21:06:25, leecam wrote:
> parent=client

Done.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
File sandbox/linux/syscall_broker/broker_policy.cc (right):

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_policy.cc:103: //
GetFileNameIfAllowedToOpen() fore more explanation.
On 2014/10/31 21:06:25, leecam wrote:
> for

Done.

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
File sandbox/linux/syscall_broker/broker_policy.h (right):

https://chromiumcodereview.appspot.com/688843003/diff/40001/sandbox/linux/sys...
sandbox/linux/syscall_broker/broker_policy.h:28: // GetFileNameIfAllowedToOpen()
fore more explanation.
On 2014/10/31 21:06:25, leecam wrote:
> for

Done.

[mdempsky, 2014-10-31 22:13:43]

lgtm

Handful of optional cleanups.  So feel free to defer to a followup CL if you're
already working on that.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_client.h (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_client.h:38: // Can be used in place of
access(). Will be async signal safe.
nit: Might be clearer to say "Is async signal safe".  ("Will be" suggests to me
that the code doesn't currently have this property, but in the future it 'will
be' updated to.)

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_client.h:43: // Can be used in place of
open(). Will be async signal safe.
(Here too, if you decide to change it.)

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_common.h (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:9: #include <stdint.h>
size_t is in <stddef.h>, not <stdint.h>

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:11: #include <string>
These aren't needed I think.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:18: static const size_t
kMaxMessageLength = 4096;
Probably don't want 'static' if you're putting these in a header file?

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:35: kCommandInvalid = 0,
Chromium style says enums use SHOUTY_CASE, though I know this is an existing
style violation.  If you do change it, it might be worth making it:

    enum class IPCCommand {
      INVALID = 0,
      OPEN,
      ACCESS,
    };

since the "Command" part becomes redundant when you have to write it
"IPCCommand::OPEN" anyway.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_policy.cc (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_policy.cc:73: for (it =
allowed_file_names.begin(); it != allowed_file_names.end(); it++) {
This could now be:

  for (const auto& allowed_file_name : allowed_file_names) {
    ...
  }

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_policy.cc:131:
GetFileNameInWhitelist(allowed_r_files_, requested_filename, NULL) &&
Could do s/NULL/nullptr/.

[jln (very slow on Chromium), 2014-10-31 23:15:50]

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_client.h (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_client.h:38: // Can be used in place of
access(). Will be async signal safe.
On 2014/10/31 22:13:43, mdempsky wrote:
> nit: Might be clearer to say "Is async signal safe".  ("Will be" suggests to
me
> that the code doesn't currently have this property, but in the future it 'will
> be' updated to.)

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_client.h:43: // Can be used in place of
open(). Will be async signal safe.
On 2014/10/31 22:13:43, mdempsky wrote:
> (Here too, if you decide to change it.)

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_common.h (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:9: #include <stdint.h>
On 2014/10/31 22:13:43, mdempsky wrote:
> size_t is in <stddef.h>, not <stdint.h>

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:11: #include <string>
On 2014/10/31 22:13:43, mdempsky wrote:
> These aren't needed I think.

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:18: static const size_t
kMaxMessageLength = 4096;
On 2014/10/31 22:13:43, mdempsky wrote:
> Probably don't want 'static' if you're putting these in a header file?

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_common.h:35: kCommandInvalid = 0,
On 2014/10/31 22:13:43, mdempsky wrote:
> Chromium style says enums use SHOUTY_CASE, though I know this is an existing
> style violation.  If you do change it, it might be worth making it:
> 
>     enum class IPCCommand {
>       INVALID = 0,
>       OPEN,
>       ACCESS,
>     };
> 
> since the "Command" part becomes redundant when you have to write it
> "IPCCommand::OPEN" anyway.

Done, but I haven't converted to an enum class: unfortunately I'm deserializing
an int from the IPC channel and I would need to explicitly set-up serialization
for this enum instead.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
File sandbox/linux/syscall_broker/broker_policy.cc (right):

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_policy.cc:73: for (it =
allowed_file_names.begin(); it != allowed_file_names.end(); it++) {
On 2014/10/31 22:13:43, mdempsky wrote:
> This could now be:
> 
>   for (const auto& allowed_file_name : allowed_file_names) {
>     ...
>   }

Done.

https://chromiumcodereview.appspot.com/688843003/diff/100001/sandbox/linux/sy...
sandbox/linux/syscall_broker/broker_policy.cc:131:
GetFileNameInWhitelist(allowed_r_files_, requested_filename, NULL) &&
On 2014/10/31 22:13:43, mdempsky wrote:
> Could do s/NULL/nullptr/.

Acknowledged.

[jln (very slow on Chromium), 2014-10-31 23:44:11]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-10-31 23:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/688843003/140001

[commit-bot: I haz the power, 2014-11-01 01:00:17]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-01 01:00:49]

Patchset 8 (id:??) landed as
https://crrev.com/70c42c20bd408c9f1b4d43f5154da43c16529cf8
Cr-Commit-Position: refs/heads/master@{#302370}