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

--HG--
extra : rebase_source : 972264a778b9361d1259851554b5b7ae8f3dcdc6

ipc/chromium/src/base/process_util.h

@@ -44,6 +44,8 @@
 #include "base/file_descriptor_shuffle.h"
 #endif
 
+#include "mozilla/UniquePtr.h"
+
 #if defined(OS_MACOSX)
 struct kinfo_proc;
 #endif
@@ -170,6 +172,18 @@ bool LaunchApp(const std::vector<std::string>& argv,
                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

ipc/chromium/src/base/process_util_bsd.cc

@@ -14,7 +14,6 @@
 
 #include <string>
 
-#include "nspr.h"
 #include "base/eintr_wrapper.h"
 
 namespace {
@@ -25,14 +24,6 @@ static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 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) {
@@ -68,39 +59,10 @@ bool LaunchApp(const std::vector<std::string>& argv,
   // 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;
   }
 
@@ -119,7 +81,6 @@ bool LaunchApp(const std::vector<std::string>& argv,
     } 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;
       }
     }
@@ -131,9 +92,7 @@ bool LaunchApp(const std::vector<std::string>& argv,
                                       &file_actions,
                                       NULL,
                                       argv_copy,
-                                      vars) == 0);
-
-  FreeEnvVarsArray(vars, varsLen);
+                                      vars.get()) == 0);
 
   posix_spawn_file_actions_destroy(&file_actions);
 

ipc/chromium/src/base/process_util_linux.cc

@@ -6,23 +6,16 @@
 
 #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.
@@ -38,87 +31,6 @@ static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 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) {
@@ -148,13 +60,7 @@ bool LaunchApp(const std::vector<std::string>& argv,
   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)
@@ -178,7 +84,7 @@ bool LaunchApp(const std::vector<std::string>& argv,
 
     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.)

ipc/chromium/src/base/process_util_mac.mm

@@ -5,8 +5,7 @@
 
 #include "base/process_util.h"
 
-#import <Cocoa/Cocoa.h>
-#include <crt_externs.h>
+#include <fcntl.h>
 #include <spawn.h>
 #include <sys/wait.h>
 
@@ -14,9 +13,6 @@
 
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
-#include "base/rand_util.h"
-#include "base/string_util.h"
-#include "base/time.h"
 
 namespace {
 
@@ -26,14 +22,6 @@ static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 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) {
@@ -69,37 +57,10 @@ bool LaunchApp(const std::vector<std::string>& argv,
   // 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;
   }
 
@@ -118,7 +79,6 @@ bool LaunchApp(const std::vector<std::string>& argv,
     } 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;
       }
     }
@@ -144,7 +104,6 @@ bool LaunchApp(const std::vector<std::string>& argv,
   // Initialize spawn attributes.
   posix_spawnattr_t spawnattr;
   if (posix_spawnattr_init(&spawnattr) != 0) {
-    FreeEnvVarsArray(vars, varsLen);
     return false;
   }
 
@@ -153,7 +112,6 @@ bool LaunchApp(const std::vector<std::string>& argv,
   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;
   }
@@ -164,9 +122,7 @@ bool LaunchApp(const std::vector<std::string>& argv,
                                       &file_actions,
                                       &spawnattr,
                                       argv_copy,
-                                      vars) == 0);
-
-  FreeEnvVarsArray(vars, varsLen);
+                                      vars.get()) == 0);
 
   posix_spawn_file_actions_destroy(&file_actions);
 

ipc/chromium/src/base/process_util_posix.cc

@@ -29,6 +29,9 @@
 #include "base/dir_reader_posix.h"
 
 #include "mozilla/UniquePtr.h"
+// For PR_DuplicateEnvironment:
+#include "prenv.h"
+#include "prmem.h"
 
 const int kMicrosecondsPerSecond = 1000000;
 
@@ -353,4 +356,43 @@ int ProcessMetrics::GetCPUUsage() {
   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