New NaCl zygote implementation 2

content/browser/zygote_main_linux.cc

Index: content/browser/zygote_main_linux.cc
diff --git a/content/browser/zygote_main_linux.cc b/content/browser/zygote_main_linux.cc
index 99a773556ff960383693cdc3e3fe7a0d1ae05ce2..267975b5d02f59662ccb66cff66ecd53b9a5ae22 100644
--- a/content/browser/zygote_main_linux.cc
+++ b/content/browser/zygote_main_linux.cc
@@ -37,9 +37,11 @@
 #include "content/common/sandbox_methods_linux.h"
 #include "content/common/set_process_title.h"
 #include "content/common/unix_domain_socket_posix.h"
+#include "content/common/zygote_fork_delegate_linux.h"
 #include "seccompsandbox/sandbox.h"
 #include "skia/ext/SkFontHost_fontconfig_control.h"
 #include "unicode/timezone.h"
+#include "ipc/ipc_switches.h"
 
 #if defined(OS_LINUX)
 #include <sys/epoll.h>
@@ -92,13 +94,29 @@ static void SELinuxTransitionToTypeOrDie(const char* type) {
 }
 #endif  // CHROMIUM_SELINUX
 
+// Search through string vector args for a string of the form
+// --swtch=value. Return the string if found, else return the
+// empty string.

[Evan Martin, 2011/06/27 18:49:32]

I'm a little concerned that this is incorrect for a command line like:
  google-chrome -- http://foo.com/--swtch=1234/blah

How much should I worry?  It would be nice to reuse our existing
command-line-parsing code for this sort of thing.

Or perhaps it would make more sense to just explicitly include the needed info
in the initial pickle that we IPC, even if it's redundant with the args that
we're sending.

I'd be ok with just marking this TODO() if you don't think it's
security-sensitive.

[Brad Chen, 2011/06/27 22:06:25]

I never liked this code this much anyway. Realizing it's not throw away I've
modified HandleForkRequest (same file) to avoid using it. Hopefully you'll agree
this is better.

On 2011/06/27 18:49:32, Evan Martin wrote:

