Bug 990230 - Fix heap use-after-free in memory reporter. r=n.nethercote
authorDave Hylands <dhylands@mozilla.com>
Thu, 17 Apr 2014 16:57:30 -0700
changeset 197687 e224847eaf96a98ed2de19a3cb3315bc9c3974c3
parent 197686 a469d4dbbb34020c929bd0d68f1ccf8fa3b6a5de
child 197688 20d57f3ddeee3998ae8d7bc1a82cde20f1eb84e0
push id3624
push userasasaki@mozilla.com
push dateMon, 09 Jun 2014 21:49:01 +0000
treeherdermozilla-beta@b1a5da15899a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersn
bugs990230
milestone31.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 990230 - Fix heap use-after-free in memory reporter. r=n.nethercote
xpcom/base/nsDumpUtils.cpp
xpcom/base/nsDumpUtils.h
--- a/xpcom/base/nsDumpUtils.cpp
+++ b/xpcom/base/nsDumpUtils.cpp
@@ -6,25 +6,27 @@
 
 #include "nsDumpUtils.h"
 #include "nsDirectoryServiceDefs.h"
 #include "nsDirectoryServiceUtils.h"
 #include "prenv.h"
 #include <errno.h>
 #include "mozilla/Services.h"
 #include "nsIObserverService.h"
