Refactor configuration of sandboxes - first steps

content/browser/worker_host/worker_process_host.cc

Index: content/browser/worker_host/worker_process_host.cc
diff --git a/content/browser/worker_host/worker_process_host.cc b/content/browser/worker_host/worker_process_host.cc
index 91b79382db0e61abc4e3e8369be56a7f1c57aef2..7019283503e336a186276e580d69684c53126b2b 100644
--- a/content/browser/worker_host/worker_process_host.cc
+++ b/content/browser/worker_host/worker_process_host.cc
@@ -47,6 +47,7 @@
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/result_codes.h"
+#include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_switches.h"
 #include "net/base/mime_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
@@ -58,7 +59,6 @@
 
 #if defined(OS_WIN)
 #include "content/common/sandbox_win.h"
-#include "content/public/common/sandboxed_process_launcher_delegate.h"
 #endif
 
 namespace content {
@@ -69,7 +69,7 @@ namespace {
 class WorkerSandboxedProcessLauncherDelegate
     : public content::SandboxedProcessLauncherDelegate {
  public:
-  WorkerSandboxedProcessLauncherDelegate() {}
+  WorkerSandboxedProcessLauncherDelegate(ChildProcessHost* /*host*/) {}
   virtual ~WorkerSandboxedProcessLauncherDelegate() {}
 
   virtual void PreSpawnTarget(sandbox::TargetPolicy* policy,
@@ -77,6 +77,48 @@ class WorkerSandboxedProcessLauncherDelegate
     AddBaseHandleClosePolicy(policy);
   }
 };
+
+#elif defined(OS_POSIX)
+// NOTE: changes to this class need to be reviewed by the security team.

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

ditto

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

Done.

+class WorkerSandboxedProcessLauncherDelegate
+    : public content::SandboxedProcessLauncherDelegate {
+ public:
+  WorkerSandboxedProcessLauncherDelegate(ChildProcessHost* host)
+      : ipc_fd_(host->TakeClientFileDescriptor()) {}
+
+  virtual ~WorkerSandboxedProcessLauncherDelegate() {}
+
+  virtual bool UseZygote() OVERRIDE {
+
+    // If debugging the child then disable the zigote
+    if (CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kWaitForDebuggerChildren)) {
+      // Look to pass-on the kWaitForDebugger flag.
+      std::string value = CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kWaitForDebuggerChildren);
+      if (value.empty() || value == switches::kWorkerProcess) {
+        return false;
+      }
+    }
+
+    if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kDebugChildren)) {
+      // Look to pass-on the kDebugOnStart flag.
+      std::string value = CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+          switches::kDebugChildren);
+      if (value.empty() || value == switches::kWorkerProcess) {
+        // launches a new xterm, and runs the worker process in gdb, reading
+        // optional commands from gdb_chrome file in the working directory.
+        return false;
+      }
+    }

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

this method body looks like a copy of the code that's still there but which only
changes flags. why not keep the logic of use_zygote there, and pass it as an
argument to this class?

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

Done. BTW I am slightly puzzled as to why there isn't equivalent code to this
for the other process types. The documentation of these flags suggest that they
apply to all child processes. If this should apply to all processes then it
seems it could be moved down into ChildProcessLauncher::LaunchInternal. This is,
however, probably a matter for a different CL.

[jam, 2014/02/28 18:07:43]

it's probably an oversight..

+    return true;
+  }
+  virtual int IpcFd() OVERRIDE {
+    return ipc_fd_;
+  }
+ private:
+  int ipc_fd_;
+};
 #endif  // OS_WIN
 
 // Notifies RenderViewHost that one or more worker objects crashed.
@@ -194,8 +236,6 @@ bool WorkerProcessHost::Init(int render_process_id, int render_frame_id) {
                              arraysize(kSwitchNames));
 
 #if defined(OS_POSIX)
-  bool use_zygote = true;
-
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kWaitForDebuggerChildren)) {
     // Look to pass-on the kWaitForDebugger flag.
@@ -203,7 +243,6 @@ bool WorkerProcessHost::Init(int render_process_id, int render_frame_id) {
         switches::kWaitForDebuggerChildren);
     if (value.empty() || value == switches::kWorkerProcess) {
       cmd_line->AppendSwitch(switches::kWaitForDebugger);
-      use_zygote = false;
     }
   }
 
@@ -215,19 +254,12 @@ bool WorkerProcessHost::Init(int render_process_id, int render_frame_id) {
       // launches a new xterm, and runs the worker process in gdb, reading
       // optional commands from gdb_chrome file in the working directory.
       cmd_line->PrependWrapper("xterm -e gdb -x gdb_chrome --args");
-      use_zygote = false;
     }
   }
 #endif
 
   process_->Launch(
-#if defined(OS_WIN)
-      new WorkerSandboxedProcessLauncherDelegate,
-      false,
-#elif defined(OS_POSIX)
-      use_zygote,
-      base::EnvironmentMap(),
-#endif
+      new WorkerSandboxedProcessLauncherDelegate(process_->GetHost()),
       cmd_line);
 
   ChildProcessSecurityPolicyImpl::GetInstance()->AddWorker(