sandbox: extract SetuidSandboxHost code from SetuidSandboxClient

sandbox: extract SetuidSandboxHost code from SetuidSandboxClient

This separates the code used to "host" the setuid sandbox binary from
the code used to run underneath it (i.e., the client).  The primary
motivation for this is so that lightweight clients (e.g., the BMM
non-SFI sandbox) can avoid all of the additional dependencies required
only for hosting the setuid sandbox.

TBR=mseaborn@chromium.org,nasko@chromium.org
BUG=455087
Committed: https://crrev.com/3cc942a3dfecc18e0b20fdc509a9608644b67036
Cr-Commit-Position: refs/heads/master@{#314734}

[mdempsky, 2015-02-04 09:03:26]

mdempsky@chromium.org changed reviewers:
+ hidehiko@chromium.org, jln@chromium.org, rickyz@chromium.org

[mdempsky, 2015-02-04 09:03:26]

(Note this CL includes the changes from
https://codereview.chromium.org/885673007/ and
https://codereview.chromium.org/902533002/; I'll sync and resolve once those
have both landed.)

[rickyz (no longer on Chrome), 2015-02-04 10:46:00]

lgtm

This makes the class much easier to understand. Before this, I had to spend some
time looking into what each use was doing

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:24: base::ScopedFD
self_exe(HANDLE_EINTR(open("/", O_RDONLY)));
Not related to your change, but mind renaming this to root_dir?

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.h (right):

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:28: // (The remaining steps are
described within SetuidSandboxClient.)
nit: The comment in setuid_sandbox_host.h points the reader at the filename
instead of the class name

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:32: static class
SetuidSandboxHost* Create();
nit: Remove the class

[mdempsky, 2015-02-04 10:53:15]

New patchsets have been uploaded after l-g-t-m from rickyz@chromium.org

[mdempsky, 2015-02-04 10:53:29]

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:24: base::ScopedFD
self_exe(HANDLE_EINTR(open("/", O_RDONLY)));
On 2015/02/04 10:46:00, rickyz wrote:
> Not related to your change, but mind renaming this to root_dir?

Done.

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.h (right):

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:28: // (The remaining steps are
described within SetuidSandboxClient.)
On 2015/02/04 10:46:00, rickyz wrote:
> nit: The comment in setuid_sandbox_host.h points the reader at the filename
> instead of the class name

Done: both now use file name.

https://codereview.chromium.org/877153005/diff/40001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:32: static class
SetuidSandboxHost* Create();
On 2015/02/04 10:46:00, rickyz wrote:
> nit: Remove the class

Done.

[hidehiko, 2015-02-04 15:40:05]

