Bug 1259852 - Merge Linux/BSD/Mac child process environment handling. r=billm f=jbeich
authorJed Davis <jld@mozilla.com>
Fri, 15 Sep 2017 11:18:43 -0600
changeset 679141 0567ed7f60742717d1377d830eb201cfa3045759
parent 679140 310eff6bb091017d6125610d2148926d4b8e2cd7
child 679142 87077c875c637b6a8266ad2905ee0fc45501db4f
push id84141
push userbmo:schien@mozilla.com
push dateThu, 12 Oct 2017 11:13:04 +0000
reviewersbillm
bugs1259852
milestone58.0a1
Bug 1259852 - Merge Linux/BSD/Mac child process environment handling. r=billm f=jbeich This is mostly based on the BSD version, which in turn is more or less the Mac version minus some race conditions. The Linux version does something similar, but more verbosely and (at least in my opinion) is harder to follow. Some changes have been made, mainly to use C++11 features like UniquePtr. MozReview-Commit-ID: 3Gv4DKCqWvu
ipc/chromium/src/base/process_util.h
ipc/chromium/src/base/process_util_bsd.cc
ipc/chromium/src/base/process_util_linux.cc
ipc/chromium/src/base/process_util_mac.mm
ipc/chromium/src/base/process_util_posix.cc
--- a/ipc/chromium/src/base/process_util.h
+++ b/ipc/chromium/src/base/process_util.h
@@ -39,16 +39,18 @@
 #include "base/child_privileges.h"
 #include "base/command_line.h"
 #include "base/process.h"
 
 #if defined(OS_POSIX)
 #include "base/file_descriptor_shuffle.h"
 #endif
 
