Bug 1133073 - Use PR_DuplicateEnvironment to avoid post-fork malloc on all Linux platforms. r=dhylands
☠☠ backed out by 18f5d9d83ab8 ☠ ☠
authorJed Davis <jld@mozilla.com>
Mon, 11 Jan 2016 12:46:50 -0800
changeset 279451 7f6bb9f7e60d7e1db8f3d12fbefadd7e9c1216e0
parent 279450 af2a9677e580efed867b8b5f2e38a845c4937f32
child 279452 67894aa872d7e67563a2ae4e97a78e35fa9e9b44
push id29882
push usercbook@mozilla.com
push dateTue, 12 Jan 2016 10:54:59 +0000
treeherdermozilla-central@e790bba372f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdhylands
bugs1133073
milestone46.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1133073 - Use PR_DuplicateEnvironment to avoid post-fork malloc on all Linux platforms. r=dhylands
config/external/nss/Makefile.in
configure.in
ipc/chromium/src/base/process_util_linux.cc
mozglue/build/BionicGlue.cpp
--- a/config/external/nss/Makefile.in
+++ b/config/external/nss/Makefile.in
@@ -207,21 +207,24 @@ DEFAULT_GMAKE_FLAGS += \
 DEFAULT_GMAKE_FLAGS += ARCHFLAG='$(filter-out -W%,$(CFLAGS)) -DCHECK_FORK_GETPID $(addprefix -DANDROID_VERSION=,$(ANDROID_VERSION)) -include $(topsrcdir)/security/manager/android_stub.h'
 endif
 endif
 
 ifdef WRAP_LDFLAGS
 NSS_EXTRA_LDFLAGS += $(WRAP_LDFLAGS)
 endif
 
-ifdef MOZ_GLUE_WRAP_LDFLAGS
+# The SHARED_LIBS part is needed unconditionally on Android.  It's not
+# clear why this is the case, but see bug 1133073 (starting around
+# comment #8) for context.
+ifneq (,$(or $(MOZ_GLUE_WRAP_LDFLAGS), $(filter Android, $(OS_TARGET))))
 NSS_EXTRA_LDFLAGS += $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) $(MOZ_GLUE_WRAP_LDFLAGS)
 endif
 
-ifneq (,$(WRAP_LDFLAGS)$(MOZ_GLUE_WRAP_LDFLAGS))
+ifneq (,$(NSS_EXTRA_LDFLAGS))
 DEFAULT_GMAKE_FLAGS += \
 	LDFLAGS='$(LDFLAGS) $(NSS_EXTRA_LDFLAGS)' \
 	DSO_LDOPTS='$(DSO_LDOPTS) $(LDFLAGS) $(NSS_EXTRA_LDFLAGS)' \
 	$(NULL)
 endif
 
 DEFAULT_GMAKE_FLAGS += FREEBL_NO_DEPEND=0
 ifeq ($(OS_TARGET),Linux)
--- a/configure.in
+++ b/configure.in
@@ -7266,17 +7266,16 @@ if test -f "${srcdir}/${MOZ_BUILD_APP}/c
      "${srcdir}/${MOZ_BUILD_APP}/configure.in" > $tmpscript
   . $tmpscript
   rm -f $tmpscript
 fi
 
 dnl We need to wrap dlopen and related functions on Android because we use
 dnl our own linker.
 if test "$OS_TARGET" = Android; then
-    MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=PR_GetEnv,--wrap=PR_SetEnv"
     if test "$MOZ_WIDGET_TOOLKIT" = gonk -a -n "$MOZ_NUWA_PROCESS"; then
         MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=pthread_create,--wrap=epoll_wait,--wrap=poll,--wrap=pthread_cond_timedwait,--wrap=pthread_cond_wait,--wrap=epoll_create,--wrap=epoll_ctl,--wrap=close,--wrap=pthread_key_create,--wrap=pthread_key_delete,--wrap=socketpair,--wrap=pthread_self,--wrap=pthread_mutex_lock,--wrap=pthread_mutex_trylock,--wrap=pthread_join,--wrap=pipe,--wrap=pipe2"
     fi
     if test "$MOZ_WIDGET_TOOLKIT" = android -a "$MOZ_ANDROID_MIN_SDK_VERSION" -lt 11; then
         MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=getaddrinfo,--wrap=freeaddrinfo,--wrap=gai_strerror"
     fi
 fi
 
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -17,16 +17,19 @@
 #include "base/eintr_wrapper.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "nsLiteralString.h"
 #include "mozilla/UniquePtr.h"
 
