Bug 1569862 - Implement PHC on Windows. r=glandium

The most important part is the `GetPHCAddrInfo()` function in
`exception_handler.cc`, which extracts a crash address from an
`EXCEPTION_POINTERS` object.

The commit also changes the code so that it works if MozStackWalk() returns an
empty stack trace. This happens in practice on Windows on automation sometimes,
I'm not sure why.

The rest of the commit is just plumbing and the smoothing over of minor
platform differences.

Differential Revision: https://phabricator.services.mozilla.com/D39841

memory/replace/phc/PHC.cpp

@@ -137,22 +137,14 @@ class InfallibleAllocPolicy {
 
 class StackTrace : public phc::StackTrace {
  public:
-  StackTrace() : phc::StackTrace(), mSkipped(false) {}
-
-  bool IsEmpty() const { return mLength == 0 && !mSkipped; }
+  StackTrace() : phc::StackTrace() {}
 
   void Clear() {
     mLength = 0;
-    mSkipped = false;
   }
 
   void Fill();
 
-  void FillSkipped() {
-    mLength = 0;
-    mSkipped = true;
-  }
-
  private:
   static void StackWalkCallback(uint32_t aFrameNumber, void* aPc, void* aSp,
                                 void* aClosure) {
@@ -162,11 +154,6 @@ class StackTrace : public phc::StackTrace {
     st->mLength++;
     MOZ_ASSERT(st->mLength == aFrameNumber);
   }
-
-  // There are some rare cases (see FillSkipped's call sites) where we want to
-  // get a stack trace but cannot do so safely. When this field is set it
-  // indicates such a stack trace.
-  bool mSkipped;
 };
 
 // WARNING WARNING WARNING: this function must only be called when GMut::sMutex
@@ -190,7 +177,6 @@ class StackTrace : public phc::StackTrace {
 //
 void StackTrace::Fill() {
   mLength = 0;
-  mSkipped = false;
 
 #if defined(XP_WIN) && defined(_M_IX86)
   // This avoids MozStackWalk(), which causes unusably slow startup on Win32
@@ -558,16 +544,16 @@ class GMut {
     size_t mUsableSize;
 
     // The allocation stack.
-    // - NeverAllocated: empty.
-    // - InUse: non-empty.
-    // - Freed: non-empty.
-    StackTrace mAllocStack;
+    // - NeverAllocated: Nothing.
+    // - InUse: Some.
+    // - Freed: Some.
+    Maybe<StackTrace> mAllocStack;
 
     // The free stack.
-    // - NeverAllocated: empty.
-    // - InUse: empty.
-    // - Freed: non-empty.
-    StackTrace mFreeStack;
+    // - NeverAllocated: Nothing.
+    // - InUse: Some.
+    // - Freed: Some.
+    Maybe<StackTrace> mFreeStack;
 
     // The time at which the page is available for reuse, as measured against
     // GAtomic::sNow. When the page is in use this value will be kMaxTime.
@@ -620,8 +606,8 @@ class GMut {
     page.mState = PageState::InUse;
     page.mArenaId = aArenaId;
     page.mUsableSize = aUsableSize;
-    page.mAllocStack = aAllocStack;
-    page.mFreeStack.Clear();
+    page.mAllocStack = Some(aAllocStack);
+    page.mFreeStack = Nothing();
     page.mReuseTime = kMaxTime;
   }
 
@@ -641,7 +627,7 @@ class GMut {
     page.mUsableSize = aNewUsableSize;
     // We could just keep the original alloc stack, but the realloc stack is
     // more recent and therefore seems more useful.
-    page.mAllocStack = aAllocStack;
+    page.mAllocStack = Some(aAllocStack);
     // page.mFreeStack is not changed.
     // page.mReuseTime is not changed.
   };
@@ -667,7 +653,7 @@ class GMut {
 
     // page.mAllocStack is left unchanged, for reporting on UAF.
 
-    page.mFreeStack = aFreeStack;
+    page.mFreeStack = Some(aFreeStack);
     page.mReuseTime = GAtomic::Now() + aReuseDelay;
   }
 
@@ -776,8 +762,8 @@ class GMut {
     MOZ_ASSERT(aPage.mState == PageState::InUse);
     // There is nothing to assert about aPage.mArenaId.
     MOZ_ASSERT(aPage.mUsableSize > 0);
-    MOZ_ASSERT(!aPage.mAllocStack.IsEmpty());
-    MOZ_ASSERT(aPage.mFreeStack.IsEmpty());
+    MOZ_ASSERT(aPage.mAllocStack.isSome());
+    MOZ_ASSERT(aPage.mFreeStack.isNothing());
     MOZ_ASSERT(aPage.mReuseTime == kMaxTime);
   }
 
@@ -789,8 +775,8 @@ class GMut {
     MOZ_ASSERT(isFresh || aPage.mState == PageState::Freed);
     MOZ_ASSERT_IF(isFresh, aPage.mArenaId == Nothing());
     MOZ_ASSERT(isFresh == (aPage.mUsableSize == 0));
-    MOZ_ASSERT(isFresh == (aPage.mAllocStack.IsEmpty()));
-    MOZ_ASSERT(isFresh == (aPage.mFreeStack.IsEmpty()));
+    MOZ_ASSERT(isFresh == (aPage.mAllocStack.isNothing()));
+    MOZ_ASSERT(isFresh == (aPage.mFreeStack.isNothing()));
     MOZ_ASSERT(aPage.mReuseTime != kMaxTime);
 #endif
   }
@@ -1023,8 +1009,7 @@ MOZ_ALWAYS_INLINE static void* PageRealloc(const Maybe<arena_id_t>& aArenaId,
   // Get the stack trace *before* locking the mutex.
   StackTrace stack;
   if (GTls::IsDisabledOnCurrentThread()) {
-    // PHC is disabled on this thread. Get a dummy stack.
-    stack.FillSkipped();
+    // PHC is disabled on this thread. Leave the stack empty.
   } else {
     // Disable on this thread *before* getting the stack trace.
     disable.emplace();
@@ -1097,8 +1082,7 @@ MOZ_ALWAYS_INLINE static void PageFree(const Maybe<arena_id_t>& aArenaId,
   // Get the stack trace *before* locking the mutex.
   StackTrace freeStack;
   if (GTls::IsDisabledOnCurrentThread()) {
-    // PHC is disabled on this thread. Get a dummy stack.
-    freeStack.FillSkipped();
+    // PHC is disabled on this thread. Leave the stack empty.
   } else {
     // Disable on this thread *before* getting the stack trace.
     disable.emplace();
@@ -1153,7 +1137,7 @@ static size_t replace_malloc_usable_size(usable_ptr_t aPtr) {
   MutexAutoLock lock(GMut::sMutex);
 
   // Check for malloc_usable_size() of a freed block.
-  gMut->EnsureInUse(lock, aPtr, *i);
+  gMut->EnsureInUse(lock, const_cast<void*>(aPtr), *i);
 
   return gMut->PageUsableSize(lock, *i);
 }

memory/replace/phc/PHC.h

@@ -8,13 +8,16 @@
 #define PHC_h
 
 #include "mozilla/Assertions.h"
+#include "mozilla/Maybe.h"
 #include <stdint.h>
 #include <stdlib.h>
 
 namespace mozilla {
 namespace phc {
 
-// Note: a more compact stack trace representation could be achieved with
+// Note: a stack trace may have no frames due to a collection problem.
+//
+// Also note: a more compact stack trace representation could be achieved with
 // some effort.
 struct StackTrace {
  public:
@@ -70,8 +73,8 @@ class AddrInfo {
 
   // The allocation and free stack traces of the containing PHC allocation, if
   // there is one.
-  StackTrace mAllocStack;
-  StackTrace mFreeStack;
+  mozilla::Maybe<StackTrace> mAllocStack;
+  mozilla::Maybe<StackTrace> mFreeStack;
 
   // Default to no PHC info.
   AddrInfo()

memory/replace/phc/test/gtest/TestPHC.cpp

@@ -16,10 +16,10 @@ bool PHCInfoEq(phc::AddrInfo& aInfo, phc::AddrInfo::Kind aKind, void* aBaseAddr,
   return aInfo.mKind == aKind && aInfo.mBaseAddr == aBaseAddr &&
          aInfo.mUsableSize == aUsableSize &&
          // Proper stack traces will have at least 3 elements.
-         (aHasAllocStack ? (aInfo.mAllocStack.mLength > 2)
-                         : (aInfo.mAllocStack.mLength == 0)) &&
-         (aHasFreeStack ? (aInfo.mFreeStack.mLength > 2)
-                        : (aInfo.mFreeStack.mLength == 0));
+         (aHasAllocStack ? (aInfo.mAllocStack->mLength > 2)
+                         : (aInfo.mAllocStack.isNothing())) &&
+         (aHasFreeStack ? (aInfo.mFreeStack->mLength > 2)
+                        : (aInfo.mFreeStack.isNothing()));
 }
 
 bool JeInfoEq(jemalloc_ptr_info_t& aInfo, PtrInfoTag aTag, void* aAddr,
@@ -70,15 +70,15 @@ TEST(PHC, TestPHCBasics)
   ASSERT_TRUE(ReplaceMalloc::IsPHCAllocation(p, &phcInfo));
   ASSERT_TRUE(
       PHCInfoEq(phcInfo, phc::AddrInfo::Kind::InUsePage, p, 32ul, true, false));
-  ASSERT_EQ(malloc_usable_size(p), 32ul);
+  ASSERT_EQ(moz_malloc_usable_size(p), 32ul);
   jemalloc_ptr_info(p, &jeInfo);
   ASSERT_TRUE(JeInfoEq(jeInfo, TagLiveAlloc, p, 32, 0));
 
   // Test an in-use PHC allocation, via an address in its middle.
   ASSERT_TRUE(ReplaceMalloc::IsPHCAllocation(p + 10, &phcInfo));
   ASSERT_TRUE(
       PHCInfoEq(phcInfo, phc::AddrInfo::Kind::InUsePage, p, 32ul, true, false));
-  ASSERT_EQ(malloc_usable_size(p), 32ul);
+  ASSERT_EQ(moz_malloc_usable_size(p), 32ul);
   jemalloc_ptr_info(p + 10, &jeInfo);
   ASSERT_TRUE(JeInfoEq(jeInfo, TagLiveAlloc, p, 32, 0));
 

toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.cc

@@ -451,7 +451,8 @@ int ExceptionHandler::ThreadEntry(void *arg) {
 }
 
 #ifdef MOZ_PHC
-void GetPHCAddrInfo(siginfo_t* siginfo, mozilla::phc::AddrInfo* addr_info) {
+static void GetPHCAddrInfo(siginfo_t* siginfo,
+                           mozilla::phc::AddrInfo* addr_info) {
   // Is this a crash involving a PHC allocation?
   if (siginfo->si_signo == SIGSEGV || siginfo->si_signo == SIGBUS) {
     ReplaceMalloc::IsPHCAllocation(siginfo->si_addr, addr_info);

toolkit/crashreporter/breakpad-client/windows/handler/exception_handler.cc

@@ -39,6 +39,10 @@
 #include "windows/handler/exception_handler.h"
 #include "common/windows/guid_string.h"
 
+#ifdef MOZ_PHC
+#include "replace_malloc_bridge.h"
+#endif
+
 namespace google_breakpad {
 
 // This define is new to Windows 10.
@@ -823,24 +827,45 @@ bool ExceptionHandler::WriteMinidumpForChild(HANDLE child,
   CloseHandle(child_thread_handle);
 
   if (callback) {
+    // nullptr here for phc::AddrInfo* is ok because this is not a crash.
     success = callback(handler.dump_path_c_, handler.next_minidump_id_c_,
                        callback_context, NULL, NULL, nullptr, success);
   }
 
   return success;
 }
 
+#ifdef MOZ_PHC
+static void GetPHCAddrInfo(EXCEPTION_POINTERS* exinfo,
+                           mozilla::phc::AddrInfo* addr_info) {
+  // Is this a crash involving a PHC allocation?
+  PEXCEPTION_RECORD rec = exinfo->ExceptionRecord;
+  if (rec->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) {
+    // rec->ExceptionInformation[0] contains a value indicating what type of
+    // operation it what, and rec->ExceptionInformation[1] contains the
+    // virtual address of the inaccessible data.
+    char* crashAddr = reinterpret_cast<char*>(rec->ExceptionInformation[1]);
+    ReplaceMalloc::IsPHCAllocation(crashAddr, addr_info);
+  }
+}
+#endif
+
 bool ExceptionHandler::WriteMinidumpWithException(
     DWORD requesting_thread_id,
     EXCEPTION_POINTERS* exinfo,
     MDRawAssertionInfo* assertion) {
+    mozilla::phc::AddrInfo addr_info;
+#ifdef MOZ_PHC
+    GetPHCAddrInfo(exinfo, &addr_info);
+#endif
+
   // Give user code a chance to approve or prevent writing a minidump.  If the
   // filter returns false, don't handle the exception at all.  If this method
   // was called as a result of an exception, returning false will cause
   // HandleException to call any previous handler or return
   // EXCEPTION_CONTINUE_SEARCH on the exception thread, allowing it to appear
   // as though this handler were not present at all.
-  if (filter_ && !filter_(callback_context_, exinfo, nullptr, assertion)) {
+  if (filter_ && !filter_(callback_context_, exinfo, &addr_info, assertion)) {
     return false;
   }
 
@@ -861,7 +886,7 @@ bool ExceptionHandler::WriteMinidumpWithException(
     // scenario, the server process ends up creating the dump path and dump
     // id so they are not known to the client.
     success = callback_(dump_path_c_, next_minidump_id_c_, callback_context_,
-                        exinfo, assertion, nullptr, success);
+                        exinfo, assertion, &addr_info, success);
   }
 
   return success;

toolkit/crashreporter/breakpad-client/windows/handler/objs.mozbuild

@@ -12,3 +12,6 @@ subdir = 'toolkit/crashreporter/breakpad-client/windows/handler'
 objs_handler = [
     '/%s/%s' % (subdir, s) for s in lobjs_handler
 ]
+
+if CONFIG['MOZ_PHC']:
+    DEFINES['MOZ_PHC'] = True

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -673,7 +673,11 @@ class BinaryAnnotationWriter : public AnnotationWriter {
 // (not hexadecimal!) addresses, e.g. "12345678,12345679,12345680".
 static void WritePHCStackTrace(AnnotationWriter& aWriter,
                                const Annotation aName,
-                               const phc::StackTrace* aStack) {
+                               const Maybe<phc::StackTrace>& aStack) {
+  if (aStack.isNothing()) {
+    return;
+  }
+
   // 21 is the max length of a 64-bit decimal address entry, including the
   // trailing comma or '\0'. And then we add another 32 just to be safe.
   char addrsString[mozilla::phc::StackTrace::kMaxFrames * 21 + 32];
@@ -727,9 +731,8 @@ static void WritePHCAddrInfo(AnnotationWriter& writer,
     writer.Write(Annotation::PHCUsableSize, usableSizeString);
 
     WritePHCStackTrace(writer, Annotation::PHCAllocStack,
-                       &aAddrInfo->mAllocStack);
-    WritePHCStackTrace(writer, Annotation::PHCFreeStack,
-                       &aAddrInfo->mFreeStack);
+                       aAddrInfo->mAllocStack);
+    WritePHCStackTrace(writer, Annotation::PHCFreeStack, aAddrInfo->mFreeStack);
   }
 }
 #endif
@@ -1352,7 +1355,7 @@ static void FreeBreakpadVM() {
  * Also calls FreeBreakpadVM if appropriate.
  */
 static bool FPEFilter(void* context, EXCEPTION_POINTERS* exinfo,
-                      const phc::AddrInfo* addr_info,
+                      const phc::AddrInfo* addrInfo,
                       MDRawAssertionInfo* assertion) {
   if (!exinfo) {
     mozilla::IOInterposer::Disable();