New NaCl zygote implementation 2

chrome/nacl/nacl_helper_linux.cc

Index: chrome/nacl/nacl_helper_linux.cc
diff --git a/chrome/nacl/nacl_helper_linux.cc b/chrome/nacl/nacl_helper_linux.cc
new file mode 100644
index 0000000000000000000000000000000000000000..728b98baff0443132c3241fe4592e9e90c85dce0
--- /dev/null
+++ b/chrome/nacl/nacl_helper_linux.cc
@@ -0,0 +1,161 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// A mini-zygote specifically for Native Client.
+
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <unistd.h>
+
+#include <vector>
+
+#include "base/at_exit.h"
+#include "base/command_line.h"
+#include "base/eintr_wrapper.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
+#include "chrome/nacl/nacl_helper_linux.h"
+#include "content/common/main_function_params.h"
+#include "content/common/unix_domain_socket_posix.h"
+#include "webkit/glue/webkit_glue.h"
+
+#ifdef NDEBUG

[agl, 2011/06/10 17:28:03]

Deserves a comment about why it's not using the usual DCHECK.

[agl, 2011/06/10 17:28:03]

Optional nit: Chrome code typically uses

#if defined(X)
and
#if !defined(X)

[Brad Chen, 2011/06/14 00:16:01]

Done.

