Linux sandbox: more paranoid checks

content/common/sandbox_linux.h

Index: content/common/sandbox_linux.h
diff --git a/content/common/sandbox_linux.h b/content/common/sandbox_linux.h
index 61230ca96dc6b65c1a14f1d06026f5041b0996b9..4cf698ada9c3034283129dd1768f11555fdbf5cc 100644
--- a/content/common/sandbox_linux.h
+++ b/content/common/sandbox_linux.h
@@ -37,20 +37,9 @@ class LinuxSandbox {
   static LinuxSandbox* GetInstance();
 
   // Do some initialization that can only be done before any of the sandboxes
-  // is enabled.
-  //
-  // There are two versions of this function. One takes a process_type
-  // as an argument, the other doesn't.
-  // It may be necessary to call PreinitializeSandboxBegin before knowing the
-  // process type (this is for instance the case with the Zygote).
-  // In that case, it is crucial that PreinitializeSandboxFinish() gets
-  // called for every child process.
-  // TODO(markus, jln) we know this is not always done at the moment
-  // (crbug.com/139877).
-  void PreinitializeSandbox(const std::string& process_type);
-  // These should be called together.
-  void PreinitializeSandboxBegin();
-  void PreinitializeSandboxFinish(const std::string& process_type);
+  // is enabled. If using the setuid sandbox, this should be called manually

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

Typo: "are enabled".

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

Done.

+  // before the setuid sandbox is engaged.
+  void PreinitializeSandbox();
 
   // Initialize the sandbox with the given pre-built configuration. Currently
   // seccomp-bpf and address space limitations (the setuid sandbox works
@@ -58,14 +47,15 @@ class LinuxSandbox {
   // LinuxSandbox singleton if it doesn't already exist.
   static bool InitializeSandbox();
 
-  // Returns the Status of the renderers' sandbox. Can only be queried if we
-  // went through PreinitializeSandbox() or PreinitializeSandboxBegin(). This
-  // is a bitmask and uses the constants defined in "enum LinuxSandboxStatus".
-  // Since we need to provide the status before the sandboxes are actually
-  // started, this returns what will actually happen once the various Start*
-  // functions are called from inside a renderer.
+  // Returns the Status of the renderers' sandbox. Can only be queried after
+  // going through PreinitializeSandbox(). This is a bitmask and uses the
+  // constants defined in "enum LinuxSandboxStatus". Since the status needs to

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

It would therefore be better to declare this function as returning a
LinuxSandboxStatus instead of int.

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

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

+  // be provided before the sandboxes are actually started, this returns what
+  // will actually happen once the various Start* functions are called from
+  // inside a renderer.
   int GetStatus() const;
-  // Is the current process single threaded?
+  // Is the current process single threaded? Will return "true" if it cannot be

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

NIT: Blank lines between each declaration.

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

NIT: Simplify this documentation:

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

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

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

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

Done.

+  // determined.
   bool IsSingleThreaded() const;
   // Did we start Seccomp BPF?
   bool seccomp_bpf_started() const;
@@ -90,10 +80,15 @@ class LinuxSandbox {
 
   // We must have been pre_initialized_ before using this.
   bool seccomp_bpf_supported() const;
+  // The last part of the initialization is to make sure any temporary "hole"
+  // in the sandbox is closed. For now, this consists of closing proc_fd_.
+  void SealSandbox();
 
-  int proc_fd_;
+  int proc_fd_;  // A file descriptor to /proc. It's dangerous to have it

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

NIT: Put multi-line documentation above the declaration.

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

Done.

+                 // around as it could allow for sandbox bypasses. It needs
+                 // to be closed before we consider ourselves sandboxed.
   bool seccomp_bpf_started_;
-  // Have we been through PreinitializeSandbox or PreinitializeSandboxBegin?
+  // Did PreinitializeSandbox() run?
   bool pre_initialized_;
   bool seccomp_bpf_supported_;  // Accurate if pre_initialized_.
   scoped_ptr<sandbox::SetuidSandboxClient> setuid_sandbox_client_;