Change UnixDomainSocket::RecvMsg to return ScopedVector<base::ScopedFD>

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>
 #include <sys/wait.h>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/linux_util.h"
 #include "base/logging.h"
+#include "base/macros.h"
 #include "base/pickle.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/posix/global_descriptors.h"
 #include "base/posix/unix_domain_socket_linux.h"
 #include "base/process/kill.h"
 #include "content/common/child_process_sandbox_support_impl_linux.h"
 #include "content/common/sandbox_linux/sandbox_linux.h"
 #include "content/common/set_process_title.h"
 #include "content/common/zygote_commands_linux.h"
 #include "content/public/common/content_descriptors.h"
@@ skipping 87 matching lines @@
   }
   *process_info = it->second;
   return true;
 }
 
 bool Zygote::UsingSUIDSandbox() const {
   return sandbox_flags_ & kSandboxLinuxSUID;
 }
 
 bool Zygote::HandleRequestFromBrowser(int fd) {
-  std::vector<int> fds;
+  ScopedVector<base::ScopedFD> fds;
   char buf[kZygoteMaxMessageLength];
   const ssize_t len = UnixDomainSocket::RecvMsg(fd, buf, sizeof(buf), &fds);
 
   if (len == 0 || (len == -1 && errno == ECONNRESET)) {
     // EOF from the browser. We should die.
     _exit(0);
     return false;
   }
 
   if (len == -1) {
     PLOG(ERROR) << "Error reading message from browser";
     return false;
   }
 
   Pickle pickle(buf, len);
   PickleIterator iter(pickle);
 
   int kind;
   if (pickle.ReadInt(&iter, &kind)) {
     switch (kind) {
       case kZygoteCommandFork:
         // This function call can return multiple times, once per fork().
-        return HandleForkRequest(fd, pickle, iter, fds);
+        return HandleForkRequest(fd, pickle, iter, fds.Pass());
 
       case kZygoteCommandReap:
         if (!fds.empty())
           break;
         HandleReapRequest(fd, pickle, iter);
         return false;
       case kZygoteCommandGetTerminationStatus:
         if (!fds.empty())
           break;
         HandleGetTerminationStatus(fd, pickle, iter);
         return false;
       case kZygoteCommandGetSandboxStatus:
         HandleGetSandboxStatus(fd, pickle, iter);
         return false;
       default:
         NOTREACHED();
         break;
     }
   }
 
   LOG(WARNING) << "Error parsing message from browser";
-  for (std::vector<int>::const_iterator
-       i = fds.begin(); i != fds.end(); ++i)
-    close(*i);
   return false;
 }
 
 // TODO(jln): remove callers to this broken API. See crbug.com/274855.
 void Zygote::HandleReapRequest(int fd,
                                const Pickle& pickle,
                                PickleIterator iter) {
   base::ProcessId child;
 
   if (!pickle.ReadInt(&iter, &child)) {
@@ skipping 249 matching lines @@
     close(dummy_fd);
   if (pipe_fds[0] >= 0)
     close(pipe_fds[0]);
   if (pipe_fds[1] >= 0)
     close(pipe_fds[1]);
   return -1;
 }
 
 base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
                                         PickleIterator iter,
-                                        std::vector<int>& fds,
+                                        ScopedVector<base::ScopedFD> fds,
                                         std::string* uma_name,
                                         int* uma_sample,
                                         int* uma_boundary_value) {
   std::vector<std::string> args;
   int argc = 0;
   int numfds = 0;
   base::GlobalDescriptors::Mapping mapping;
   std::string process_type;
   std::string channel_id;
   const std::string channel_id_prefix = std::string("--")
@@ skipping 15 matching lines @@
 
   if (!pickle.ReadInt(&iter, &numfds))
     return -1;
   if (numfds != static_cast<int>(fds.size()))
     return -1;
 
   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(key, fds[i]->get()));
   }
 
   mapping.push_back(std::make_pair(
       static_cast<uint32_t>(kSandboxIPCChannel), GetSandboxFD()));
 
   // Returns twice, once per process.
   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.

[jln (very slow on Chromium), 2014/04/28 23:48:55]

This is not passed in fds I suppose? Do you mind adding a PCHECK(0 ==
IGNORE_EINTR()) as a tag-along fix? (Or feel free to defer to another CL).

[mdempsky, 2014/04/29 00:10:33]

Correct.  There's no ScopedFD that owns this socket to my knowledge, so we still
need to manually close it. =/
Done.

+
+    // Pass ownership of file descriptors from fds to GlobalDescriptors.
+    for (ScopedVector<base::ScopedFD>::iterator i = fds.begin(); i != fds.end();

[jln (very slow on Chromium), 2014/04/28 23:48:55]

Wouldn't it be cleaner to iterate on "mapping" instead?

[mdempsky, 2014/04/29 00:10:33]

Probably, but as discussed, |mapping| just has the bare file descriptors, and
there's no clean way to correlate the elements of |mapping| back to |fds|, so
this is the best option currently available I think. :(

+         ++i)
+      ignore_result((*i)->release());
     base::GlobalDescriptors::GetInstance()->Reset(mapping);
 
     // Reset the process-wide command line to our new command line.
     CommandLine::Reset();
     CommandLine::Init(0, NULL);
     CommandLine::ForCurrentProcess()->InitFromArgv(args);
 
     // Update the process title. The argv was already cached by the call to
     // SetProcessTitleFromCommandLine in ChromeMain, so we can pass NULL here
     // (we don't have the original argv at this point).
     SetProcessTitleFromCommandLine(NULL);
   } else if (child_pid < 0) {
     LOG(ERROR) << "Zygote could not fork: process_type " << process_type
         << " numfds " << numfds << " child_pid " << child_pid;
   }
   return child_pid;
 }
 
 bool Zygote::HandleForkRequest(int fd,
                                const Pickle& pickle,
                                PickleIterator iter,
-                               std::vector<int>& fds) {
+                               ScopedVector<base::ScopedFD> fds) {
   std::string uma_name;
   int uma_sample;
   int uma_boundary_value;
-  base::ProcessId child_pid = ReadArgsAndFork(pickle, iter, fds,
-                                              &uma_name, &uma_sample,
-                                              &uma_boundary_value);
+  base::ProcessId child_pid = ReadArgsAndFork(
+      pickle, iter, fds.Pass(), &uma_name, &uma_sample, &uma_boundary_value);
   if (child_pid == 0)
     return true;
-  for (std::vector<int>::const_iterator
-       i = fds.begin(); i != fds.end(); ++i)
-    close(*i);
   if (uma_name.empty()) {
     // There is no UMA report from this particular fork.
     // Use the initial UMA report if any, and clear that record for next time.
     // Note the swap method here is the efficient way to do this, since
     // we know uma_name is empty.
     uma_name.swap(initial_uma_name_);
     uma_sample = initial_uma_sample_;
     uma_boundary_value = initial_uma_boundary_value_;
   }
   // Must always send reply, as ZygoteHost blocks while waiting for it.
@@ skipping 15 matching lines @@
                                     PickleIterator iter) {
   if (HANDLE_EINTR(write(fd, &sandbox_flags_, sizeof(sandbox_flags_))) !=
                    sizeof(sandbox_flags_)) {
     PLOG(ERROR) << "write";
   }
 
   return false;
 }
 
 }  // namespace content