From 13ecdfb5c86efc2b91457785491f6bd08f19f969 Mon Sep 17 00:00:00 2001 From: Doug Thayer Date: Fri, 27 Mar 2020 21:00:47 +0000 Subject: [PATCH] Bug 1595596 - Use MMAP_FAULT_HANDLER in StartupCache r=aklotz Please double check that I am using this correctly. I believe we are seeing the crash in the linked bug because we are not handling hardware faults when reading from the memory mapped file. This patch just wraps all accesses in the MMAP_FAULT_HANDLER_ macros. Depends on D53042 Differential Revision: https://phabricator.services.mozilla.com/D53043 --HG-- rename : modules/libjar/MmapFaultHandler.cpp => mozglue/misc/MmapFaultHandler.cpp rename : modules/libjar/MmapFaultHandler.h => mozglue/misc/MmapFaultHandler.h extra : moz-landing-system : lando --- modules/libjar/moz.build | 1 - modules/libjar/nsJARInputStream.cpp | 2 +- modules/libjar/nsZipArchive.cpp | 2 +- .../misc}/MmapFaultHandler.cpp | 117 +++++++++--------- .../misc}/MmapFaultHandler.h | 29 +++-- mozglue/misc/moz.build | 2 + startupcache/StartupCache.cpp | 20 ++- 7 files changed, 97 insertions(+), 76 deletions(-) rename {modules/libjar => mozglue/misc}/MmapFaultHandler.cpp (66%) rename {modules/libjar => mozglue/misc}/MmapFaultHandler.h (69%) diff --git a/modules/libjar/moz.build b/modules/libjar/moz.build index a24a957df966..0055e5012b8a 100644 --- a/modules/libjar/moz.build +++ b/modules/libjar/moz.build @@ -30,7 +30,6 @@ EXPORTS += [ ] UNIFIED_SOURCES += [ - 'MmapFaultHandler.cpp', 'nsJAR.cpp', 'nsJARChannel.cpp', 'nsJARInputStream.cpp', diff --git a/modules/libjar/nsJARInputStream.cpp b/modules/libjar/nsJARInputStream.cpp index 9ad323e06bde..668e116bd01f 100644 --- a/modules/libjar/nsJARInputStream.cpp +++ b/modules/libjar/nsJARInputStream.cpp @@ -11,7 +11,7 @@ # include "brotli/decode.h" // brotli #endif #include "nsZipArchive.h" -#include "MmapFaultHandler.h" +#include "mozilla/MmapFaultHandler.h" #include "nsEscape.h" #include "nsDebug.h" diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp index a61a1eb5b348..5cbd6d71afc2 100644 --- a/modules/libjar/nsZipArchive.cpp +++ b/modules/libjar/nsZipArchive.cpp @@ -16,7 +16,7 @@ # include "brotli/decode.h" // brotli #endif #include "nsISupportsUtils.h" -#include "MmapFaultHandler.h" +#include "mozilla/MmapFaultHandler.h" #include "prio.h" #include "plstr.h" #include "mozilla/Attributes.h" diff --git a/modules/libjar/MmapFaultHandler.cpp b/mozglue/misc/MmapFaultHandler.cpp similarity index 66% rename from modules/libjar/MmapFaultHandler.cpp rename to mozglue/misc/MmapFaultHandler.cpp index 6f9f8a5ff3ac..e1cfd16626ba 100644 --- a/modules/libjar/MmapFaultHandler.cpp +++ b/mozglue/misc/MmapFaultHandler.cpp @@ -8,13 +8,43 @@ #if defined(XP_UNIX) && !defined(XP_DARWIN) -# include "nsZipArchive.h" -# include "mozilla/Atomics.h" -# include "mozilla/StaticMutex.h" +# include "PlatformMutex.h" # include "MainThreadUtils.h" +# include "mozilla/Atomics.h" +# include "mozilla/GuardObjects.h" # include "mozilla/ThreadLocal.h" # include +class MmapMutex : private mozilla::detail::MutexImpl { + public: + MmapMutex() : mozilla::detail::MutexImpl() {} + void Lock() { mozilla::detail::MutexImpl::lock(); } + void Unlock() { mozilla::detail::MutexImpl::unlock(); } +}; + +class StaticMmapMutex { + public: + void Lock() { Mutex()->Lock(); } + + void Unlock() { Mutex()->Unlock(); } + + private: + MmapMutex* Mutex() { + if (mMutex) { + return mMutex; + } + + MmapMutex* mutex = new MmapMutex(); + if (!mMutex.compareExchange(nullptr, mutex)) { + delete mutex; + } + + return mMutex; + } + + mozilla::Atomic mMutex; +}; + static MOZ_THREAD_LOCAL(MmapAccessScope*) sMmapAccessScope; static struct sigaction sPrevSIGBUSHandler; @@ -31,7 +61,6 @@ static void MmapSIGBUSHandler(int signum, siginfo_t* info, void* context) { // The address is inside the buffer, handle the failure. siglongjmp(mas->mJmpBuf, signum); - return; } // This signal is not caused by accessing region protected by MmapAccessScope. @@ -49,7 +78,20 @@ static void MmapSIGBUSHandler(int signum, siginfo_t* info, void* context) { } mozilla::Atomic gSIGBUSHandlerInstalled(false); -mozilla::StaticMutex gSIGBUSHandlerMutex; +StaticMmapMutex gSIGBUSHandlerMutex; + +class MOZ_RAII MmapMutexAutoLock { + public: + explicit MmapMutexAutoLock(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM) { + MOZ_GUARD_OBJECT_NOTIFIER_INIT; + gSIGBUSHandlerMutex.Lock(); + } + + ~MmapMutexAutoLock() { gSIGBUSHandlerMutex.Unlock(); } + + private: + MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER +}; void InstallMmapFaultHandler() { // This function is called from MmapAccessScope's constructor because there is @@ -60,7 +102,7 @@ void InstallMmapFaultHandler() { return; } - mozilla::StaticMutexAutoLock lock(gSIGBUSHandlerMutex); + MmapMutexAutoLock lock; // We must check it again, because the handler could be installed on another // thread when we were waiting for the lock. @@ -81,29 +123,15 @@ void InstallMmapFaultHandler() { gSIGBUSHandlerInstalled = true; } -MmapAccessScope::MmapAccessScope(void* aBuf, uint32_t aBufLen) { +MmapAccessScope::MmapAccessScope(void* aBuf, uint32_t aBufLen, + const char* aFilename) { // Install signal handler if it wasn't installed yet. InstallMmapFaultHandler(); // We'll handle the signal only if the crashing address is inside this buffer. mBuf = aBuf; mBufLen = aBufLen; - - SetThreadLocalScope(); -} - -MmapAccessScope::MmapAccessScope(nsZipHandle* aZipHandle) - : mBuf(nullptr), mBufLen(0) { - // Install signal handler if it wasn't installed yet. - InstallMmapFaultHandler(); - - // It's OK if aZipHandle is null (e.g. called from nsJARInputStream::Read - // when mFd was already release), because no access to mmapped memory is made - // in this case. - if (aZipHandle && aZipHandle->mMap) { - // Handle SIGBUS only when it's an mmaped zip file. - mZipHandle = aZipHandle; - } + mFilename = aFilename; SetThreadLocalScope(); } @@ -129,48 +157,15 @@ void MmapAccessScope::SetThreadLocalScope() { } bool MmapAccessScope::IsInsideBuffer(void* aPtr) { - bool isIn; - - if (mZipHandle) { - isIn = - aPtr >= mZipHandle->mFileStart && - aPtr < (void*)((char*)mZipHandle->mFileStart + mZipHandle->mTotalLen); - } else { - isIn = aPtr >= mBuf && aPtr < (void*)((char*)mBuf + mBufLen); - } - - return isIn; + return aPtr >= mBuf && aPtr < (void*)((char*)mBuf + mBufLen); } void MmapAccessScope::CrashWithInfo(void* aPtr) { - if (!mZipHandle) { - // All we have is the buffer and the crashing address. - MOZ_CRASH_UNSAFE_PRINTF( - "SIGBUS received when accessing mmaped zip file [buffer=%p, " - "buflen=%" PRIu32 ", address=%p]", - mBuf, mBufLen, aPtr); - } - - nsCOMPtr file = mZipHandle->mFile.GetBaseFile(); - nsCString fileName; - file->GetNativeLeafName(fileName); - - // Get current file size - int fileSize = -1; - if (PR_Seek64(mZipHandle->mNSPRFileDesc, 0, PR_SEEK_SET) != -1) { - fileSize = PR_Available64(mZipHandle->mNSPRFileDesc); - } - - // MOZ_CRASH_UNSAFE_PRINTF has limited number of arguments, so append fileSize - // to fileName - fileName.Append(", filesize="); - fileName.AppendInt(fileSize); - + // All we have is the buffer and the crashing address. MOZ_CRASH_UNSAFE_PRINTF( - "SIGBUS received when accessing mmaped zip file [file=%s, buffer=%p, " - "buflen=%" PRIu32 ", address=%p]", - fileName.get(), (char*)mZipHandle->mFileStart, mZipHandle->mTotalLen, - aPtr); + "SIGBUS received when accessing mmaped file [buffer=%p, " + "buflen=%u, address=%p, filename=%s]", + mBuf, mBufLen, aPtr, mFilename); } #endif diff --git a/modules/libjar/MmapFaultHandler.h b/mozglue/misc/MmapFaultHandler.h similarity index 69% rename from modules/libjar/MmapFaultHandler.h rename to mozglue/misc/MmapFaultHandler.h index b8f87bd97030..249c133db256 100644 --- a/modules/libjar/MmapFaultHandler.h +++ b/mozglue/misc/MmapFaultHandler.h @@ -36,18 +36,15 @@ #else // Linux -# include "mozilla/RefPtr.h" # include "mozilla/GuardObjects.h" # include # include -class nsZipHandle; - class MOZ_RAII MmapAccessScope { public: - MmapAccessScope(void* aBuf, uint32_t aBufLen); - explicit MmapAccessScope(nsZipHandle* aZipHandle); - ~MmapAccessScope(); + MFBT_API MmapAccessScope(void* aBuf, uint32_t aBufLen, + const char* aFilename = nullptr); + MFBT_API ~MmapAccessScope(); MmapAccessScope(const MmapAccessScope&) = delete; MmapAccessScope& operator=(const MmapAccessScope&) = delete; @@ -63,14 +60,26 @@ class MOZ_RAII MmapAccessScope { private: void* mBuf; + const char* mFilename; uint32_t mBufLen; - RefPtr mZipHandle; MmapAccessScope* mPreviousScope; }; -# define MMAP_FAULT_HANDLER_BEGIN_HANDLE(fd) \ - { \ - MmapAccessScope mmapScope(fd); \ +# define MMAP_FAULT_HANDLER_BEGIN_HANDLE(fd) \ + { \ + void* mmapScopeBuf = nullptr; \ + nsCString mmapScopeFilename; \ + uint32_t mmapScopeBufLen = 0; \ + if (fd && fd->mMap) { \ + mmapScopeBuf = (void*)fd->mFileStart; \ + mmapScopeBufLen = fd->mTotalLen; \ + } \ + if (fd && fd->mFile) { \ + nsCOMPtr file = fd->mFile.GetBaseFile(); \ + file->GetNativeLeafName(mmapScopeFilename); \ + } \ + MmapAccessScope mmapScope(mmapScopeBuf, mmapScopeBufLen, \ + mmapScopeFilename.get()); \ if (sigsetjmp(mmapScope.mJmpBuf, 0) == 0) { # define MMAP_FAULT_HANDLER_BEGIN_BUFFER(buf, bufLen) \ { \ diff --git a/mozglue/misc/moz.build b/mozglue/misc/moz.build index e859205f8ca1..b8129cdcc479 100644 --- a/mozglue/misc/moz.build +++ b/mozglue/misc/moz.build @@ -10,6 +10,7 @@ EXPORTS.mozilla += [ 'AutoProfilerLabel.h', 'decimal/Decimal.h', 'decimal/DoubleConversion.h', + 'MmapFaultHandler.h', 'PlatformConditionVariable.h', 'PlatformMutex.h', 'Printf.h', @@ -30,6 +31,7 @@ if CONFIG['OS_ARCH'] == 'WINNT': SOURCES += [ 'AutoProfilerLabel.cpp', + 'MmapFaultHandler.cpp', 'Printf.cpp', 'StackWalk.cpp', 'TimeStamp.cpp', diff --git a/startupcache/StartupCache.cpp b/startupcache/StartupCache.cpp index 1780ce8b8292..453e002a72d8 100644 --- a/startupcache/StartupCache.cpp +++ b/startupcache/StartupCache.cpp @@ -11,6 +11,7 @@ #include "mozilla/IOBuffers.h" #include "mozilla/MemoryReporting.h" #include "mozilla/MemUtils.h" +#include "mozilla/MmapFaultHandler.h" #include "mozilla/ResultExtensions.h" #include "mozilla/scache/StartupCache.h" #include "mozilla/ScopeExit.h" @@ -35,6 +36,10 @@ #include "nsIProtocolHandler.h" #include "GeckoProfiler.h" +#if defined(XP_WIN) +# include +#endif + #ifdef IS_BIG_ENDIAN # define SC_ENDIAN "big" #else @@ -254,6 +259,8 @@ Result StartupCache::LoadArchive() { auto data = mCacheData.get(); auto end = data + size; + MMAP_FAULT_HANDLER_BEGIN_BUFFER(data.get(), size) + if (memcmp(MAGIC, data.get(), sizeof(MAGIC))) { return Err(NS_ERROR_UNEXPECTED); } @@ -327,6 +334,8 @@ Result StartupCache::LoadArchive() { cleanup.release(); } + MMAP_FAULT_HANDLER_CATCH(Err(NS_ERROR_UNEXPECTED)) + return Ok(); } @@ -373,6 +382,8 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf, value.mData = MakeUnique(value.mUncompressedSize); Span uncompressed = MakeSpan(value.mData.get(), value.mUncompressedSize); + MMAP_FAULT_HANDLER_BEGIN_BUFFER(uncompressed.Elements(), + uncompressed.Length()) bool finished = false; while (!finished) { auto result = mDecompressionContext->Decompress( @@ -388,6 +399,8 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf, finished = decompressionResult.mFinished; } + MMAP_FAULT_HANDLER_CATCH(NS_ERROR_FAILURE) + label = Telemetry::LABELS_STARTUP_CACHE_REQUESTS::HitDisk; } @@ -642,8 +655,11 @@ void StartupCache::ThreadedPrefetch(void* aClosure) { NS_SetCurrentThreadName("StartupCache"); mozilla::IOInterposer::RegisterCurrentThread(); StartupCache* startupCacheObj = static_cast(aClosure); - PrefetchMemory(startupCacheObj->mCacheData.get().get(), - startupCacheObj->mCacheData.size()); + uint8_t* buf = startupCacheObj->mCacheData.get().get(); + size_t size = startupCacheObj->mCacheData.size(); + MMAP_FAULT_HANDLER_BEGIN_BUFFER(buf, size) + PrefetchMemory(buf, size); + MMAP_FAULT_HANDLER_CATCH() mozilla::IOInterposer::UnregisterCurrentThread(); }