NaCl: Clean up how FDs are passed to nacl_helper instances on Linux

child_process_launcher.cc constructs a mapping from FD numbers to FDs,
and normal Chromium child processes receive this as a mapping.
However, before this change, when zygote_linux.cc passed these FDs to
nacl_helper, it stripped the keys from the mapping and only passed the
values.

This meant that if child_process_launcher.cc were changed to add more
key+value pairs to the FD mapping, nacl_helper_linux.cc would need to
be updated to use or skip over them.  Also, the order in which
child_process_launcher.cc adds the FDs to the mapping would be
significant, but only to nacl_helper, which would be weird.  This
would be particularly awkward for FDs that are added to the mapping
conditionally.

We clean this up by explicitly taking the one FD that nacl_helper
needs from the FD mapping, rather than passing across all of the
mapping's values.

The aim of this cleanup is to simplify this change --
https://codereview.chromium.org/22911027/ -- which changes
child_process_launcher.cc to add an FD conditionally.

Also make the #defines of the FD numbers clearer by moving them to
zygote_fork_delegate_linux.h, since that's the component that's
responsible for them.

BUG=none
TEST=NaCl tests in browser_tests

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

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

6 files changed, 46 insertions, 26 deletions

diff --git a/chrome/nacl/nacl_helper_linux.cc b/chrome/nacl/nacl_helper_linux.cc
index 53d247d..88d44b1 100644
--- a/chrome/nacl/nacl_helper_linux.cc
+++ b/chrome/nacl/nacl_helper_linux.cc
@@ -30,6 +30,7 @@
 #include "base/rand_util.h"
 #include "components/nacl/loader/nacl_listener.h"
 #include "components/nacl/loader/nacl_sandbox_linux.h"
+#include "content/public/common/zygote_fork_delegate_linux.h"
 #include "crypto/nss_util.h"
 #include "ipc/ipc_descriptors.h"
 #include "ipc/ipc_switches.h"
@@ -56,8 +57,9 @@ void BecomeNaClLoader(const std::vector<int>& child_fds,
     LOG(ERROR) << "Could not initialize NaCl's second "
       << "layer sandbox (seccomp-bpf).";
   }
-  base::GlobalDescriptors::GetInstance()->Set(kPrimaryIPCChannel,
-                                              child_fds[kNaClBrowserFDIndex]);
+  base::GlobalDescriptors::GetInstance()->Set(
+      kPrimaryIPCChannel,
+      child_fds[content::ZygoteForkDelegate::kBrowserFDIndex]);
 
   base::MessageLoopForIO main_message_loop;
   NaClListener listener;
@@ -70,6 +72,8 @@ 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) {
+  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];
@@ -77,8 +81,7 @@ void ChildNaClLoaderInit(const std::vector<int>& child_fds,
   // 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 int nread = HANDLE_EINTR(read(child_fds[kNaClParentFDIndex], buffer,
-                                      kMaxReadSize));
+  const int nread = HANDLE_EINTR(read(parent_fd, buffer, kMaxReadSize));
   const std::string switch_prefix = std::string("--") +
       switches::kProcessChannelID + std::string("=");
   const size_t len = switch_prefix.length();
@@ -95,10 +98,10 @@ void ChildNaClLoaderInit(const std::vector<int>& child_fds,
       validack = true;
     }
   }