+#include "prenv.h"
+#include "prmem.h"
+
 #ifdef MOZ_B2G_LOADER
 #include "ProcessUtils.h"
 
 using namespace mozilla::ipc;
 #endif	// MOZ_B2G_LOADER
 
 #ifdef MOZ_WIDGET_GONK
 /*
@@ -42,99 +45,61 @@ using namespace mozilla::ipc;
  * On platforms that are not gonk based, 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
 #endif
 
-#ifdef ANDROID
-#include <pthread.h>
-/*
- * Currently, PR_DuplicateEnvironment is implemented in
- * mozglue/build/BionicGlue.cpp
- */
-#define HAVE_PR_DUPLICATE_ENVIRONMENT
-
-#include "plstr.h"
-#include "prenv.h"
-#include "prmem.h"
-/* Temporary until we have PR_DuplicateEnvironment in prenv.h */
-extern "C" { NSPR_API(pthread_mutex_t *)PR_GetEnvLock(void); }
-#endif
-
 namespace {
 
 enum ParsingState {
   KEY_NAME,
   KEY_VALUE
 };
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
-/* 
- * I tried to put PR_DuplicateEnvironment down in mozglue, but on android 
- * this winds up failing because the strdup/free calls wind up mismatching. 
- */
-
-static char **
-PR_DuplicateEnvironment(void)
-{
-    char **result = NULL;
-    char **s;
-    char **d;
-    pthread_mutex_lock(PR_GetEnvLock());
-    for (s = environ; *s != NULL; s++)
-      ;
-    if ((result = (char **)PR_Malloc(sizeof(char *) * (s - environ + 1))) != NULL) {
-      for (s = environ, d = result; *s != NULL; s++, d++) {
-        *d = PL_strdup(*s);
-      }
-      *d = NULL;
-    }
-    pthread_mutex_unlock(PR_GetEnvLock());
-    return result;
-}
-
 class EnvironmentEnvp
 {
 public:
   EnvironmentEnvp()
     : mEnvp(PR_DuplicateEnvironment()) {}
 
-  EnvironmentEnvp(const environment_map &em)
+  explicit EnvironmentEnvp(const environment_map &em)
   {
     mEnvp = (char **)PR_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;
-      *e = PL_strdup(str.c_str());
+      size_t len = str.length() + 1;
+      *e = static_cast<char*>(PR_Malloc(len));
+      memcpy(*e, str.c_str(), len);
     }
     *e = NULL;
   }
 
   ~EnvironmentEnvp()
   {
     if (!mEnvp) {
       return;
     }
     for (char **e = mEnvp; *e; ++e) {
-      PL_strfree(*e);
+      PR_Free(*e);
     }
     PR_Free(mEnvp);
   }
 
   char * const *AsEnvp() { return mEnvp; }
 
   void ToMap(environment_map &em)
   {
@@ -173,17 +138,16 @@ public:
   {
     for (const_iterator it = em.begin(); it != em.end(); ++it) {
       (*this)[it->first] = it->second;
     }
   }
 private:
   std::auto_ptr<EnvironmentEnvp> mEnvp;
 };
-#endif // HAVE_PR_DUPLICATE_ENVIRONMENT
 
 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);
 }
 
@@ -263,25 +227,23 @@ bool LaunchApp(const std::vector<std::st
 #endif // MOZ_B2G_LOADER
 
   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());
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
   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;
   }
-#endif
 
   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) {
@@ -295,27 +257,20 @@ 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);
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
     execve(argv_cstr[0], argv_cstr.get(), envp);
-#else
-    for (environment_map::const_iterator it = env_vars_to_set.begin();
-         it != env_vars_to_set.end(); ++it) {
-      if (setenv(it->first.c_str(), it->second.c_str(), 1/*overwrite*/))
-        _exit(127);
-    }
-    execv(argv_cstr[0], argv_cstr.get());
-#endif
     // 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);
     if (wait)
       HANDLE_EINTR(waitpid(pid, 0, 0));
 
--- a/mozglue/build/BionicGlue.cpp
+++ b/mozglue/build/BionicGlue.cpp
@@ -134,57 +134,16 @@ raise(int sig)
   // Bug 943170: POSIX specifies pthread_kill(pthread_self(), sig) as
   // equivalent to raise(sig), but Bionic also has a bug with these
   // functions, where a forked child will kill its parent instead.
 
   extern pid_t gettid(void);
   return syscall(__NR_tgkill, getpid(), gettid(), sig);
 }
 
-/*
- * The following wrappers for PR_Xxx are needed until we can get
- * PR_DuplicateEnvironment landed in NSPR.
- * See see bug 772734 and bug 773414.
- *
- * We can't #include the pr headers here, and we can't call any of the
- * PR/PL functions either, so we just reimplemnt using native code.
- */
-
-static pthread_mutex_t  _pr_envLock = PTHREAD_MUTEX_INITIALIZER;
-
-extern "C" NS_EXPORT char*
-__wrap_PR_GetEnv(const char *var)
-{
-    char *ev;
-
-    pthread_mutex_lock(&_pr_envLock);
-    ev = getenv(var);
-    pthread_mutex_unlock(&_pr_envLock);
-    return ev;
-}
-
-extern "C" NS_EXPORT int
-__wrap_PR_SetEnv(const char *string)
-{
-    int result;
-
-    if ( !strchr(string, '=')) return(-1);
-
-    pthread_mutex_lock(&_pr_envLock);
-    result = putenv(const_cast<char*>(string));
-    pthread_mutex_unlock(&_pr_envLock);
-    return (result)? -1 : 0;
-}
-
-extern "C" NS_EXPORT pthread_mutex_t *
-PR_GetEnvLock(void)
-{
-  return &_pr_envLock;
-}
-
 /* Flash plugin uses symbols that are not present in Android >= 4.4 */
 #ifndef MOZ_WIDGET_GONK
 namespace android {
   namespace VectorImpl {
     NS_EXPORT void reservedVectorImpl1(void) { }
     NS_EXPORT void reservedVectorImpl2(void) { }
     NS_EXPORT void reservedVectorImpl3(void) { }
     NS_EXPORT void reservedVectorImpl4(void) { }