Simplify ZygoteForkDelegate API further

This patch makes three changes:

  1. Removes the AckChild() delegate method used to send a custom
     message to the child process.
  2. Instead, the parent always writes the child's PID (as seen by the
     browser) over the pipe.  (Exception: When writing to a NaCl child
     process, we instead send 0 to avoid leaking the real PID into the
     NaCl address space.)
  3. Makes the Fork() delegate method responsible for sending the IPC
     channel ID to the child process.

This is in preparation for the next patch which will switch the pipe
direction to make the child responsible for discovering its own PID
and sending it to the parent process.  By removing AckChild(), this
simplifies the protocol and makes this change easier to implement.

BUG=357670

Review URL: https://codereview.chromium.org/240673002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@264764 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 43 insertions, 42 deletions

diff --git a/components/nacl/loader/nacl_helper_linux.cc b/components/nacl/loader/nacl_helper_linux.cc
index 70de656..15f58eb 100644
--- a/components/nacl/loader/nacl_helper_linux.cc
+++ b/components/nacl/loader/nacl_helper_linux.cc
@@ -28,6 +28,7 @@
 #include "base/posix/global_descriptors.h"
 #include "base/posix/unix_domain_socket_linux.h"
 #include "base/process/kill.h"
+#include "base/process/process_handle.h"
 #include "base/rand_util.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_listener.h"