+static std::string ExtractArg(std::vector<std::string>& args,
+                              const std::string& swtch) {
+  const std::string key = "--" + swtch + "=";
+  int len = key.length();
+  for (size_t i = 0; i < args.size(); i++) {
+    if (key.compare(0, len, args[i], 0, len) == 0) {
+      return args[i];
+    }
+  }
+  return "";
+}
+
 // This is the object which implements the zygote. The ZygoteMain function,
 // which is called from ChromeMain, simply constructs one of these objects and
 // runs it.
 class Zygote {
  public:
-  explicit Zygote(int sandbox_flags)
-      : sandbox_flags_(sandbox_flags) {
+  explicit Zygote(int sandbox_flags, ZygoteForkDelegate* helper)
+    : sandbox_flags_(sandbox_flags),
+      helper_(helper) {
   }
 
   bool ProcessRequests() {
@@ -165,6 +183,7 @@ class Zygote {
         case ZygoteHost::kCmdFork:
           // This function call can return multiple times, once per fork().
           return HandleForkRequest(fd, pickle, iter, fds);
+
         case ZygoteHost::kCmdReap:
           if (!fds.empty())
             break;
@@ -247,9 +266,12 @@ class Zygote {
   // sandbox, it returns the real PID of the child process as it
   // appears outside the sandbox, rather than returning the PID inside
   // the sandbox.
-  int ForkWithRealPid() {
-    if (!g_suid_sandbox_active)
+  int ForkWithRealPid(const std::string process_type, std::vector<int>& fds,

[Evan Martin, 2011/06/27 18:49:32]

These should be const std::string&

[Brad Chen, 2011/06/27 22:06:25]

Done.

+                      const std::string channel_switch) {
+    const bool use_helper = (helper_ && helper_->CanHelp(process_type));
+    if (!(use_helper || g_suid_sandbox_active)) {
       return fork();
+    }
 
     int dummy_fd;
     ino_t dummy_inode;
@@ -270,7 +292,13 @@ class Zygote {
       goto error;
     }
 
-    pid = fork();
+    if (use_helper) {
+      fds.push_back(dummy_fd);
+      fds.push_back(pipe_fds[0]);
+      pid = helper_->Fork(fds);
+    } else {
+      pid = fork();
+    }
     if (pid < 0) {
       goto error;
     } else if (pid == 0) {
@@ -294,33 +322,43 @@ class Zygote {
       dummy_fd = -1;
       close(pipe_fds[0]);
       pipe_fds[0] = -1;
-      uint8_t reply_buf[512];
-      Pickle request;
-      request.WriteInt(LinuxSandbox::METHOD_GET_CHILD_WITH_INODE);
-      request.WriteUInt64(dummy_inode);
-
-      const ssize_t r = UnixDomainSocket::SendRecvMsg(
-          kMagicSandboxIPCDescriptor, reply_buf, sizeof(reply_buf), NULL,
-          request);
-      if (r == -1) {
-        LOG(ERROR) << "Failed to get child process's real PID";
-        goto error;
-      }
-
       base::ProcessId real_pid;
-      Pickle reply(reinterpret_cast<char*>(reply_buf), r);
-      void* iter2 = NULL;
-      if (!reply.ReadInt(&iter2, &real_pid))
-        goto error;
-      if (real_pid <= 0) {
-        // METHOD_GET_CHILD_WITH_INODE failed. Did the child die already?
-        LOG(ERROR) << "METHOD_GET_CHILD_WITH_INODE failed";
-        goto error;
+      if (g_suid_sandbox_active) {
+        uint8_t reply_buf[512];
+        Pickle request;
+        request.WriteInt(LinuxSandbox::METHOD_GET_CHILD_WITH_INODE);
+        request.WriteUInt64(dummy_inode);
+
+        const ssize_t r = UnixDomainSocket::SendRecvMsg(
+            kMagicSandboxIPCDescriptor, reply_buf, sizeof(reply_buf), NULL,
+            request);
+        if (r == -1) {
+          LOG(ERROR) << "Failed to get child process's real PID";
+          goto error;
+        }
+
+        Pickle reply(reinterpret_cast<char*>(reply_buf), r);
+        void* iter2 = NULL;

[Evan Martin, 2011/06/27 18:49:32]

Why "iter2" and not just "iter"?

[Brad Chen, 2011/06/27 22:06:25]

An irrelevant artifact; fixed.

On 2011/06/27 18:49:32, Evan Martin wrote:

+        if (!reply.ReadInt(&iter2, &real_pid))
+          goto error;
+        if (real_pid <= 0) {
+          // METHOD_GET_CHILD_WITH_INODE failed. Did the child die already?
+          LOG(ERROR) << "METHOD_GET_CHILD_WITH_INODE failed";
+          goto error;
+        }
+        real_pids_to_sandbox_pids[real_pid] = pid;
       }
-      real_pids_to_sandbox_pids[real_pid] = pid;
-      if (HANDLE_EINTR(write(pipe_fds[1], "x", 1)) != 1) {
-        LOG(ERROR) << "Failed to synchronise with child process";
-        goto error;
+      if (use_helper) {
+        real_pid = pid;
+        if (!helper_->AckChild(pipe_fds[1], channel_switch)) {
+          LOG(ERROR) << "Failed to synchronise with NaCl child process";
+          goto error;
+        }
+      } else {
+        if (HANDLE_EINTR(write(pipe_fds[1], "x", 1)) != 1) {
+          LOG(ERROR) << "Failed to synchronise with child process";
+          goto error;
+        }
       }
       close(pipe_fds[1]);
       return real_pid;
@@ -342,12 +380,16 @@ class Zygote {
 
   // Handle a 'fork' request from the browser: this means that the browser
   // wishes to start a new renderer.
-  bool HandleForkRequest(int fd, const Pickle& pickle, void* iter,
-                         std::vector<int>& fds) {
+  bool HandleForkRequest(int fd, const Pickle& pickle,
+                         void* iter, std::vector<int>& fds) {
     std::vector<std::string> args;
     int argc, numfds;
     base::GlobalDescriptors::Mapping mapping;
     base::ProcessId child;
+    std::string process_type;
+
+    if (!pickle.ReadString(&iter, &process_type))
+      goto error;
 
     if (!pickle.ReadInt(&iter, &argc))
       goto error;
@@ -374,7 +416,8 @@ class Zygote {
     mapping.push_back(std::make_pair(
         static_cast<uint32_t>(kSandboxIPCChannel), kMagicSandboxIPCDescriptor));
 
-    child = ForkWithRealPid();
+    child = ForkWithRealPid(process_type, fds,
+                            ExtractArg(args, switches::kProcessChannelID));
 
     if (!child) {
 #if defined(SECCOMP_SANDBOX)
@@ -447,6 +490,7 @@ class Zygote {
   ProcessMap real_pids_to_sandbox_pids;
 
   const int sandbox_flags_;
+  ZygoteForkDelegate* helper_;
 };
 
 // With SELinux we can carve out a precise sandbox, so we don't have to play
@@ -705,7 +749,8 @@ static bool EnterSandbox() {
 
 #endif  // CHROMIUM_SELINUX
 
-bool ZygoteMain(const MainFunctionParams& params) {
+bool ZygoteMain(const MainFunctionParams& params,
+                ZygoteForkDelegate* forkdelegate) {
 #if !defined(CHROMIUM_SELINUX)
   g_am_zygote_or_renderer = true;
 #endif
@@ -724,6 +769,10 @@ bool ZygoteMain(const MainFunctionParams& params) {
   }
 #endif  // SECCOMP_SANDBOX
 
+  VLOG(1) << "initializing fork delegate";
+  forkdelegate->Init(getenv("SBX_D") != NULL, // g_suid_sandbox_active,
+                     kBrowserDescriptor, kMagicSandboxIPCDescriptor);
+
   // Turn on the SELinux or SUID sandbox
   if (!EnterSandbox()) {
     LOG(FATAL) << "Failed to enter sandbox. Fail safe abort. (errno: "
@@ -760,7 +809,7 @@ bool ZygoteMain(const MainFunctionParams& params) {
   }
 #endif  // SECCOMP_SANDBOX
 
-  Zygote zygote(sandbox_flags);
+  Zygote zygote(sandbox_flags, forkdelegate);
   // This function call can return multiple times, once per fork().
   return zygote.ProcessRequests();
 }