-  if (HANDLE_EINTR(close(child_fds[kNaClDummyFDIndex])) != 0)
-    LOG(ERROR) << "close(child_fds[kNaClDummyFDIndex]) failed";
-  if (HANDLE_EINTR(close(child_fds[kNaClParentFDIndex])) != 0)
-    LOG(ERROR) << "close(child_fds[kNaClParentFDIndex]) failed";
+  if (HANDLE_EINTR(close(dummy_fd)) != 0)
+    LOG(ERROR) << "close(dummy_fd) failed";
+  if (HANDLE_EINTR(close(parent_fd)) != 0)
+    LOG(ERROR) << "close(parent_fd) failed";
   if (validack) {
     BecomeNaClLoader(child_fds, system_info);
   } else {
@@ -113,7 +116,7 @@ void ChildNaClLoaderInit(const std::vector<int>& child_fds,
 bool HandleForkRequest(const std::vector<int>& child_fds,
                        const NaClLoaderSystemInfo& system_info,
                        Pickle* output_pickle) {
-  if (kNaClParentFDIndex + 1 != child_fds.size()) {
+  if (content::ZygoteForkDelegate::kNumPassedFDs != child_fds.size()) {
     LOG(ERROR) << "nacl_helper: unexpected number of fds, got "
         << child_fds.size();
     return false;
diff --git a/components/nacl/common/nacl_helper_linux.h b/components/nacl/common/nacl_helper_linux.h
index a9324b3..d55ba1f 100644
--- a/components/nacl/common/nacl_helper_linux.h
+++ b/components/nacl/common/nacl_helper_linux.h
@@ -33,15 +33,4 @@ enum NaClZygoteIPCCommand {
 // content/browser/zygote_main_linux.cc
 // kMagicSandboxIPCDescriptor.
 
-// A fork request from the Zygote to the helper includes an array
-// of three file descriptors. These constants are used as indicies
-// into the array.
-// Used to pass in the descriptor for talking to the Browser
-#define kNaClBrowserFDIndex 0
-// The next two are used in the protocol for discovering the
-// child processes real PID from within the SUID sandbox. See
-// http://code.google.com/p/chromium/wiki/LinuxZygote
-#define kNaClDummyFDIndex 1
-#define kNaClParentFDIndex 2
-
 #endif  // COMPONENTS_NACL_COMMON_NACL_HELPER_LINUX_H_
diff --git a/components/nacl/zygote/nacl_fork_delegate_linux.cc b/components/nacl/zygote/nacl_fork_delegate_linux.cc
index d94ad6b..2a7e007 100644
--- a/components/nacl/zygote/nacl_fork_delegate_linux.cc
+++ b/components/nacl/zygote/nacl_fork_delegate_linux.cc
@@ -258,7 +258,7 @@ bool NaClForkDelegate::CanHelp(const std::string& process_type,
 pid_t NaClForkDelegate::Fork(const std::vector<int>& fds) {
   VLOG(1) << "NaClForkDelegate::Fork";
 
-  DCHECK(fds.size() == kNaClParentFDIndex + 1);
+  DCHECK(fds.size() == kNumPassedFDs);
 
   // First, send a remote fork request.
   Pickle write_pickle;
diff --git a/content/public/common/zygote_fork_delegate_linux.h b/content/public/common/zygote_fork_delegate_linux.h
index a5ea9f2..87b4859 100644
--- a/content/public/common/zygote_fork_delegate_linux.h
+++ b/content/public/common/zygote_fork_delegate_linux.h
@@ -43,6 +43,18 @@ class ZygoteForkDelegate {
   virtual bool CanHelp(const std::string& process_type, std::string* uma_name,
                        int* uma_sample, int* uma_boundary_value) = 0;
 
+  // Indexes of FDs in the vector passed to Fork().
+  enum {
+    // Used to pass in the descriptor for talking to the Browser
+    kBrowserFDIndex,
+    // The next two are used in the protocol for discovering the
+    // child processes real PID from within the SUID sandbox. See
+    // http://code.google.com/p/chromium/wiki/LinuxZygote
+    kDummyFDIndex,
+    kParentFDIndex,
+    kNumPassedFDs  // Number of FDs in the vector passed to Fork().
+  };
+
   // Delegate forks, returning a -1 on failure. Outside the
   // suid sandbox, Fork() returns the Linux process ID.
   // This method is not aware of any potential pid namespaces, so it'll
diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 08f1ecb..cc509c1 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -41,6 +41,14 @@ namespace {
 void SIGCHLDHandler(int signal) {
 }
 
+int LookUpFd(const base::GlobalDescriptors::Mapping& fd_mapping, uint32_t key) {
+  for (size_t index = 0; index < fd_mapping.size(); ++index) {
+    if (fd_mapping[index].first == key)
+      return fd_mapping[index].second;
+  }
+  return -1;
+}
+
 }  // namespace
 
 Zygote::Zygote(int sandbox_flags,
@@ -274,7 +282,7 @@ void Zygote::HandleGetTerminationStatus(int fd,
 }
 
 int Zygote::ForkWithRealPid(const std::string& process_type,
-                            std::vector<int>& fds,
+                            const base::GlobalDescriptors::Mapping& fd_mapping,
                             const std::string& channel_switch,
                             std::string* uma_name,
                             int* uma_sample,
@@ -303,8 +311,15 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
   }
 
   if (use_helper) {
-    fds.push_back(dummy_fd);
-    fds.push_back(pipe_fds[0]);
+    std::vector<int> fds;
+    int ipc_channel_fd = LookUpFd(fd_mapping, kPrimaryIPCChannel);
+    if (ipc_channel_fd < 0) {
+      DLOG(ERROR) << "Failed to find kPrimaryIPCChannel in FD mapping";
+      goto error;
+    }
+    fds.push_back(ipc_channel_fd);  // kBrowserFDIndex
+    fds.push_back(dummy_fd);  // kDummyFDIndex
+    fds.push_back(pipe_fds[0]);  // kParentFDIndex
     pid = helper_->Fork(fds);
   } else {
     pid = fork();
@@ -459,7 +474,7 @@ base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
       static_cast<uint32_t>(kSandboxIPCChannel), GetSandboxFD()));
 
   // Returns twice, once per process.
-  base::ProcessId child_pid = ForkWithRealPid(process_type, fds, channel_id,
+  base::ProcessId child_pid = ForkWithRealPid(process_type, mapping, channel_id,
                                               uma_name, uma_sample,
                                               uma_boundary_value);
   if (!child_pid) {
diff --git a/content/zygote/zygote_linux.h b/content/zygote/zygote_linux.h
index ffea49a..37e89b5 100644
--- a/content/zygote/zygote_linux.h
+++ b/content/zygote/zygote_linux.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/containers/small_map.h"
+#include "base/posix/global_descriptors.h"
 #include "base/process/kill.h"
 #include "base/process/process.h"
 
@@ -77,7 +78,7 @@ class Zygote {
   // fills in uma_name et al with a report the helper wants to make via
   // UMA_HISTOGRAM_ENUMERATION.
   int ForkWithRealPid(const std::string& process_type,
-                      std::vector<int>& fds,
+                      const base::GlobalDescriptors::Mapping& fd_mapping,
                       const std::string& channel_switch,
                       std::string* uma_name,
                       int* uma_sample,