+#include "mozilla/UniquePtr.h"
+
 #if defined(OS_MACOSX)
 struct kinfo_proc;
 #endif
 
 namespace base {
 
 // These can be used in a 32-bit bitmask.
 enum ProcessArchitecture {
@@ -165,16 +167,28 @@ bool LaunchApp(const std::vector<std::st
                ChildPrivileges privs,
                bool wait, ProcessHandle* process_handle,
                ProcessArchitecture arch=GetCurrentProcessArchitecture());
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                const environment_map& env_vars_to_set,
                bool wait, ProcessHandle* process_handle,
                ProcessArchitecture arch=GetCurrentProcessArchitecture());
+
+// Deleter for the array of strings allocated within BuildEnvironmentArray.
+struct FreeEnvVarsArray
+{
+  void operator()(char** array);
+};
+
+typedef mozilla::UniquePtr<char*[], FreeEnvVarsArray> EnvironmentArray;
+
+// Merge an environment map with the current environment.
+// Existing variables are overwritten by env_vars_to_set.
+EnvironmentArray BuildEnvironmentArray(const environment_map& env_vars_to_set);
 #endif
 
 // Adjust the privileges of this process to match |privs|.  Only
 // returns if privileges were successfully adjusted.
 void SetCurrentProcessPrivileges(ChildPrivileges privs);
 
 // Executes the application specified by cl. This function delegates to one
 // of the above two platform-specific functions.
--- a/ipc/chromium/src/base/process_util_bsd.cc
+++ b/ipc/chromium/src/base/process_util_bsd.cc
@@ -9,35 +9,26 @@
 #include "base/process_util.h"
 
 #include <fcntl.h>
 #include <spawn.h>
 #include <sys/wait.h>
 
 #include <string>
 
-#include "nspr.h"
 #include "base/eintr_wrapper.h"
 
 namespace {
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
-void FreeEnvVarsArray(char* array[], int length)
-{
-  for (int i = 0; i < length; i++) {
-    free(array[i]);
-  }
-  delete[] array;
-}
-
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   return LaunchApp(argv, fds_to_remap, environment_map(),
                    wait, process_handle);
 }
 
 bool LaunchApp(const std::vector<std::string>& argv,
@@ -63,49 +54,20 @@ bool LaunchApp(const std::vector<std::st
     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();
 
-  // Copy environment to a new char array and add the variables
-  // in env_vars_to_set.
-  // Existing variables are overwritten by env_vars_to_set.
-  int pos = 0;
-  environment_map combined_env_vars = env_vars_to_set;
-  char **environ = PR_DuplicateEnvironment();
-  while(environ[pos] != NULL) {
-    std::string varString = environ[pos];
-    std::string varName = varString.substr(0, varString.find_first_of('='));
-    std::string varValue = varString.substr(varString.find_first_of('=') + 1);
-    if (combined_env_vars.find(varName) == combined_env_vars.end()) {
-      combined_env_vars[varName] = varValue;
-    }
-    PR_Free(environ[pos++]); // PR_DuplicateEnvironment() uses PR_Malloc().
-  }
-  PR_Free(environ); // PR_DuplicateEnvironment() uses PR_Malloc().
-  int varsLen = combined_env_vars.size() + 1;
-
-  char** vars = new char*[varsLen];
-  int i = 0;
-  for (environment_map::const_iterator it = combined_env_vars.begin();
-       it != combined_env_vars.end(); ++it) {
-    std::string entry(it->first);
-    entry += "=";
-    entry += it->second;
-    vars[i] = strdup(entry.c_str());
-    i++;
-  }
-  vars[i] = NULL;
+  EnvironmentArray vars = BuildEnvironmentArray(env_vars_to_set);
 
   posix_spawn_file_actions_t file_actions;
   if (posix_spawn_file_actions_init(&file_actions) != 0) {
-    FreeEnvVarsArray(vars, varsLen);
     return false;
   }
 
   // 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;
@@ -114,31 +76,28 @@ bool LaunchApp(const std::vector<std::st
     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);
-        FreeEnvVarsArray(vars, varsLen);
         return false;
       }
     }
   }
 
   pid_t pid = 0;
   int spawn_succeeded = (posix_spawnp(&pid,
                                       argv_copy[0],
                                       &file_actions,
                                       NULL,
                                       argv_copy,
-                                      vars) == 0);
-
-  FreeEnvVarsArray(vars, varsLen);
+                                      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",
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -1,129 +1,41 @@
 /* -*- 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.
 
 #include "base/process_util.h"
 
-#include <ctype.h>
-#include <fcntl.h>
-#include <memory>
-#include <unistd.h>
 #include <string>
 #include <sys/types.h>
 #include <sys/wait.h>
+#include <unistd.h>
 
 #include "base/eintr_wrapper.h"
-#include "base/file_util.h"
 #include "base/logging.h"
-#include "base/string_util.h"
-#include "nsLiteralString.h"
+#include "mozilla/Move.h"
 #include "mozilla/UniquePtr.h"
 
-#include "prenv.h"
-
 /*
  * We fall back to an arbitrary UID. This is generally the UID for user
  * `nobody', albeit it is not always the case.
  */
 # define CHILD_UNPRIVILEGED_UID 65534
 # define CHILD_UNPRIVILEGED_GID 65534
 
 namespace {
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
-class EnvironmentEnvp
-{
-public:
-  EnvironmentEnvp()
-    : mEnvp(PR_DuplicateEnvironment()) {}
-
-  explicit EnvironmentEnvp(const environment_map &em)
-  {
-    mEnvp = (char**) malloc(sizeof(char *) * (em.size() + 1));
-    if (!mEnvp) {
-      return;
-    }
-    char **e = mEnvp;
-    for (environment_map::const_iterator it = em.begin();
-         it != em.end(); ++it, ++e) {
-      std::string str = it->first;
-      str += "=";
-      str += it->second;
-      size_t len = str.length() + 1;
-      *e = static_cast<char*>(malloc(len));
-      memcpy(*e, str.c_str(), len);
-    }
-    *e = NULL;
-  }
-
-  ~EnvironmentEnvp()
-  {
-    if (!mEnvp) {
-      return;
-    }
-    for (char **e = mEnvp; *e; ++e) {
-      free(*e);
-    }
-    free(mEnvp);
-  }
-
-  char * const *AsEnvp() { return mEnvp; }
-
-  void ToMap(environment_map &em)
-  {
-    if (!mEnvp) {
-      return;
-    }
-    em.clear();
-    for (char **e = mEnvp; *e; ++e) {
-      const char *eq;
-      if ((eq = strchr(*e, '=')) != NULL) {
-        std::string varname(*e, eq - *e);
-        em[varname.c_str()] = &eq[1];
-      }
-    }
-  }
-
-private:
-  char **mEnvp;
-};
-
-class Environment : public environment_map
-{
-public:
-  Environment()
-  {
-    EnvironmentEnvp envp;
-    envp.ToMap(*this);
-  }
-
-  char * const *AsEnvp() {
-    mEnvp.reset(new EnvironmentEnvp(*this));
-    return mEnvp->AsEnvp();
-  }
-
-  void Merge(const environment_map &em)
-  {
-    for (const_iterator it = em.begin(); it != em.end(); ++it) {
-      (*this)[it->first] = it->second;
-    }
-  }
-private:
-  std::auto_ptr<EnvironmentEnvp> mEnvp;
-};
-
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   return LaunchApp(argv, fds_to_remap, environment_map(),
                    wait, process_handle);
 }
 
 bool LaunchApp(const std::vector<std::string>& argv,
@@ -143,23 +55,17 @@ bool LaunchApp(const std::vector<std::st
                bool wait, ProcessHandle* process_handle,
                ProcessArchitecture arch) {
   mozilla::UniquePtr<char*[]> argv_cstr(new char*[argv.size() + 1]);
   // Illegal to allocate memory after fork and before execvp
   InjectiveMultimap fd_shuffle1, fd_shuffle2;
   fd_shuffle1.reserve(fds_to_remap.size());
   fd_shuffle2.reserve(fds_to_remap.size());
 
-  Environment env;
-  env.Merge(env_vars_to_set);
-  char * const *envp = env.AsEnvp();
-  if (!envp) {
-    DLOG(ERROR) << "FAILED to duplicate environment for: " << argv_cstr[0];
-    return false;
-  }
+  EnvironmentArray envp = BuildEnvironmentArray(env_vars_to_set);
 
   pid_t pid = fork();
   if (pid < 0)
     return false;
 
   if (pid == 0) {
     for (file_handle_mapping_vector::const_iterator
         it = fds_to_remap.begin(); it != fds_to_remap.end(); ++it) {
@@ -173,17 +79,17 @@ bool LaunchApp(const std::vector<std::st
     CloseSuperfluousFds(fd_shuffle2);
 
     for (size_t i = 0; i < argv.size(); i++)
       argv_cstr[i] = const_cast<char*>(argv[i].c_str());
     argv_cstr[argv.size()] = NULL;
 
     SetCurrentProcessPrivileges(privs);
 
-    execve(argv_cstr[0], argv_cstr.get(), envp);
+    execve(argv_cstr[0], argv_cstr.get(), envp.get());
     // if we get here, we're in serious trouble and should complain loudly
     // NOTE: This is async signal unsafe; it could deadlock instead.  (But
     // only on debug builds; otherwise it's a signal-safe no-op.)
     DLOG(ERROR) << "FAILED TO exec() CHILD PROCESS, path: " << argv_cstr[0];
     _exit(127);
   } else {
     gProcessLog.print("==> process %d launched child process %d\n",
                       GetCurrentProcId(), pid);
--- a/ipc/chromium/src/base/process_util_mac.mm
+++ b/ipc/chromium/src/base/process_util_mac.mm
@@ -1,44 +1,32 @@
 // 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.
 
 
 #include "base/process_util.h"
 
-#import <Cocoa/Cocoa.h>
-#include <crt_externs.h>
+#include <fcntl.h>
 #include <spawn.h>
 #include <sys/wait.h>
 
 #include <string>
 
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
-#include "base/rand_util.h"
-#include "base/string_util.h"
-#include "base/time.h"
 
 namespace {
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
-void FreeEnvVarsArray(char* array[], int length)
-{
-  for (int i = 0; i < length; i++) {
-    free(array[i]);
-  }
-  delete[] array;
-}
-
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   return LaunchApp(argv, fds_to_remap, environment_map(),
                    wait, process_handle);
 }
 
 bool LaunchApp(const std::vector<std::string>& argv,
@@ -64,47 +52,20 @@ bool LaunchApp(const std::vector<std::st
     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();
 
-  // Copy _NSGetEnviron() to a new char array and add the variables
-  // in env_vars_to_set.
-  // Existing variables are overwritten by env_vars_to_set.
-  int pos = 0;
-  environment_map combined_env_vars = env_vars_to_set;
-  while((*_NSGetEnviron())[pos] != NULL) {
-    std::string varString = (*_NSGetEnviron())[pos];
-    std::string varName = varString.substr(0, varString.find_first_of('='));
-    std::string varValue = varString.substr(varString.find_first_of('=') + 1);
-    if (combined_env_vars.find(varName) == combined_env_vars.end()) {
-      combined_env_vars[varName] = varValue;
-    }
-    pos++;
-  }
-  int varsLen = combined_env_vars.size() + 1;
-
-  char** vars = new char*[varsLen];
-  int i = 0;
-  for (environment_map::const_iterator it = combined_env_vars.begin();
-       it != combined_env_vars.end(); ++it) {
-    std::string entry(it->first);
-    entry += "=";
-    entry += it->second;
-    vars[i] = strdup(entry.c_str());
-    i++;
-  }
-  vars[i] = NULL;
+  EnvironmentArray vars = BuildEnvironmentArray(env_vars_to_set);
 
   posix_spawn_file_actions_t file_actions;
   if (posix_spawn_file_actions_init(&file_actions) != 0) {
-    FreeEnvVarsArray(vars, varsLen);
     return false;
   }
 
   // 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;
@@ -113,17 +74,16 @@ bool LaunchApp(const std::vector<std::st
     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);
-        FreeEnvVarsArray(vars, varsLen);
         return false;
       }
     }
   }
 
   // Set up the CPU preference array.
   cpu_type_t cpu_types[1];
   switch (arch) {
@@ -139,39 +99,35 @@ bool LaunchApp(const std::vector<std::st
     default:
       cpu_types[0] = CPU_TYPE_ANY;
       break;
   }
 
   // Initialize spawn attributes.
   posix_spawnattr_t spawnattr;
   if (posix_spawnattr_init(&spawnattr) != 0) {
-    FreeEnvVarsArray(vars, varsLen);
     return false;
   }
 
   // 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) {
-    FreeEnvVarsArray(vars, varsLen);
     posix_spawnattr_destroy(&spawnattr);
     return false;
   }
 
   int pid = 0;
   int spawn_succeeded = (posix_spawnp(&pid,
                                       argv_copy[0],
                                       &file_actions,
                                       &spawnattr,
                                       argv_copy,
-                                      vars) == 0);
-
-  FreeEnvVarsArray(vars, varsLen);
+                                      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;
--- a/ipc/chromium/src/base/process_util_posix.cc
+++ b/ipc/chromium/src/base/process_util_posix.cc
@@ -24,16 +24,19 @@
 #include "base/platform_thread.h"
 #include "base/process_util.h"
 #include "base/sys_info.h"
 #include "base/time.h"
 #include "base/waitable_event.h"
 #include "base/dir_reader_posix.h"
 
 #include "mozilla/UniquePtr.h"
+// For PR_DuplicateEnvironment:
+#include "prenv.h"
+#include "prmem.h"
 
 const int kMicrosecondsPerSecond = 1000000;
 
 namespace base {
 
 ProcessId GetCurrentProcId() {
   return getpid();
 }
@@ -348,9 +351,48 @@ int ProcessMetrics::GetCPUUsage() {
                              time_delta);
 
   last_system_time_ = system_time;
   last_time_ = time;
 
   return cpu;
 }
 
+void
+FreeEnvVarsArray::operator()(char** array)
+{
+  for (char** varPtr = array; *varPtr != nullptr; ++varPtr) {
+    free(*varPtr);
+  }
+  delete[] array;
+}
+
+EnvironmentArray
+BuildEnvironmentArray(const environment_map& env_vars_to_set)
+{
+  base::environment_map combined_env_vars = env_vars_to_set;
+  char **environ = PR_DuplicateEnvironment();
+  for (char** varPtr = environ; *varPtr != nullptr; ++varPtr) {
+    std::string varString = *varPtr;
+    size_t equalPos = varString.find_first_of('=');
+    std::string varName = varString.substr(0, equalPos);
+    std::string varValue = varString.substr(equalPos + 1);
+    if (combined_env_vars.find(varName) == combined_env_vars.end()) {
+      combined_env_vars[varName] = varValue;
+    }
+    PR_Free(*varPtr); // PR_DuplicateEnvironment() uses PR_Malloc().
+  }
+  PR_Free(environ); // PR_DuplicateEnvironment() uses PR_Malloc().
+
+  EnvironmentArray array(new char*[combined_env_vars.size() + 1]);
+  size_t i = 0;
+  for (const auto& key_val : combined_env_vars) {
+    std::string entry(key_val.first);
+    entry += "=";
+    entry += key_val.second;
+    array[i] = strdup(entry.c_str());
+    i++;
+  }
+  array[i] = nullptr;
+  return array;
+}
+
 }  // namespace base