New NaCl zygote implementation 2

chrome/nacl/nacl_helper_linux.cc

+// 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 <stdlib.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 "base/rand_util.h"
+#include "chrome/common/nacl_helper_linux.h"
+#include "content/common/main_function_params.h"
+#include "content/common/unix_domain_socket_posix.h"
+
+// We define DCHECK internally instead of including base/logging.h
+// from Chromium, in hopes of avoiding a large dependency tree. The
+// goal is to make the nacl_helper binary small, as it is a new
+// binary in the Chrome linux distribution.

[Mark Seaborn, 2011/06/15 16:09:04]

'linux' -> 'Linux'

[Brad Chen, 2011/06/15 18:47:47]

Done.

+#if defined(NDEBUG)
+#define DCHECK(condition) {}
+#define DEBUG(s) do { if (0) { s; } } while (0)
+#else
+#define DCHECK(condition) \
+  { if (!condition) fprintf(stderr, "Check failed: %s.\n", #condition); }
+#define DEBUG(s) s
+#endif
+
+#if !defined(NDEBUG)
+class GURL;
+// This is another attempt to reduce binary size by pruning dependencies.
+// 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 this seems okay.
+namespace webkit_glue {
+
+bool FindProxyForUrl(const GURL& url, std::string* proxy_list) {

[jam, 2011/06/15 00:35:31]

btw this same approach is used with the nacl win64 dll, and it's proven to be a
pain.  i.e. someone adds some dependency to a file in common, which happens to
be included by nacl_win64.dll and suddenly a build fails. The developer then has
to figure out what's wrong, which isn't intuitive, and once they figure out they
add a stub.  we have 9 such files now (search for *_dummy.cc), and I would
really want us to not repeat this same issue on Linux as well.

+  fprintf(stderr, "Who is calling FindProxyForUrl?");
+  _exit(-1);
+  return false;
+}
+
+std::string GetProductVersion() {
+  fprintf(stderr, "Who is calling GetProductVersion?");
+  _exit(-1);
+  return "";
+}
+
+}  // namespace webkit_glue
+#endif  // NDEBUG
+
+static bool g_suid_sandbox_active;
+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.
+static void BecomeNaClLoader(const int zygote_fd,
+                             const std::vector<int>& child_fds) {
+  // Set up zygote descriptor as required by Chrome
+  DCHECK(zygote_fd == kNaClZygoteDescriptor);
+  close(zygote_fd);
+  int zfd = dup2(child_fds[kNaClBrowserFDIndex], kNaClZygoteDescriptor);
+  if (zfd != kNaClZygoteDescriptor) {
+    fprintf(stderr, "Could not initialize kNaClZygoteDescriptor (%d)\n", zfd);
+    _exit(-1);
+  }
+  // Set up sandbox descriptor as required by Chrome
+  close(kNaClSandboxDescriptor);
+  int sandbox_fd = dup2(child_fds[kNaClSandboxFDIndex], kNaClSandboxDescriptor);
+  if (sandbox_fd != kNaClSandboxDescriptor) {
+    fprintf(stderr, "Could not initialize kSandboxDescriptor (%d)\n",
+            sandbox_fd);
+    _exit(-1);
+  }
+
+  base::mac::ScopedNSAutoreleasePool autorelease_pool;
+  SandboxInitWrapper sandbox_wrapper;
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  const MainFunctionParams mainparams(command_line,
+                                      sandbox_wrapper,
+                                      &autorelease_pool);
+
+  DEBUG(fprintf(stderr, "calling NaClMain\n"));
+  NaClMain(mainparams);
+  _exit(0);
+}
+
+// Some of this code was lifted from
+// content/browser/zygote_main_linux.cc:ForkWithRealPid()
+static void HandleForkRequest(const int zygote_fd,
+                              const std::vector<int>& child_fds) {
+  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[kNaClParentFDIndex], buffer, 1));
+      if (nread != 1 || buffer[0] != 'x') {
+        perror("read");

[Mark Seaborn, 2011/06/15 16:09:04]

You're calling perror() when read() did not necessarily return an error.

[Brad Chen, 2011/06/15 18:47:47]

Done.

+        fprintf(stderr, "Failed to synch with zygote (%d:%d)\n",
+                nread, buffer[0]);
+      } else {
+        DEBUG(fprintf(stderr,
+                      "NaCl loader is synchronised with Chrome zygote\n"));
+      }
+    }
+    close(child_fds[kNaClDummyFDIndex]);
+    close(child_fds[kNaClParentFDIndex]);
+    BecomeNaClLoader(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]);
+  }
+  // Now tell childpid to the Chrome zygote.
+  if (HANDLE_EINTR(send(zygote_fd, &childpid, sizeof(childpid), MSG_EOR))
+      != sizeof(childpid)) {
+    fprintf(stderr, "*** send() to zygote failed");
+  }
+}
+
+int main(int argc, char *argv[]) {
+  CommandLine::Init(argc, argv);
+  base::AtExitManager exit_manager;
+  base::RandUint64();  // acquire /dev/urandom fd before sandbox is raised
+
+  g_suid_sandbox_active = (NULL != getenv("SBX_D"));
+
+  if (HANDLE_EINTR(send(kNaClZygoteDescriptor, kNaClHelperMagic,

[Mark Seaborn, 2011/06/15 16:09:04]

Please put a comment in about why you are sending a message here.  i.e. The
parent needs to wait for the execve() call to complete before it turns on the
SUID sandbox.

Can you rename kNaClHelperMagic to something more descriptive?  e.g.
kNaClHelperStartupAck.

[Brad Chen, 2011/06/15 18:47:47]

Done.

+                        sizeof(kNaClHelperMagic), MSG_EOR)) !=

[Mark Seaborn, 2011/06/15 16:09:04]

I've not seen MSG_EOR before.  What function is it performing here?

[Brad Chen, 2011/06/15 18:47:47]

It is to terminate the message for the UnixDomainSocket::RecvMsg on the other
side.

On 2011/06/15 16:09:04, Mark Seaborn wrote:

+      sizeof(kNaClHelperMagic)) {
+    perror("send");
+  }
+
+  while (true) {
+    int badpid = -1;
+    std::vector<int> fds;
+    static const unsigned kMaxMessageLength = 2048;
+    char buf[kMaxMessageLength];
+    const ssize_t msglen = UnixDomainSocket::RecvMsg(kNaClZygoteDescriptor,
+                                                     &buf, sizeof(buf), &fds);
+    if (0 == msglen || (-1 == msglen && ECONNRESET == errno)) {
+      // EOF from zygote. Rest in peace...

[Mark Seaborn, 2011/06/15 16:09:04]

Note that since this process is sharing an FD table with the zygote process,
this EOF is probably not going to happen.  This process will be holding both
ends of the socket pair.  This is not good because it would mean the NaCl helper
process will not go away when Chromium exits.  Can you check this?

[Brad Chen, 2011/06/15 18:47:47]

I've confirmed that the nacl_helper gets cleaned without this code, and have
removed it.

On 2011/06/15 16:09:04, Mark Seaborn wrote:

+      _exit(0);
+      return 0;
+    }
+    if (msglen == -1) {
+      fprintf(stderr, "NaCl helper: error reading message from zygote\n");
+      continue;
+    }
+    // fork request
+    if (msglen == sizeof(kNaClForkRequest) &&
+        memcmp(buf, kNaClForkRequest, msglen) == 0) {
+      if (kNaClSandboxFDIndex + 1 == fds.size() ||
+          kNaClParentFDIndex + 1 == fds.size()) {
+        HandleForkRequest(kNaClZygoteDescriptor, fds);
+        // child does not return
+      } else {
+        fprintf(stderr, "NaCl helper expected 2 or 4 fds, got %d\n",
+                int(fds.size()));
+      }
+    } else {
+      fprintf(stderr, "NaCl helper unrecognized request: %s\n", buf);
+    }
+    if (HANDLE_EINTR(send(kNaClZygoteDescriptor, &badpid,
+                          sizeof(badpid), MSG_EOR)) != sizeof(badpid)) {
+      perror("send");

[Mark Seaborn, 2011/06/15 16:09:04]

This is called in cases where send() didn't return an error.

[Brad Chen, 2011/06/15 18:47:47]

Done.

+    }
+  }
+}