Bug 1133073 - Use PR_DuplicateEnvironment to avoid post-fork malloc on all Linux platforms. r=dhylands
authorJed Davis <jld@mozilla.com>
Mon, 11 Jan 2016 14:17:01 -0800
changeset 279652 f75b461c2c3c420f51597415721e71f2adef19eb
parent 279651 214b31421207ed37a113659dad2fb66a7a31ebea
child 279653 0d4f53b5744d74accdb9a858c489d4292483a70d
push id70170
push userjedavis@mozilla.com
push dateTue, 12 Jan 2016 23:39:59 +0000
treeherdermozilla-inbound@f75b461c2c3c [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
CLOBBER
config/external/nss/Makefile.in
configure.in
ipc/chromium/src/base/process_util_linux.cc
mozglue/build/BionicGlue.cpp
--- a/CLOBBER
+++ b/CLOBBER
@@ -17,9 +17,9 @@
 #
 # Modifying this file will now automatically clobber the buildbot machines \o/
 #
 
 # Are you updating CLOBBER because you think it's needed for your WebIDL
 # changes to stick? As of bug 928195, this shouldn't be necessary! Please
 # don't change CLOBBER for WebIDL changes any more.
 
-Bug 1209344 - Remove debug button from about:addons. r=mossop
+Bug 1133073 - Use PR_DuplicateEnvironment from NSPR and remove interim mozglue wrappers.
--- 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) { }