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

content/zygote/zygote_linux.cc

 // Copyright (c) 2012 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.
 
 #include "content/zygote/zygote_linux.h"
 
 #include <fcntl.h>
 #include <string.h>
 #include <sys/socket.h>
 #include <sys/types.h>
@@ skipping 23 matching lines @@
 // See http://code.google.com/p/chromium/wiki/LinuxZygote
 
 namespace content {
 
 namespace {
 
 // NOP function. See below where this handler is installed.
 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,
                ZygoteForkDelegate* helper)
     : sandbox_flags_(sandbox_flags),
       helper_(helper),
       initial_uma_sample_(0),
       initial_uma_boundary_value_(0) {
   if (helper_) {
     helper_->InitialUMA(&initial_uma_name_,
@@ skipping 213 matching lines @@
   Pickle write_pickle;
   write_pickle.WriteInt(static_cast<int>(status));
   write_pickle.WriteInt(exit_code);
   ssize_t written =
       HANDLE_EINTR(write(fd, write_pickle.data(), write_pickle.size()));
   if (written != static_cast<ssize_t>(write_pickle.size()))
     PLOG(ERROR) << "write";
 }
 
 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,
                             int* uma_boundary_value) {
   const bool use_helper = (helper_ && helper_->CanHelp(process_type,
                                                        uma_name,
                                                        uma_sample,
                                                        uma_boundary_value));
   int dummy_fd;
   ino_t dummy_inode;
   int pipe_fds[2] = { -1, -1 };
   base::ProcessId pid = 0;
 
   dummy_fd = socket(PF_UNIX, SOCK_DGRAM, 0);
   if (dummy_fd < 0) {
     LOG(ERROR) << "Failed to create dummy FD";
     goto error;
   }
   if (!base::FileDescriptorGetInode(&dummy_inode, dummy_fd)) {
     LOG(ERROR) << "Failed to get inode for dummy FD";
     goto error;
   }
   if (pipe(pipe_fds) != 0) {
     LOG(ERROR) << "Failed to create pipe";
     goto error;
   }
 
   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);
   } else {
     pid = fork();
   }
   if (pid < 0) {
     goto error;
   } else if (pid == 0) {
     // In the child process.
@@ skipping 124 matching lines @@
       return -1;
     args.push_back(arg);
     if (arg.compare(0, channel_id_prefix.length(), channel_id_prefix) == 0)
       channel_id = arg;
   }
 
   if (!pickle.ReadInt(&iter, &numfds))
     return -1;
   if (numfds != static_cast<int>(fds.size()))
     return -1;
 

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

Do you want to add a comment here explaining a bit what this is?

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

Something like "// Unmarshal FD mapping"?  This part seems fairly
self-explanatory, and I'm not changing it in this change, so not sure what you
want here.

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

The whole thing is generally very poorly documented. But you're right that the
code is obvious, |mapping| could be documented in zygote_linux.h ?

Your call, I didn't deal with this code in a while and I don't remember well
where the documentation is the most lacking.

   for (int i = 0; i < numfds; ++i) {
     base::GlobalDescriptors::Key key;
     if (!pickle.ReadUInt32(&iter, &key))
       return -1;
     mapping.push_back(std::make_pair(key, fds[i]));
   }
 
   mapping.push_back(std::make_pair(
       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) {
     // This is the child process.
 
     close(kZygoteSocketPairFd);  // Our socket from the browser.
     if (UsingSUIDSandbox())
       close(kZygoteIdFd);  // Another socket from the browser.
     base::GlobalDescriptors::GetInstance()->Reset(mapping);
 
@@ skipping 56 matching lines @@
                                     PickleIterator iter) {
   if (HANDLE_EINTR(write(fd, &sandbox_flags_, sizeof(sandbox_flags_))) !=
                    sizeof(sandbox_flags_)) {
     PLOG(ERROR) << "write";
   }
 
   return false;
 }
 
 }  // namespace content