LGTM. Thank you very much for your quick work!

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:64: SetuidSandboxClient*
SetuidSandboxClient::Create() {
nit, and Just-FYI: This pattern usually can be simplified by scoped_ptr.

class SetuidSandboxClient {
 public:
   ...
 private:
  explicit SetuidSandboxClient(base::Environment* env);

  scoped_ptr<base::Environment> env_;
  ...
};

SetuidSandboxClient* SetuidSandboxClient::Create() {
  return new SetuidSandboxClient(CHECK_NOTNULL(base::Environment::Create()));
}

SetuidSandboxClient::SetuidSandboxClient(base::Environment* env)
  : env_(env),
    ... {
}

SetuidSandboxClient::~SetuidSandboxClient() {
}

Or, maybe env arg of ctor also can be scoped_ptr.
Ditto for Host::Create(). Up to you.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.h (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.h:8: #include
"base/environment.h"
nit: It's better to keep including base/basictypes.h, or base/macros.h for
DISALLOW_COPY_AND_ASSIGN for the sane manner.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.cc:8: #include <sys/stat.h>
nit: #include <stdlib.h>   // for free() at L69? Or, base::FreeDeleter would be
more C++? Up to you.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.cc:20: #include "base/macros.h"
nit: Looks unused?

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.h (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2015 for added files?

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:27: // 4. A launches B with
base::LaunchProcess, using the amended CommandLine.
nit: maybe base::CommandLine, to be consistent with base::LaunchProcess?

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc:6: 
nit: Following the source code, maybe #include <string>, too?

[jln (very slow on Chromium), 2015-02-04 23:28:25]

lgtm

And +1 to hidehiko's comment to make env a scoped_ptr, but up to you.

https://chromiumcodereview.appspot.com/877153005/diff/60001/sandbox/linux/sui...
File sandbox/linux/suid/client/setuid_sandbox_host.cc (right):

https://chromiumcodereview.appspot.com/877153005/diff/60001/sandbox/linux/sui...
sandbox/linux/suid/client/setuid_sandbox_host.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2015 for new file (and remove "(c)")

https://chromiumcodereview.appspot.com/877153005/diff/60001/sandbox/linux/sui...
File sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/877153005/diff/60001/sandbox/linux/sui...
sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2015

[mdempsky, 2015-02-05 02:29:53]

New patchsets have been uploaded after l-g-t-m from
hidehiko@chromium.org,jln@chromium.org

[mdempsky, 2015-02-05 03:02:08]

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.cc:64: SetuidSandboxClient*
SetuidSandboxClient::Create() {
On 2015/02/04 15:40:05, hidehiko wrote:
> nit, and Just-FYI: This pattern usually can be simplified by scoped_ptr.

Yep, I was intending to do that in a followup CL, but went ahead and did it here
per your suggestion.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_client.h (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_client.h:8: #include
"base/environment.h"
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: It's better to keep including base/basictypes.h, or base/macros.h for
> DISALLOW_COPY_AND_ASSIGN for the sane manner.

Oops, yeah, added "base/macros.h".

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.cc:8: #include <sys/stat.h>
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: #include <stdlib.h>   // for free() at L69? Or, base::FreeDeleter would
be
> more C++? Up to you.

Bah, missed the free(), thanks.  (Also apparently I forget getenv() is from
<stdlib.h>, not <unistd.h>.)

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.cc:20: #include "base/macros.h"
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: Looks unused?

Hm, yeah.  I wonder if I accidentally added it here when I meant to add to the
.h for DISALLOW_*().  Fixed.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host.h (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: 2015 for added files?

Done.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host.h:27: // 4. A launches B with
base::LaunchProcess, using the amended CommandLine.
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: maybe base::CommandLine, to be consistent with base::LaunchProcess?

Done.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
File sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc (right):

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2015/02/04 23:28:24, jln wrote:
> 2015

Done.

https://codereview.chromium.org/877153005/diff/60001/sandbox/linux/suid/clien...
sandbox/linux/suid/client/setuid_sandbox_host_unittest.cc:6: 
On 2015/02/04 15:40:05, hidehiko wrote:
> nit: Following the source code, maybe #include <string>, too?

Done.

[mdempsky, 2015-02-05 03:13:44]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2015-02-05 03:14:32]

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

[commit-bot: I haz the power, 2015-02-05 03:20:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-05 03:20:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mdempsky, 2015-02-05 03:21:39]

mdempsky@chromium.org changed reviewers:
+ mseaborn@chromium.org, nasko@chromium.org

[mdempsky, 2015-02-05 03:22:31]

mseaborn: TBR'ing you for trivial refactoring to
components/nacl/zygote/nacl_fork_delegate_linux.cc

nasko: TBR'ing you for trivial refactoring to
content/browser/browser_main_loop.cc

[mdempsky, 2015-02-05 03:22:56]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2015-02-05 03:24:26]

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

[commit-bot: I haz the power, 2015-02-05 03:29:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-05 03:31:06]

Patchset 5 (id:??) landed as
https://crrev.com/3cc942a3dfecc18e0b20fdc509a9608644b67036
Cr-Commit-Position: refs/heads/master@{#314734}

[nasko, 2015-02-05 18:03:09]

Retroactive LGTM on content/

[Mark Seaborn, 2015-02-05 23:54:47]

LGTM for components/nacl/zygote/nacl_fork_delegate_linux.cc, for the record.