+#define DCHECK(condition) {}
+#else
+#define DCHECK(condition) \
+  { if (!condition) fprintf(stderr, "Check failed: %s.\n", #condition); }
+#endif
+
+#ifndef NDEBUG

[agl, 2011/06/10 17:28:03]

optional nit: #if !defined(NDEBUG)

[Brad Chen, 2011/06/14 00:16:01]

Done.

+// These two symbols from webkit_glue are being referenced someplace
+// in debug builds but apparently are not being called. I do not know
+// from where they are referenced, but as long as it's not in release
+// builds it's okay.
+namespace webkit_glue {
+
+bool FindProxyForUrl(const GURL& url, std::string* proxy_list) {
+  fprintf(stderr, "Who is calling FindProxyForUrl?");
+  _exit(-1);
+  return false;
+}
+
+std::string GetProductVersion() {
+  static std::string version("What is the product version?");
+  fprintf(stderr, "Who is calling GetProductVersion?");
+  _exit(-1);
+  return version;

[agl, 2011/06/10 17:28:03]

Seems superfluous to return anything but an empty string after _exit.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+}
+
+}  // namespace webkit_glue
+#endif  // NDEBUG
+
+const bool g_suid_sandbox_active;

[agl, 2011/06/10 17:28:03]

should either be extern, static or needs a comment about where else it's used.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+extern int NaClMain(const MainFunctionParams&);
+
+// The child must mimic the behavior of zygote_main_linux.c on the child
+// side of the fork. See zygote_main_linux.cc:HandleForkRequest from
+//   if (!child) {
+// Note this code doesn't attempt to support SELINUX or the SECCOMP sandbox.

[agl, 2011/06/10 17:28:03]

nit: Either "Note: " or "Note that "

+void BeLoader(const int zygote_fd, const std::vector<int> child_fds) {

[agl, 2011/06/10 17:28:03]

static

[agl, 2011/06/10 17:28:03]

I think you should consider renaming this function. BecomeNaClLoader or
something?

[Brad Chen, 2011/06/14 00:16:01]

Done.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+  // NaCl main expects the child_fd to be on kZygoteDescriptor
+  DCHECK(zygote_fd == kZygoteDescriptor);
+  close(zygote_fd);
+  DCHECK(kZygoteDescriptor == dup(child_fds[0]));
+
+  base::AtExitManager exit_manager;
+  base::mac::ScopedNSAutoreleasePool autorelease_pool;
+  SandboxInitWrapper sandbox_wrapper;
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  const MainFunctionParams mainparams(command_line,
+                                      sandbox_wrapper,
+                                      &autorelease_pool);
+
+  NaClMain(mainparams);
+  _exit(0);
+}
+
+// Some of this code was lifted from
+// content/browser/zygote_main_linux.cc:ForkWithRealPid()
+void HandleForkRequest(const int zygote_fd, const std::vector<int> child_fds) {

[agl, 2011/06/10 17:28:03]

static

[agl, 2011/06/10 17:28:03]

ampersand after "const std::vector<int>", I suspect.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+  pid_t childpid = fork();
+  if (childpid < 0) {
+    perror("fork");
+    fprintf(stderr, "*** HandleForkRequest failed\n");
+    // fall through to parent case below
+  } else if (childpid == 0) {  // In the child process.
+    if (g_suid_sandbox_active) {
+      char buffer[1];
+      // Wait until the parent process has discovered our PID.  We
+      // should not fork any child processes (which the seccomp
+      // sandbox does) until then, because that can interfere with the
+      // parent's discovery of our PID.
+      int nread = HANDLE_EINTR(read(child_fds[2], buffer, 1));
+      if (nread != 1 || buffer[0] != 'x') {
+        perror("read");
+        fprintf(stderr, "Failed to synch with zygote (%d:%d)\n",
+                nread, buffer[0]);
+      } else {
+        fprintf(stderr, "NaCl loader is synchronised with Chrome zygote");
+      }
+    }
+    BeLoader(zygote_fd, child_fds);
+    // NOTREACHED
+    return;
+  }
+  // I am the parent.
+  // First, close the dummy_fd so the sandbox won't find me when
+  // looking for the child's pid in /proc. Also close other fds.
+  for (size_t i = 0; i < child_fds.size(); i++) close(child_fds[i]);

[agl, 2011/06/10 17:28:03]

Body of loop on its own line.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+  // Now tell childpid to the Chrome zygote.
+  if (send(zygote_fd, &childpid, sizeof(childpid), MSG_EOR)
+      != sizeof(childpid)) {
+    fprintf(stderr, "*** send() to zygote failed");
+  }
+}
+
+int main(int argc, char *argv[]) {
+  pid_t mypid = getpid();
+  CommandLine::Init(argc, argv);
+
+  g_suid_sandbox_active = (NULL != getenv("SBX_D"));
+
+  if (send(kZygoteDescriptor, kNaClHelperMagic,
+           sizeof(kNaClHelperMagic), MSG_EOR) != 4) {
+    perror("send");
+  }
+
+  while (1) {

[agl, 2011/06/10 17:28:03]

nit: while (true) is more typical of Chrome code.

[Brad Chen, 2011/06/14 00:16:01]

Done.

+    int badpid = -1;
+    std::vector<int> fds;
+    static const unsigned kMaxMessageLength = 2048;
+    char buf[kMaxMessageLength];
+    const ssize_t msglen = (UnixDomainSocket::RecvMsg(kZygoteDescriptor,

[agl, 2011/06/10 17:28:03]

parentheses around RHS are superfluous. 

[Brad Chen, 2011/06/14 00:16:01]

Done.

+                                                      &buf, sizeof(buf), &fds));
+    if (0 == msglen || (-1 == msglen && ECONNRESET == errno)) {
+      // EOF from zygote. Rest in peace...
+      _exit(0);
+      return 0;
+    }
+    if (msglen == -1) {
+      fprintf(stderr, "NaCl helper: error reading message from zygote\n");
+      continue;
+    }
+    // fork request
+    if (0 == strcmp(buf, kNaClForkRequest)) {

[agl, 2011/06/10 17:28:03]

if (msglen == sizeof(kNaClForkRequest) - 1 && memcmp(buf, kNaClForkRequest,
msglen) == 0)

[Brad Chen, 2011/06/14 00:16:01]

Done.

+      if (1 == fds.size() || 3 == fds.size()) {
+        HandleForkRequest(kZygoteDescriptor, fds);
+        // child does not return
+      } else {
+        fprintf(stderr, "NaCl helper expected 1 or 3 fds, got %d\n",
+                int(fds.size()));
+        send(kZygoteDescriptor, &badpid, sizeof(badpid), MSG_EOR);
+      }
+    } else {
+      fprintf(stderr, "NaCl helper unrecognized request: %s\n", buf);
+      send(kZygoteDescriptor, &badpid, sizeof(badpid), MSG_EOR);
+    }
+  }
+}