- #include "mozilla/ClearOnShutdown.h"
+#include "mozilla/ClearOnShutdown.h"
 
 #if defined(XP_LINUX) || defined(__FreeBSD__) // {
 #include "mozilla/Preferences.h"
 #include <fcntl.h>
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
+using namespace mozilla;
+
 /*
  * The following code supports triggering a registered callback upon
  * receiving a specific signal.
  *
  * Take about:memory for example, we register
  * 1. doGCCCDump for doMemoryReport
  * 2. doMemoryReport for sDumpAboutMemorySignum(SIGRTMIN)
  *                       and sDumpAboutMemoryAfterMMUSignum(SIGRTMIN+1).
@@ -113,59 +115,62 @@ SignalPipeWatcher::GetSingleton()
   if (!sSingleton) {
     sSingleton = new SignalPipeWatcher();
     sSingleton->Init();
     ClearOnShutdown(&sSingleton);
   }
   return sSingleton;
 }
 
-/* static */ void
-SignalPipeWatcher::RegisterCallback(const uint8_t aSignal,
+void
+SignalPipeWatcher::RegisterCallback(uint8_t aSignal,
                                     PipeCallback aCallback)
 {
-  for (SignalInfoArray::index_type i = 0; 
-       i < SignalPipeWatcher::mSignalInfo.Length(); i++)
+  MutexAutoLock lock(mSignalInfoLock);
+
+  for (SignalInfoArray::index_type i = 0; i < mSignalInfo.Length(); i++)
   {
-    if (SignalPipeWatcher::mSignalInfo[i].mSignal == aSignal) {
+    if (mSignalInfo[i].mSignal == aSignal) {
       LOG("Register Signal(%d) callback failed! (DUPLICATE)", aSignal);
       return;
     }
   }
-  SignalInfo aSignalInfo = { aSignal, aCallback };
-  SignalPipeWatcher::mSignalInfo.AppendElement(aSignalInfo);
-  SignalPipeWatcher::RegisterSignalHandler(aSignalInfo.mSignal);
+  SignalInfo signalInfo = { aSignal, aCallback };
+  mSignalInfo.AppendElement(signalInfo);
+  RegisterSignalHandler(signalInfo.mSignal);
 }
 
-/* static */ void
-SignalPipeWatcher::RegisterSignalHandler(const uint8_t aSignal)
+void
+SignalPipeWatcher::RegisterSignalHandler(uint8_t aSignal)
 {
   struct sigaction action;
   memset(&action, 0, sizeof(action));
   sigemptyset(&action.sa_mask);
   action.sa_handler = DumpSignalHandler;
 
   if (aSignal) {
     if (sigaction(aSignal, &action, nullptr)) {
       LOG("SignalPipeWatcher failed to register sig %d.", aSignal);
     }
   } else {
-    for (SignalInfoArray::index_type i = 0; i < SignalPipeWatcher::mSignalInfo.Length(); i++) {
-      if (sigaction(SignalPipeWatcher::mSignalInfo[i].mSignal, &action, nullptr)) {
+    MutexAutoLock lock(mSignalInfoLock);
+    for (SignalInfoArray::index_type i = 0; i < mSignalInfo.Length(); i++) {
+      if (sigaction(mSignalInfo[i].mSignal, &action, nullptr)) {
         LOG("SignalPipeWatcher failed to register signal(%d) "
-            "dump signal handler.",SignalPipeWatcher::mSignalInfo[i].mSignal);
+            "dump signal handler.", mSignalInfo[i].mSignal);
       }
     }
   }
 }
 
 SignalPipeWatcher::~SignalPipeWatcher()
 {
-  if (sDumpPipeWriteFd != -1)
-    SignalPipeWatcher::StopWatching();
+  if (sDumpPipeWriteFd != -1) {
+    StopWatching();
+  }
 }
 
 int SignalPipeWatcher::OpenFd()
 {
   MOZ_ASSERT(XRE_GetIOMessageLoop() == MessageLoopForIO::current());
 
   // Create a pipe.  When we receive a signal in our signal handler, we'll
   // write the signum to the write-end of this pipe.
@@ -177,17 +182,17 @@ int SignalPipeWatcher::OpenFd()
 
   // Close this pipe on calls to exec().
   fcntl(pipeFds[0], F_SETFD, FD_CLOEXEC);
   fcntl(pipeFds[1], F_SETFD, FD_CLOEXEC);
 
   int readFd = pipeFds[0];
   sDumpPipeWriteFd = pipeFds[1];
 
-  SignalPipeWatcher::RegisterSignalHandler();
+  RegisterSignalHandler();
   return readFd;
 }
 
 void SignalPipeWatcher::StopWatching()
 {
   MOZ_ASSERT(XRE_GetIOMessageLoop() == MessageLoopForIO::current());
 
   // Close sDumpPipeWriteFd /after/ setting the fd to -1.
@@ -210,20 +215,23 @@ void SignalPipeWatcher::OnFileCanReadWit
   uint8_t signum;
   ssize_t numReceived = read(aFd, &signum, sizeof(signum));
   if (numReceived != sizeof(signum)) {
     LOG("Error reading from buffer in "
         "SignalPipeWatcher::OnFileCanReadWithoutBlocking.");
     return;
   }
 
-  for (SignalInfoArray::index_type i = 0; i < SignalPipeWatcher::mSignalInfo.Length(); i++) {
-    if(signum == SignalPipeWatcher::mSignalInfo[i].mSignal) {
-      SignalPipeWatcher::mSignalInfo[i].mCallback(signum);
-      return;
+  {
+    MutexAutoLock lock(mSignalInfoLock);
+    for (SignalInfoArray::index_type i = 0; i < mSignalInfo.Length(); i++) {
+      if(signum == mSignalInfo[i].mSignal) {
+        mSignalInfo[i].mCallback(signum);
+        return;
+      }
     }
   }
   LOG("SignalPipeWatcher got unexpected signum.");
 }
 
 StaticRefPtr<FifoWatcher> FifoWatcher::sSingleton;
 
 /* static */ FifoWatcher*
@@ -252,35 +260,36 @@ FifoWatcher::MaybeCreate()
   }
 
   if (!Preferences::GetBool("memory_info_dumper.watch_fifo.enabled", false)) {
     LOG("Fifo watcher disabled via pref.");
     return false;
   }
 
   // The FifoWatcher is held alive by the observer service.
-  if (!FifoWatcher::sSingleton) {
-    FifoWatcher::GetSingleton();
+  if (!sSingleton) {
+    GetSingleton();
   }
   return true;
 }
 
 void
-FifoWatcher::RegisterCallback(const nsCString& aCommand,FifoCallback aCallback)
+FifoWatcher::RegisterCallback(const nsCString& aCommand, FifoCallback aCallback)
 {
-  for (FifoInfoArray::index_type i = 0;
-       i < FifoWatcher::mFifoInfo.Length(); i++)
+  MutexAutoLock lock(mFifoInfoLock);
+
+  for (FifoInfoArray::index_type i = 0; i < mFifoInfo.Length(); i++)
   {
-    if (FifoWatcher::mFifoInfo[i].mCommand.Equals(aCommand)) {
+    if (mFifoInfo[i].mCommand.Equals(aCommand)) {
       LOG("Register command(%s) callback failed! (DUPLICATE)", aCommand.get());
       return;
     }
   }
   FifoInfo aFifoInfo = { aCommand, aCallback };
-  FifoWatcher::mFifoInfo.AppendElement(aFifoInfo);
+  mFifoInfo.AppendElement(aFifoInfo);
 }
 
 FifoWatcher::~FifoWatcher()
 {
 }
 
 int FifoWatcher::OpenFd()
 {
@@ -387,21 +396,25 @@ void FifoWatcher::OnFileCanReadWithoutBl
   nsAutoCString inputStr;
   inputStr.Append(buf, nread);
 
   // Trimming whitespace is important because if you do
   //   |echo "foo" >> debug_info_trigger|,
   // it'll actually write "foo\n" to the fifo.
   inputStr.Trim("\b\t\r\n");
 
-  for (FifoInfoArray::index_type i = 0; i < FifoWatcher::mFifoInfo.Length(); i++) {
-    const nsCString commandStr = FifoWatcher::mFifoInfo[i].mCommand;
-    if(inputStr == commandStr.get()) {
-      FifoWatcher::mFifoInfo[i].mCallback(inputStr);
-      return;
+  {
+    MutexAutoLock lock(mFifoInfoLock);
+
+    for (FifoInfoArray::index_type i = 0; i < mFifoInfo.Length(); i++) {
+      const nsCString commandStr = mFifoInfo[i].mCommand;
+      if(inputStr == commandStr.get()) {
+        mFifoInfo[i].mCallback(inputStr);
+        return;
+      }
     }
   }
   LOG("Got unexpected value from fifo; ignoring it.");
 }
 
 #endif // XP_LINUX }
 
 // In Android case, this function will open a file named aFilename under
--- a/xpcom/base/nsDumpUtils.h
+++ b/xpcom/base/nsDumpUtils.h
@@ -6,16 +6,17 @@
 
 #ifndef mozilla_nsDumpUtils_h
 #define mozilla_nsDumpUtils_h
 
 #include "nsIObserver.h"
 #include "base/message_loop.h"
 #include "nsXULAppAPI.h"
 #include "nsThreadUtils.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/StaticPtr.h"
 #include "nsTArray.h"
 
 #ifdef LOG
 #undef LOG
 #endif
 
 #ifdef ANDROID
@@ -121,53 +122,57 @@ public:
 
 private:
   nsAutoCString mDirPath;
 
   static StaticRefPtr<FifoWatcher> sSingleton;
 
   FifoWatcher(nsCString aPath)
     : mDirPath(aPath)
+    , mFifoInfoLock("FifoWatcher.mFifoInfoLock")
   {}
 
+  mozilla::Mutex mFifoInfoLock; // protects mFifoInfo
   FifoInfoArray mFifoInfo;
 };
 
 typedef void (* PipeCallback)(const uint8_t recvSig);
 struct SignalInfo {
   uint8_t mSignal;
   PipeCallback mCallback;
 };
 typedef nsTArray<SignalInfo> SignalInfoArray;
 
 class SignalPipeWatcher : public FdWatcher
 {
 public:
   static SignalPipeWatcher* GetSingleton();
 
-  void RegisterCallback(const uint8_t aSignal, PipeCallback aCallback);
+  void RegisterCallback(uint8_t aSignal, PipeCallback aCallback);
 
-  void RegisterSignalHandler(const uint8_t aSignal = 0);
+  void RegisterSignalHandler(uint8_t aSignal = 0);
 
   virtual ~SignalPipeWatcher();
 
   virtual int OpenFd();
 
   virtual void StopWatching();
 
   virtual void OnFileCanReadWithoutBlocking(int aFd);
 
 private:
   static StaticRefPtr<SignalPipeWatcher> sSingleton;
 
   SignalPipeWatcher()
+    : mSignalInfoLock("SignalPipeWatcher.mSignalInfoLock")
   {
     MOZ_ASSERT(NS_IsMainThread());
   }
 
+  mozilla::Mutex mSignalInfoLock; // protects mSignalInfo
   SignalInfoArray mSignalInfo;
 };
 
 #endif // XP_LINUX }
 
 
 class nsDumpUtils
 {