Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd. r?glandium draft
authorJed Davis <jld@mozilla.com>
Wed, 21 Mar 2018 11:18:21 -0700
changeset 818341 2bf5a083d1324361534974b880b360f3415001f3
parent 817669 e951f4ad123aa87d1d392c286db14cabb41a8560
push id116248
push userbmo:jld@mozilla.com
push dateFri, 13 Jul 2018 21:01:01 +0000
reviewersglandium
bugs1400051
milestone63.0a1
Bug 1400051 - IPC: use process_util_linux on BSD and remove process_util_bsd. r?glandium This change is mainly to avoid the use of SetAllFDsToCloseOnExec, which has an unavoidable race condition that can leak file descriptors into child processes. The "Linux" implementation of child process creation, now that B2G support has been removed, is essentially a generic Unix fork+dup2+execve which works on BSD (and is already used on Solaris). MozReview-Commit-ID: KUpP2RYIYNp
ipc/chromium/moz.build
ipc/chromium/src/base/process_util.h
ipc/chromium/src/base/process_util_bsd.cc
ipc/chromium/src/base/process_util_linux.cc
--- a/ipc/chromium/moz.build
+++ b/ipc/chromium/moz.build
@@ -90,26 +90,19 @@ if os_macosx:
         # This file cannot be built in unified mode because of the redefinition
         # of NoOp.
         'src/base/platform_thread_mac.mm',
     ]
 
 if os_bsd:
     SOURCES += [
         'src/base/atomicops_internals_x86_gcc.cc',
+        'src/base/process_util_linux.cc',
         'src/base/time_posix.cc',
     ]
-    if CONFIG['OS_ARCH'] == 'GNU_kFreeBSD':
-        SOURCES += [
-            'src/base/process_util_linux.cc'
-        ]
-    else:
-        SOURCES += [
-            'src/base/process_util_bsd.cc'
-        ]
 
 if os_linux:
     SOURCES += [
         'src/base/atomicops_internals_x86_gcc.cc',
         'src/base/process_util_linux.cc',
         'src/base/time_posix.cc',
     ]
     if CONFIG['OS_TARGET'] == 'Android':
--- a/ipc/chromium/src/base/process_util.h
+++ b/ipc/chromium/src/base/process_util.h
@@ -116,17 +116,17 @@ struct LaunchOptions {
 #if defined(OS_POSIX)
   environment_map env_map;
 
   // A mapping of (src fd -> dest fd) to propagate into the child
   // process.  All other fds will be closed, except std{in,out,err}.
   file_handle_mapping_vector fds_to_remap;
 #endif
 
-#if defined(OS_LINUX) || defined(OS_SOLARIS)
+#if defined(OS_LINUX)
   struct ForkDelegate {
     virtual ~ForkDelegate() { }
     virtual pid_t Fork() = 0;
   };
 
   // If non-null, the fork delegate will be called instead of fork().
   mozilla::UniquePtr<ForkDelegate> fork_delegate = nullptr;
 #endif
deleted file mode 100644
--- a/ipc/chromium/src/base/process_util_bsd.cc
+++ /dev/null
@@ -1,101 +0,0 @@
-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
-/* vim: set ts=8 sts=2 et sw=2 tw=80: */
-// Copyright (c) 2008 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.
-
-// derived from process_util_mac.cc
-
-#include "base/process_util.h"
-
-#include <fcntl.h>
-#include <spawn.h>
-#include <sys/wait.h>
-
-#include <string>
-
-#include "base/eintr_wrapper.h"
-
-namespace {
-
-static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
-
-}  // namespace
-
-namespace base {
-
-
-bool LaunchApp(const std::vector<std::string>& argv,
-               const LaunchOptions& options,
-               ProcessHandle* process_handle)
-{
-  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(options.env_map);
-
-  posix_spawn_file_actions_t file_actions;
-  if (posix_spawn_file_actions_init(&file_actions) != 0) {
-    return false;
-  }
-
-  // Turn fds_to_remap array into a set of dup2 calls.
-  for (const auto& fd_map : options.fds_to_remap) {
-    int src_fd = fd_map.first;
-    int dest_fd = fd_map.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;
-      }
-    }
-  }
-
-  pid_t pid = 0;
-  int spawn_succeeded = (posix_spawnp(&pid,
-                                      argv_copy[0],
-                                      &file_actions,
-                                      NULL,
-                                      argv_copy,
-                                      vars.get()) == 0);
-
-  posix_spawn_file_actions_destroy(&file_actions);
-
-  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 (options.wait)
-      HANDLE_EINTR(waitpid(pid, 0, 0));
-
-    if (process_handle)
-      *process_handle = pid;
-  }
-
-  return retval;
-}
-
-bool LaunchApp(const CommandLine& cl,
-               const LaunchOptions& options,
-               ProcessHandle* process_handle) {
-  return LaunchApp(cl.argv(), options, process_handle);
-}
-
-}  // namespace base
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -11,16 +11,19 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
 #include "mozilla/ipc/FileDescriptorShuffle.h"
 #include "mozilla/UniquePtr.h"
 
+// WARNING: despite the name, this file is also used on the BSDs and
+// Solaris (basically, Unixes that aren't Mac OS), not just Linux.
+
 namespace {
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
@@ -31,17 +34,22 @@ bool LaunchApp(const std::vector<std::st
   mozilla::UniquePtr<char*[]> argv_cstr(new char*[argv.size() + 1]);
 
   EnvironmentArray envp = BuildEnvironmentArray(options.env_map);
   mozilla::ipc::FileDescriptorShuffle shuffle;
   if (!shuffle.Init(options.fds_to_remap)) {
     return false;
   }
 
+#ifdef OS_LINUX
   pid_t pid = options.fork_delegate ? options.fork_delegate->Fork() : fork();
+#else
+  pid_t pid = fork();
+#endif
+
   if (pid < 0)
     return false;
 
   if (pid == 0) {
     // In the child:
     for (const auto& fds : shuffle.Dup2Sequence()) {
       if (HANDLE_EINTR(dup2(fds.first, fds.second)) != fds.second) {
         // This shouldn't happen, but check for it.  And see below