Refactor configuration of sandboxes - first steps

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index a5a1341dcfb16c65a3bcc8baf41eccd24c8c6b65..e956fc68d6bdc88e63f3546ae560c745827cb9c1 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -40,6 +40,7 @@
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
+#include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_switches.h"
 #include "native_client/src/shared/imc/nacl_imc_c.h"
@@ -63,7 +64,6 @@
 #include "components/nacl/browser/nacl_broker_service_win.h"
 #include "components/nacl/common/nacl_debug_exception_handler_win.h"
 #include "content/public/common/sandbox_init.h"
-#include "content/public/common/sandboxed_process_launcher_delegate.h"
 #endif
 
 using content::BrowserThread;
@@ -133,7 +133,7 @@ bool RunningOnWOW64() {
 class NaClSandboxedProcessLauncherDelegate
     : public content::SandboxedProcessLauncherDelegate {
  public:
-  NaClSandboxedProcessLauncherDelegate() {}
+  NaClSandboxedProcessLauncherDelegate(ChildProcessHost* /*host*/) {}
   virtual ~NaClSandboxedProcessLauncherDelegate() {}
 
   virtual void PostSpawnTarget(base::ProcessHandle process) {
@@ -149,6 +149,28 @@ class NaClSandboxedProcessLauncherDelegate
     }
   }
 };
+#elif defined(OS_POSIX)

[jam, 2014/02/26 19:47:52]

why have two implementations in this file instead of just one with ifdefs? the
latter is how things are usually done. especially if one of the motivations here
is to have more code sharing between different platforms

[aberent, 2014/02/28 08:51:06]

Done. I have tried various versions of this, and whatever one does it seems to
get a bit messy because of the need for different initializers in the different
implementations of the constructor. I would welcome your opinion on the way I do
it in the next patch set.

+// NOTE: changes to this class need to be reviewed by the security team.
+class NaClSandboxedProcessLauncherDelegate
+    : public content::SandboxedProcessLauncherDelegate {
+ public:
+  NaClSandboxedProcessLauncherDelegate(ChildProcessHost* host)
+      : ipc_fd_(host->TakeClientFileDescriptor()) {}
+
+  virtual ~NaClSandboxedProcessLauncherDelegate() {}
+
+  virtual bool UseZygote() OVERRIDE {
+    CommandLine::StringType nacl_loader_prefix =
+        CommandLine::ForCurrentProcess()->GetSwitchValueNative(
+            switches::kNaClLoaderCmdPrefix);
+    return nacl_loader_prefix.empty();
+  }
+  virtual int IpcFd() OVERRIDE {
+    return ipc_fd_;
+  }
+ private:
+  int ipc_fd_;
+};
 
 #endif  // OS_WIN
 
@@ -535,17 +557,12 @@ bool NaClProcessHost::LaunchSelLdr() {
       SendErrorToRenderer("broker service did not launch process");
       return false;
     }
-  } else {
-    process_->Launch(new NaClSandboxedProcessLauncherDelegate,
-                     false,
-                     cmd_line.release());
+    return true;
   }
-#elif defined(OS_POSIX)
-  process_->Launch(nacl_loader_prefix.empty(),  // use_zygote
-                   base::EnvironmentMap(),
-                   cmd_line.release());
 #endif
-
+  process_->Launch(
+      new NaClSandboxedProcessLauncherDelegate(process_->GetHost()),
+      cmd_line.release());
   return true;
 }