From d3b8a39069450801d5ba2b5e68ca66b36c09a794 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 2 Aug 2019 00:33:04 +0000 Subject: [PATCH] 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 | 56 +++++++------------ memory/replace/phc/PHC.h | 9 ++- memory/replace/phc/test/gtest/TestPHC.cpp | 12 ++-- .../linux/handler/exception_handler.cc | 3 +- .../windows/handler/exception_handler.cc | 29 +++++++++- .../windows/handler/objs.mozbuild | 3 + toolkit/crashreporter/nsExceptionHandler.cpp | 13 +++-- 7 files changed, 72 insertions(+), 53 deletions(-) diff --git a/memory/replace/phc/PHC.cpp b/memory/replace/phc/PHC.cpp index afe60bbf7efa3..37a0b2bba5914 100644 --- a/memory/replace/phc/PHC.cpp +++ b/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 mAllocStack; // The free stack. - // - NeverAllocated: empty. - // - InUse: empty. - // - Freed: non-empty. - StackTrace mFreeStack; + // - NeverAllocated: Nothing. + // - InUse: Some. + // - Freed: Some. + Maybe 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& 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& 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(aPtr), *i); return gMut->PageUsableSize(lock, *i); } diff --git a/memory/replace/phc/PHC.h b/memory/replace/phc/PHC.h index 5c606f06566e9..fd29c308b764c 100644 --- a/memory/replace/phc/PHC.h +++ b/memory/replace/phc/PHC.h @@ -8,13 +8,16 @@ #define PHC_h #include "mozilla/Assertions.h" +#include "mozilla/Maybe.h" #include #include 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 mAllocStack; + mozilla::Maybe mFreeStack; // Default to no PHC info. AddrInfo() diff --git a/memory/replace/phc/test/gtest/TestPHC.cpp b/memory/replace/phc/test/gtest/TestPHC.cpp index c0ec0a54c4922..f1a629a9feb37 100644 --- a/memory/replace/phc/test/gtest/TestPHC.cpp +++ b/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,7 +70,7 @@ 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)); @@ -78,7 +78,7 @@ TEST(PHC, TestPHCBasics) 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)); diff --git a/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.cc b/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.cc index 4ae454d23363c..ff976a09e5bc2 100644 --- a/toolkit/crashreporter/breakpad-client/linux/handler/exception_handler.cc +++ b/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); diff --git a/toolkit/crashreporter/breakpad-client/windows/handler/exception_handler.cc b/toolkit/crashreporter/breakpad-client/windows/handler/exception_handler.cc index 2c764af0d9dc6..6012ac0cf668b 100644 --- a/toolkit/crashreporter/breakpad-client/windows/handler/exception_handler.cc +++ b/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,6 +827,7 @@ 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); } @@ -830,17 +835,37 @@ bool ExceptionHandler::WriteMinidumpForChild(HANDLE child, 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(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; diff --git a/toolkit/crashreporter/breakpad-client/windows/handler/objs.mozbuild b/toolkit/crashreporter/breakpad-client/windows/handler/objs.mozbuild index efff899558ead..22dc0d27dd602 100644 --- a/toolkit/crashreporter/breakpad-client/windows/handler/objs.mozbuild +++ b/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 diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp index ec32995feb4eb..7eaea879a78a9 100644 --- a/toolkit/crashreporter/nsExceptionHandler.cpp +++ b/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& 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();