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

content/zygote/zygote_linux.cc

Index: content/zygote/zygote_linux.cc
diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 08f1ecbb5d29dd537b5394ab0c6a55ec9046d57a..c3e09bda042f2a95228449108acc5b2ac37d3949 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,6 +311,13 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
   }
 
   if (use_helper) {
+    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);

[jln (very slow on Chromium), 2013/09/30 19:15:39]

I don't understand why this works. The size of fds is checked against a static
value in NaClForkDelegate::Fork() and we're increasing it here.

[Mark Seaborn, 2013/10/01 22:32:54]

I'm not increasing the number of FDs that is sent to nacl_helper in this change.
 It's 3 FDs before and after the change.

Before the change, 'fds' contains 1 FD when passed to Zygote::ForkWithRealPid(),
which adds 2 more.  After the change, Zygote::ForkWithRealPid() creates its own
'fds' vector and adds 3 FDs to it.

Do you think I should comment this as follows?

  fds.push_back(ipc_channel_fd);  // kNaClBrowserFDIndex
  fds.push_back(dummy_fd);        // kNaClDummyFDIndex
  fds.push_back(pipe_fds[0]);     // kNaClParentFDIndex

The problem with that is that this code path is nominally independent of NaCl
(even though it exists only for NaCl), and these comments "depend" on the
concept from nacl_helper_linux.h (by referring to it).

Maybe I should move kNaClBrowserFDIndex etc. to be defined in
content/public/common/zygote_fork_delegate_linux.h, and remove the "NaCl"
prefix?

[jln (very slow on Chromium), 2013/10/01 22:51:46]

Ohh yeah of course, sorry I missed that.
That could be a good additional change yeah. Feel free to add to this CL, or to
another one.
In general this is a very dark area, so cleanups and documentations are very
welcome!

[Mark Seaborn, 2013/10/07 15:23:13]

I've made that change.  Please take a look.

     fds.push_back(dummy_fd);
     fds.push_back(pipe_fds[0]);
     pid = helper_->Fork(fds);
@@ -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) {