Bug 1380096 - Avoid non-null terminated strings and heap for main thread runnable name, r=erahm
authorMichael Layzell <michael@thelayzells.com>
Mon, 17 Jul 2017 11:16:44 -0400
changeset 418086 bd9af2730ced5dabe8a3c542778b3e8feddb938a
parent 418085 7a493733bf3ca4a25795e4289bea17011843a04a
child 418087 9aed7f5598221d091f6978563d1a9919d424a514
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm
bugs1380096
milestone56.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 1380096 - Avoid non-null terminated strings and heap for main thread runnable name, r=erahm For some reason, I continuously ran into windows x64 specific failures when trying to read this heap allocated data while the main thread was paused. I don't know specifically how this happened, but I am able to avoid it by instead directly allocating the buffer in a `mozilla::Array` in static storage, and copying that data instead. MozReview-Commit-ID: 473d6IpHlc4
xpcom/threads/ThreadStackHelper.cpp
xpcom/threads/nsThread.cpp
xpcom/threads/nsThread.h
--- a/xpcom/threads/ThreadStackHelper.cpp
+++ b/xpcom/threads/ThreadStackHelper.cpp
@@ -133,30 +133,29 @@ ThreadStackHelper::GetStacksInternal(Sta
 
 #ifdef MOZ_THREADSTACKHELPER_PSEUDO
   ScopedSetPtr<Stack> stackPtr(mStackToFill, aStack);
 #endif
 #ifdef MOZ_THREADSTACKHELPER_NATIVE
   ScopedSetPtr<NativeStack> nativeStackPtr(mNativeStackToFill, aNativeStack);
 #endif
 
-  char nameBuffer[1000] = {0};
+  Array<char, nsThread::kRunnableNameBufSize> runnableName;
+  runnableName[0] = '\0';
   auto callback = [&, this] (void** aPCs, size_t aCount, bool aIsMainThread) {
     // NOTE: We cannot allocate any memory in this callback, as the target
     // thread is suspended, so we first copy it into a stack-allocated buffer,
     // and then once the target thread is resumed, we can copy it into a real
     // nsCString.
     //
     // Currently we only store the names of runnables which are running on the
     // main thread, so we only want to read sMainThreadRunnableName and copy its
     // value in the case that we are currently suspending the main thread.
-    if (aIsMainThread && nsThread::sMainThreadRunnableName) {
-      strncpy(nameBuffer, nsThread::sMainThreadRunnableName, sizeof(nameBuffer));
-      // Make sure the string is null-terminated.
-      nameBuffer[sizeof(nameBuffer) - 1] = '\0';
+    if (aIsMainThread) {
+      runnableName = nsThread::sMainThreadRunnableName;
     }
 
 #ifdef MOZ_THREADSTACKHELPER_PSEUDO
     if (mStackToFill) {
       FillStackBuffer();
     }
 #endif
 
@@ -171,20 +170,21 @@ ThreadStackHelper::GetStacksInternal(Sta
   };
 
   if (mStackToFill || mNativeStackToFill) {
     profiler_suspend_and_sample_thread(mThreadId,
                                        callback,
                                        /* aSampleNative = */ !!aNativeStack);
   }
 
-  // Copy the name buffer allocation into the output string.
-  if (nameBuffer[0] != 0) {
-    aRunnableName = nameBuffer;
-  }
+  // Copy the name buffer allocation into the output string. We explicitly set
+  // the last byte to null in case we read in some corrupted data without a null
+  // terminator.
+  runnableName[nsThread::kRunnableNameBufSize - 1] = '\0';
+  aRunnableName.AssignASCII(runnableName.cbegin());
 #endif
 }
 
 bool
 ThreadStackHelper::PrepareStackBuffer(Stack& aStack)
 {
   // Return false to skip getting the stack and return an empty stack
   aStack.clear();
--- a/xpcom/threads/nsThread.cpp
+++ b/xpcom/threads/nsThread.cpp
@@ -94,17 +94,17 @@ using namespace mozilla;
 static LazyLogModule sThreadLog("nsThread");
 #ifdef LOG
 #undef LOG
 #endif
 #define LOG(args) MOZ_LOG(sThreadLog, mozilla::LogLevel::Debug, args)
 
 NS_DECL_CI_INTERFACE_GETTER(nsThread)
 
-const char* nsThread::sMainThreadRunnableName = nullptr;
+Array<char, nsThread::kRunnableNameBufSize> nsThread::sMainThreadRunnableName;
 
 //-----------------------------------------------------------------------------
 // Because we do not have our own nsIFactory, we have to implement nsIClassInfo
 // somewhat manually.
 
 class nsThreadClassInfo : public nsIClassInfo
 {
 public:
@@ -1417,25 +1417,34 @@ nsThread::ProcessNextEvent(bool aMayWait
           // overdrawn our idle budget, if it's negative it will go in
           // the 0 bucket of the histogram.
           idleTimer.emplace(name, mNextIdleDeadline);
         }
       }
 
       // If we're on the main thread, we want to record our current runnable's
       // name in a static so that BHR can record it.
-      const char* restoreRunnableName = nullptr;
+      Array<char, kRunnableNameBufSize> restoreRunnableName;
+      restoreRunnableName[0] = '\0';
       auto clear = MakeScopeExit([&] {
         if (MAIN_THREAD == mIsMainThread) {
+          MOZ_ASSERT(NS_IsMainThread());
           sMainThreadRunnableName = restoreRunnableName;
         }
       });
       if (MAIN_THREAD == mIsMainThread) {
+        MOZ_ASSERT(NS_IsMainThread());
         restoreRunnableName = sMainThreadRunnableName;
-        sMainThreadRunnableName = name.get();
+
+        // Copy the name into sMainThreadRunnableName's buffer, and append a
+        // terminating null.
+        uint32_t length = std::min((uint32_t) kRunnableNameBufSize - 1,
+                                   (uint32_t) name.Length());
+        memcpy(sMainThreadRunnableName.begin(), name.BeginReading(), length);
+        sMainThreadRunnableName[length] = '\0';
       }
 #endif
 
       event->Run();
     } else if (aMayWait) {
       MOZ_ASSERT(ShuttingDown(),
                  "This should only happen when shutting down");
       rv = NS_ERROR_UNEXPECTED;
--- a/xpcom/threads/nsThread.h
+++ b/xpcom/threads/nsThread.h
@@ -16,16 +16,17 @@
 #include "nsString.h"
 #include "nsTObserverArray.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/NotNull.h"
 #include "mozilla/TimeStamp.h"
 #include "nsAutoPtr.h"
 #include "mozilla/AlreadyAddRefed.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/Array.h"
 
 namespace mozilla {
 class CycleCollectedJSContext;
 }
 
 using mozilla::NotNull;
 
 // A native thread
@@ -88,17 +89,18 @@ public:
   {
     kMaybeReport,
     kForceReport
   };
 
   static bool SaveMemoryReportNearOOM(ShouldSaveMemoryReport aShouldSave);
 #endif
 
-  static const char* sMainThreadRunnableName;
+  static const uint32_t kRunnableNameBufSize = 1000;
+  static mozilla::Array<char, kRunnableNameBufSize> sMainThreadRunnableName;
 
 private:
   void DoMainThreadSpecificProcessing(bool aReallyWait);
 
   // Returns a null TimeStamp if we're not in the idle period.
   mozilla::TimeStamp GetIdleDeadline();
   void GetIdleEvent(nsIRunnable** aEvent, mozilla::MutexAutoLock& aProofOfLock);
   void GetEvent(bool aWait, nsIRunnable** aEvent,