Bug 1270752 - Fix lifetime of buffer passed to PR_SetEnv(). r=jduell
authorHonza Bambas <honzab.moz@firemni.cz>
Thu, 26 May 2016 03:18:00 -0400
changeset 340194 062491c6264039e58c2b300997ba9eef23dbc59a
parent 340193 ebb8ec09a40a70d92416be63f08a7fed6c3a556b
child 340195 050485808a590c3db6c2bf001a1c18897d26db04
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjduell
bugs1270752
milestone49.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 1270752 - Fix lifetime of buffer passed to PR_SetEnv(). r=jduell
ipc/glue/GeckoChildProcessHost.cpp
ipc/glue/GeckoChildProcessHost.h
--- a/ipc/glue/GeckoChildProcessHost.cpp
+++ b/ipc/glue/GeckoChildProcessHost.cpp
@@ -487,32 +487,34 @@ GeckoChildProcessHost::DissociateActor()
     MessageLoop::current()->
       PostTask(NewRunnableFunction(DelayedDeleteSubprocess, this));
   }
 }
 
 int32_t GeckoChildProcessHost::mChildCounter = 0;
 
 void
-GeckoChildProcessHost::SetChildLogName(const char* varName, const char* origLogName)
+GeckoChildProcessHost::SetChildLogName(const char* varName, const char* origLogName,
+                                       nsACString &buffer)
 {
   // We currently have no portable way to launch child with environment
   // different than parent.  So temporarily change NSPR_LOG_FILE so child
   // inherits value we want it to have. (NSPR only looks at NSPR_LOG_FILE at
   // startup, so it's 'safe' to play with the parent's environment this way.)
-  nsAutoCString setChildLogName(varName);
-  setChildLogName.Append(origLogName);
+  buffer.Assign(varName);
+  buffer.Append(origLogName);
 
   // Append child-specific postfix to name
-  setChildLogName.AppendLiteral(".child-");
-  setChildLogName.AppendInt(mChildCounter);
+  buffer.AppendLiteral(".child-");
+  buffer.AppendInt(mChildCounter);
 
-  // Passing temporary to PR_SetEnv is ok here because env gets copied
-  // by exec, etc., to permanent storage in child when process launched.
-  PR_SetEnv(setChildLogName.get());
+  // Passing temporary to PR_SetEnv is ok here if we keep the temporary
+  // for the time we launch the sub-process.  It's copied to the new
+  // environment.
+  PR_SetEnv(buffer.BeginReading());
 }
 
 bool
 GeckoChildProcessHost::PerformAsyncLaunch(std::vector<std::string> aExtraOpts, base::ProcessArchitecture arch)
 {
   // If NSPR log files are not requested, we're done.
   const char* origNSPRLogName = PR_GetEnv("NSPR_LOG_FILE");
   const char* origMozLogName = PR_GetEnv("MOZ_LOG_FILE");
@@ -523,29 +525,34 @@ GeckoChildProcessHost::PerformAsyncLaunc
   ++mChildCounter;
 
   // remember original value so we can restore it.
   // - Note: this code is not called re-entrantly, nor are restoreOrig*LogName
   //   or mChildCounter touched by any other thread, so this is safe.
   static nsAutoCString restoreOrigNSPRLogName;
   static nsAutoCString restoreOrigMozLogName;
 
+  // Must keep these on the same stack where from we call PerformAsyncLaunchInternal
+  // so that PR_DuplicateEnvironment() still sees a valid memory.
+  nsAutoCString nsprLogName;
+  nsAutoCString mozLogName;
+
   if (origNSPRLogName) {
     if (restoreOrigNSPRLogName.IsEmpty()) {
       restoreOrigNSPRLogName.AssignLiteral("NSPR_LOG_FILE=");
       restoreOrigNSPRLogName.Append(origNSPRLogName);
     }
-    SetChildLogName("NSPR_LOG_FILE=", origNSPRLogName);
+    SetChildLogName("NSPR_LOG_FILE=", origNSPRLogName, nsprLogName);
   }
   if (origMozLogName) {
     if (restoreOrigMozLogName.IsEmpty()) {
       restoreOrigMozLogName.AssignLiteral("MOZ_LOG_FILE=");
       restoreOrigMozLogName.Append(origMozLogName);
     }
-    SetChildLogName("MOZ_LOG_FILE=", origMozLogName);
+    SetChildLogName("MOZ_LOG_FILE=", origMozLogName, mozLogName);
   }
 
   bool retval = PerformAsyncLaunchInternal(aExtraOpts, arch);
 
   // Revert to original value
   if (origNSPRLogName) {
     PR_SetEnv(restoreOrigNSPRLogName.get());
   }
--- a/ipc/glue/GeckoChildProcessHost.h
+++ b/ipc/glue/GeckoChildProcessHost.h
@@ -195,17 +195,20 @@ private:
   bool PerformAsyncLaunchInternal(std::vector<std::string>& aExtraOpts,
                                   base::ProcessArchitecture arch);
 
   bool RunPerformAsyncLaunch(StringVector aExtraOpts=StringVector(),
 			     base::ProcessArchitecture aArch=base::GetCurrentProcessArchitecture());
 
   static void GetPathToBinary(FilePath& exePath);
 
-  void SetChildLogName(const char* varName, const char* origLogName);
+  // The buffer is passed to preserve its lifetime until we are done
+  // with launching the sub-process.
+  void SetChildLogName(const char* varName, const char* origLogName,
+                       nsACString &buffer);
 
   // In between launching the subprocess and handing off its IPC
   // channel, there's a small window of time in which *we* might still
   // be the channel listener, and receive messages.  That's bad
   // because we have no idea what to do with those messages.  So queue
   // them here until we hand off the eventual listener.
   //
   // FIXME/cjones: this strongly indicates bad design.  Shame on us.