Bug 1400061 - Stop using SetAllFDsToCloseOnExec when launching processes on OS X. r?billm draft
authorJed Davis <jld@mozilla.com>
Wed, 04 Oct 2017 19:39:54 -0600
changeset 678810 4dfad5ba8d4d71a310c15dd9ff23c756289e5ec2
parent 678809 8f43247a812ab96e70eb4c8ec7c9fdd068d62d67
child 678811 124aa8be45f8c3d7e764de9d01749a2e72597e7c
push id84057
push userbmo:jld@mozilla.com
push dateWed, 11 Oct 2017 23:00:24 +0000
reviewersbillm
bugs1400061
milestone58.0a1
Bug 1400061 - Stop using SetAllFDsToCloseOnExec when launching processes on OS X. r?billm As its original comments indicate, SetAllFDsToCloseOnExec has an unavoidable race condition if another thread creates file descriptors during launch. Instead, use POSIX_SPAWN_CLOEXEC_DEFAULT, which is an Apple-specific extension to posix_spawn that accomplished the desired effect atomically. This patch also introduces some RAII to simplify cleanup in error cases. MozReview-Commit-ID: 6oHggs77AiY
ipc/chromium/src/base/process_util_mac.mm
--- a/ipc/chromium/src/base/process_util_mac.mm
+++ b/ipc/chromium/src/base/process_util_mac.mm
@@ -8,16 +8,17 @@
 #include <fcntl.h>
 #include <spawn.h>
 #include <sys/wait.h>
 
 #include <string>
 
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
+#include "mozilla/ScopeExit.h"
 
 namespace {
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
@@ -37,42 +38,40 @@ bool LaunchApp(const std::vector<std::st
   bool retval = true;
 
   char* argv_copy[argv.size() + 1];
   for (size_t i = 0; i < argv.size(); i++) {
     argv_copy[i] = const_cast<char*>(argv[i].c_str());
   }
   argv_copy[argv.size()] = NULL;
 
-  // Make sure we don't leak any FDs to the child process by marking all FDs
-  // as close-on-exec.
-  SetAllFDsToCloseOnExec();
-
   EnvironmentArray vars = BuildEnvironmentArray(env_vars_to_set);
 
   posix_spawn_file_actions_t file_actions;
   if (posix_spawn_file_actions_init(&file_actions) != 0) {
     return false;
   }
+  auto file_actions_guard = mozilla::MakeScopeExit([&file_actions] {
+    posix_spawn_file_actions_destroy(&file_actions);
+  });
 
   // Turn fds_to_remap array into a set of dup2 calls.
   for (file_handle_mapping_vector::const_iterator it = fds_to_remap.begin();
        it != fds_to_remap.end();
        ++it) {
     int src_fd = it->first;
     int dest_fd = it->second;
 
     if (src_fd == dest_fd) {
       int flags = fcntl(src_fd, F_GETFD);
       if (flags != -1) {
         fcntl(src_fd, F_SETFD, flags & ~FD_CLOEXEC);
       }
     } else {
       if (posix_spawn_file_actions_adddup2(&file_actions, src_fd, dest_fd) != 0) {
-        posix_spawn_file_actions_destroy(&file_actions);
         return false;
       }
     }
   }
 
   // Set up the CPU preference array.
   cpu_type_t cpu_types[1];
   switch (arch) {
@@ -90,38 +89,50 @@ bool LaunchApp(const std::vector<std::st
       break;
   }
 
   // Initialize spawn attributes.
   posix_spawnattr_t spawnattr;
   if (posix_spawnattr_init(&spawnattr) != 0) {
     return false;
   }
+  auto spawnattr_guard = mozilla::MakeScopeExit([&spawnattr] {
+    posix_spawnattr_destroy(&spawnattr);
+  });
 
   // Set spawn attributes.
   size_t attr_count = 1;
   size_t attr_ocount = 0;
   if (posix_spawnattr_setbinpref_np(&spawnattr, attr_count, cpu_types, &attr_ocount) != 0 ||
       attr_ocount != attr_count) {
-    posix_spawnattr_destroy(&spawnattr);
+    return false;
+  }
+
+  // Prevent the child process from inheriting any file descriptors
+  // that aren't named in `file_actions`.  (This is an Apple-specific
+  // extension to posix_spawn.)
+  if (posix_spawnattr_setflags(&spawnattr, POSIX_SPAWN_CLOEXEC_DEFAULT) != 0) {
     return false;
   }
 
+  // Exempt std{in,out,err} from being closed by POSIX_SPAWN_CLOEXEC_DEFAULT.
+  for (int fd = 0; fd <= STDERR_FILENO; ++fd) {
+    if (posix_spawn_file_actions_addinherit_np(&file_actions, fd) != 0) {
+      return false;
+    }
+  }
+
   int pid = 0;
   int spawn_succeeded = (posix_spawnp(&pid,
                                       argv_copy[0],
                                       &file_actions,
                                       &spawnattr,
                                       argv_copy,
                                       vars.get()) == 0);
 
-  posix_spawn_file_actions_destroy(&file_actions);
-
-  posix_spawnattr_destroy(&spawnattr);
-
   bool process_handle_valid = pid > 0;
   if (!spawn_succeeded || !process_handle_valid) {
     retval = false;
   } else {
     gProcessLog.print("==> process %d launched child process %d\n",
                       GetCurrentProcId(), pid);
     if (wait)
       HANDLE_EINTR(waitpid(pid, 0, 0));