@@ -139,27 +140,34 @@ void BecomeNaClLoader(const std::vector<int>& child_fds,
 // Start the NaCl loader in a child created by the NaCl loader Zygote.
 void ChildNaClLoaderInit(const std::vector<int>& child_fds,
                          const NaClLoaderSystemInfo& system_info,
-                         bool uses_nonsfi_mode) {
+                         bool uses_nonsfi_mode,
+                         const std::string& channel_id) {
   const int parent_fd = child_fds[content::ZygoteForkDelegate::kParentFDIndex];
   const int dummy_fd = child_fds[content::ZygoteForkDelegate::kDummyFDIndex];
+
   bool validack = false;
-  const size_t kMaxReadSize = 1024;
-  char buffer[kMaxReadSize];
+  base::ProcessId real_pid;
   // 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.
-  const ssize_t nread = HANDLE_EINTR(read(parent_fd, buffer, kMaxReadSize));
+  const ssize_t nread =
+      HANDLE_EINTR(read(parent_fd, &real_pid, sizeof(real_pid)));
+  if (static_cast<size_t>(nread) == sizeof(real_pid)) {
+    // Make sure the parent didn't accidentally send us our real PID.
+    // We don't want it to be discoverable anywhere in our address space
+    // when we start running untrusted code.
+    CHECK(real_pid == 0);
 
-  if (nread < 0) {
-    perror("read");
-    LOG(ERROR) << "read returned " << nread;
-  } else if (nread > 0) {
-    VLOG(1) << "NaCl loader is synchronised with Chrome zygote";
     CommandLine::ForCurrentProcess()->AppendSwitchASCII(
-        switches::kProcessChannelID, std::string(buffer, nread));
+        switches::kProcessChannelID, channel_id);
     validack = true;
+  } else {
+    if (nread < 0)
+      perror("read");
+    LOG(ERROR) << "read returned " << nread;
   }
+
   if (IGNORE_EINTR(close(dummy_fd)) != 0)
     LOG(ERROR) << "close(dummy_fd) failed";
   if (IGNORE_EINTR(close(parent_fd)) != 0)
@@ -185,6 +193,12 @@ bool HandleForkRequest(const std::vector<int>& child_fds,
     return false;
   }
 
+  std::string channel_id;
+  if (!input_iter->ReadString(&channel_id)) {
+    LOG(ERROR) << "Could not read channel_id string";
+    return false;
+  }
+
   if (content::ZygoteForkDelegate::kNumPassedFDs != child_fds.size()) {
     LOG(ERROR) << "nacl_helper: unexpected number of fds, got "
         << child_fds.size();
@@ -198,7 +212,7 @@ bool HandleForkRequest(const std::vector<int>& child_fds,
   }
 
   if (child_pid == 0) {
-    ChildNaClLoaderInit(child_fds, system_info, uses_nonsfi_mode);
+    ChildNaClLoaderInit(child_fds, system_info, uses_nonsfi_mode, channel_id);
     NOTREACHED();
   }
 
diff --git a/components/nacl/zygote/nacl_fork_delegate_linux.cc b/components/nacl/zygote/nacl_fork_delegate_linux.cc
index 558344d..e6bb77e9 100644
--- a/components/nacl/zygote/nacl_fork_delegate_linux.cc
+++ b/components/nacl/zygote/nacl_fork_delegate_linux.cc
@@ -288,7 +288,8 @@ bool NaClForkDelegate::CanHelp(const std::string& process_type,
 }
 
 pid_t NaClForkDelegate::Fork(const std::string& process_type,
-                             const std::vector<int>& fds) {
+                             const std::vector<int>& fds,
+                             const std::string& channel_id) {
   VLOG(1) << "NaClForkDelegate::Fork";
 
   DCHECK(fds.size() == kNumPassedFDs);
@@ -306,6 +307,7 @@ pid_t NaClForkDelegate::Fork(const std::string& process_type,
   const bool uses_nonsfi_mode =
     process_type == switches::kNaClLoaderNonSfiProcess;
   write_pickle.WriteBool(uses_nonsfi_mode);
+  write_pickle.WriteString(channel_id);
 
   char reply_buf[kNaClMaxIPCMessageLength];
   ssize_t reply_size = 0;
@@ -329,15 +331,6 @@ pid_t NaClForkDelegate::Fork(const std::string& process_type,
   return nacl_child;
 }
 
-bool NaClForkDelegate::AckChild(const int fd, const std::string& channel_id) {
-  ssize_t nwritten =
-      HANDLE_EINTR(write(fd, channel_id.c_str(), channel_id.length()));
-  if (static_cast<size_t>(nwritten) != channel_id.length()) {
-    return false;
-  }
-  return true;
-}
-
 bool NaClForkDelegate::GetTerminationStatus(pid_t pid, bool known_dead,
                                             base::TerminationStatus* status,
                                             int* exit_code) {
diff --git a/components/nacl/zygote/nacl_fork_delegate_linux.h b/components/nacl/zygote/nacl_fork_delegate_linux.h
index a0860b69..4cc105c 100644
--- a/components/nacl/zygote/nacl_fork_delegate_linux.h
+++ b/components/nacl/zygote/nacl_fork_delegate_linux.h
@@ -29,8 +29,8 @@ class NaClForkDelegate : public content::ZygoteForkDelegate {
   virtual bool CanHelp(const std::string& process_type, std::string* uma_name,
                           int* uma_sample, int* uma_boundary_value) OVERRIDE;
   virtual pid_t Fork(const std::string& process_type,
-                     const std::vector<int>& fds) OVERRIDE;
-  virtual bool AckChild(int fd, const std::string& channel_id) OVERRIDE;
+                     const std::vector<int>& fds,
+                     const std::string& channel_id) OVERRIDE;
   virtual bool GetTerminationStatus(pid_t pid, bool known_dead,
                                     base::TerminationStatus* status,
                                     int* exit_code) OVERRIDE;
diff --git a/content/public/common/zygote_fork_delegate_linux.h b/content/public/common/zygote_fork_delegate_linux.h
index 999721c..39833c5 100644
--- a/content/public/common/zygote_fork_delegate_linux.h
+++ b/content/public/common/zygote_fork_delegate_linux.h
@@ -61,13 +61,11 @@ class ZygoteForkDelegate {
   // suid sandbox, Fork() returns the Linux process ID.
   // This method is not aware of any potential pid namespaces, so it'll
   // return a raw pid just like fork() would.
+  // Delegate is responsible for communicating the channel ID to the
+  // newly created child process.
   virtual pid_t Fork(const std::string& process_type,
-                     const std::vector<int>& fds) = 0;
-
-  // After a successful fork, signal the child to indicate that
-  // the child's PID has been received. Also communicate the
-  // channel ID as a part of acknowledgement message.
-  virtual bool AckChild(int fd, const std::string& channel_id) = 0;
+                     const std::vector<int>& fds,
+                     const std::string& channel_id) = 0;
 
   // The fork delegate must also assume the role of waiting for its children
   // since the caller will not be their parents and cannot do it. |pid| here
diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 04e9d83..63c472f 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -333,7 +333,7 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
     fds.push_back(ipc_channel_fd);  // kBrowserFDIndex
     fds.push_back(dummy_fd);  // kDummyFDIndex
     fds.push_back(pipe_fds[0]);  // kParentFDIndex
-    pid = helper_->Fork(process_type, fds);
+    pid = helper_->Fork(process_type, fds, channel_id);
   } else {
     pid = fork();
   }
@@ -410,18 +410,14 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
     process_info_map_[real_pid].internal_pid = pid;
     process_info_map_[real_pid].started_from_helper = use_helper;
 
-    if (use_helper) {
-      if (!helper_->AckChild(pipe_fds[1], channel_id)) {
-        LOG(ERROR) << "Failed to synchronise with zygote fork helper";
-        goto error;
-      }
-    } else {
-      int written =
-          HANDLE_EINTR(write(pipe_fds[1], &real_pid, sizeof(real_pid)));
-      if (written != sizeof(real_pid)) {
-        LOG(ERROR) << "Failed to synchronise with child process";
-        goto error;
-      }
+    // If we're using a helper, we still need to let the child process know
+    // we've discovered its real PID, but we don't actually reveal the PID.
+    const base::ProcessId pid_for_child = use_helper ? 0 : real_pid;
+    ssize_t written =
+        HANDLE_EINTR(write(pipe_fds[1], &pid_for_child, sizeof(pid_for_child)));
+    if (written != sizeof(pid_for_child)) {
+      LOG(ERROR) << "Failed to synchronise with child process";
+      goto error;
     }
     close(pipe_fds[1]);
     return real_pid;