Linux sandbox: more paranoid checks

Linux sandbox: more paranoid checks at initialization

We're worried that InitializeSandbox() will be called multithreaded, so we
make sure to keep a file descriptor to /proc around, in Debug mode, to be
able to count threads.

Now that seccomp-legacy has been removed, we also simplify the code around
pre-initialization.

BUG=162334
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197520

[jln (very slow on Chromium), 2013-04-30 00:10:30]

Jorge: do you mind taking a look ?

Markus: please rubberstamp once Jorge is done (or if you're around, you're
welcome to do the review)

[Jorge Lucangeli Obes, 2013-04-30 17:30:26]

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:130: DCHECK(false) << error_message;
So you're doing both the DCHECK and the LOG to always log on Release mode?

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:184: << "pre-initialized properly";
End with a colon.

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:192: // At lease "..", "." and the current
thread should be present.
least

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:195: // determining if the currenet proces is
monothreaded it works: if at any
current process

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.h:91: // Did PreinitializeSandbox() run ?
Remove space before '?'

[jln (very slow on Chromium), 2013-04-30 18:35:20]

Thanks, PTAL!

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:130: DCHECK(false) << error_message;
On 2013/04/30 17:30:27, Jorge Lucangeli Obes wrote:
> So you're doing both the DCHECK and the LOG to always log on Release mode?

Yep. If this fall through for any reason (DCHECK removed, or not a GPU process,
or anything we add in the future I want to log.

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:184: << "pre-initialized properly";
On 2013/04/30 17:30:27, Jorge Lucangeli Obes wrote:
> End with a colon.

Done.

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:192: // At lease "..", "." and the current
thread should be present.
On 2013/04/30 17:30:27, Jorge Lucangeli Obes wrote:
> least

Done.

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.cc:195: // determining if the currenet proces is
monothreaded it works: if at any
On 2013/04/30 17:30:27, Jorge Lucangeli Obes wrote:
> current process

Done.

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/14411008/diff/16001/content/common/san...
content/common/sandbox_linux.h:91: // Did PreinitializeSandbox() run ?
On 2013/04/30 17:30:27, Jorge Lucangeli Obes wrote:
> Remove space before '?'

Done.

[Jorge Lucangeli Obes, 2013-04-30 21:11:39]

lgtm

[palmer, 2013-04-30 21:21:59]

Some nits and some pontification.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.cc:266: if (proc_fd_ >= 0) {
Using proc_fd_ < 0 as a signal to express whether or not the descriptor is
closed is a bit shaky to me. The single source of truth about a descriptor's
state is in the kernel; I would just unconditionally close(proc_fd_) and then
CHECK(0 == ret || EBADF == errno). (I.e. we are happy to tolerate failure due to
EBADF — descriptor already closed — but not other failures.)

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:40: // is enabled. If using the setuid sandbox,
this should be called manually
Typo: "are enabled".

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:52: // constants defined in "enum
LinuxSandboxStatus". Since the status needs to
It would therefore be better to declare this function as returning a
LinuxSandboxStatus instead of int.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:57: // Is the current process single threaded?
Will return "true" if it cannot be
NIT: Blank lines between each declaration.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:57: // Is the current process single threaded?
Will return "true" if it cannot be
NIT: Simplify this documentation:

Returns true if the process is single-threaded, or if the number of threads
cannot be determined.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:70: // never be called with threads started. If
we detect that thread have
Typo: "...detect that threads have..."

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:87: int proc_fd_;  // A file descriptor to /proc.
It's dangerous to have it
NIT: Put multi-line documentation above the declaration.

[jln (very slow on Chromium), 2013-04-30 22:09:50]

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
File content/common/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.cc:266: if (proc_fd_ >= 0) {
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> Using proc_fd_ < 0 as a signal to express whether or not the descriptor is
> closed is a bit shaky to me. The single source of truth about a descriptor's
> state is in the kernel; I would just unconditionally close(proc_fd_) and then
> CHECK(0 == ret || EBADF == errno). (I.e. we are happy to tolerate failure due
to
> EBADF — descriptor already closed — but not other failures.)

No, this would not be correct. We can't close an arbitrary file descriptor, it
could have been re-allocated to anything else.

-1 is guaranteed to never be a valid descriptor, moreover we only close in one
place (SealSandbox()), so it's easy to guarantee that a closed fd will be set to
-1. In addition, even if there was a bug around this, the CHECK would probably
catch it at some point.

(Also it's the common pattern that we use everywhere in Linux-related code)

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
File content/common/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:40: // is enabled. If using the setuid sandbox,
this should be called manually
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> Typo: "are enabled".

Done.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:52: // constants defined in "enum
LinuxSandboxStatus". Since the status needs to
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> It would therefore be better to declare this function as returning a
> LinuxSandboxStatus instead of int.

No, unfortunately it has to return an int. It's a bitmask of values in
LinuxSandboxStatus, not an element of LinuxSandboxStatus.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:57: // Is the current process single threaded?
Will return "true" if it cannot be
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> NIT: Blank lines between each declaration.

Vertical space is only required between different section. I already put a
little bit too much vertical whitespace in this class imho.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:57: // Is the current process single threaded?
Will return "true" if it cannot be
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> NIT: Simplify this documentation:
> 
> Returns true if the process is single-threaded, or if the number of threads
> cannot be determined.

Done.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:70: // never be called with threads started. If
we detect that thread have
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> Typo: "...detect that threads have..."

Done.

https://chromiumcodereview.appspot.com/14411008/diff/21001/content/common/san...
content/common/sandbox_linux.h:87: int proc_fd_;  // A file descriptor to /proc.
It's dangerous to have it
On 2013/04/30 21:21:59, Chromium Palmer wrote:
> NIT: Put multi-line documentation above the declaration.

Done.

[jln (very slow on Chromium), 2013-04-30 22:10:15]

Thanks Chris, PTAL!

[palmer, 2013-04-30 22:15:56]

lgtm

[commit-bot: I haz the power, 2013-04-30 22:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/14411008/30001

[commit-bot: I haz the power, 2013-04-30 23:49:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/14411008/30001

[commit-bot: I haz the power, 2013-05-01 00:04:19]

Change committed as 197520