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

@@ -30,7 +30,6 @@ EXPORTS += [
 ]
 
 UNIFIED_SOURCES += [
-    'MmapFaultHandler.cpp',
     'nsJAR.cpp',
     'nsJARChannel.cpp',
     'nsJARInputStream.cpp',

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"

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"

mozglue/misc/MmapFaultHandler.cpp

@@ -8,12 +8,13 @@
 
 #if defined(XP_UNIX) && !defined(XP_DARWIN)
 
-#  include "nsZipArchive.h"
+#  include "PlatformMutex.h"
 #  include "mozilla/Atomics.h"
-#  include "mozilla/StaticMutex.h"
-#  include "MainThreadUtils.h"
+#  include "mozilla/GuardObjects.h"
+#  include "mozilla/MemoryChecking.h"
 #  include "mozilla/ThreadLocal.h"
 #  include <signal.h>
+#  include <cstring>
 
 static MOZ_THREAD_LOCAL(MmapAccessScope*) sMmapAccessScope;
 
@@ -31,7 +32,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 +49,7 @@ static void MmapSIGBUSHandler(int signum, siginfo_t* info, void* context) {
 }
 
 mozilla::Atomic<bool> gSIGBUSHandlerInstalled(false);
-mozilla::StaticMutex gSIGBUSHandlerMutex;
+mozilla::Atomic<bool> gSIGBUSHandlerInstalling(false);
 
 void InstallMmapFaultHandler() {
   // This function is called from MmapAccessScope's constructor because there is
@@ -60,50 +60,39 @@ void InstallMmapFaultHandler() {
     return;
   }
 
-  mozilla::StaticMutexAutoLock lock(gSIGBUSHandlerMutex);
+  if (gSIGBUSHandlerInstalling.compareExchange(false, true)) {
+    sMmapAccessScope.infallibleInit();
 
-  // We must check it again, because the handler could be installed on another
-  // thread when we were waiting for the lock.
-  if (gSIGBUSHandlerInstalled) {
-    return;
+    struct sigaction busHandler;
+    busHandler.sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK;
+    busHandler.sa_sigaction = MmapSIGBUSHandler;
+    sigemptyset(&busHandler.sa_mask);
+    if (sigaction(SIGBUS, &busHandler, &sPrevSIGBUSHandler)) {
+      MOZ_CRASH("Unable to install SIGBUS handler");
+    }
+
+    MOZ_ASSERT(!gSIGBUSHandlerInstalled);
+    gSIGBUSHandlerInstalled = true;
+  } else {
+    // Just spin lock here. It should not take a substantial amount
+    // of time, so a mutex would likely be a spin lock anyway, and
+    // this avoids the need to new up a static mutex from within
+    // mozglue/misc, which complicates things with
+    // check_vanilla_allocations.py
+    while (!gSIGBUSHandlerInstalled) {
+    }
   }
-
-  sMmapAccessScope.infallibleInit();
-
-  struct sigaction busHandler;
-  busHandler.sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK;
-  busHandler.sa_sigaction = MmapSIGBUSHandler;
-  sigemptyset(&busHandler.sa_mask);
-  if (sigaction(SIGBUS, &busHandler, &sPrevSIGBUSHandler)) {
-    MOZ_CRASH("Unable to install SIGBUS handler");
-  }
-
-  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 +118,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<nsIFile> 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

mozglue/misc/MmapFaultHandler.h

@@ -36,18 +36,15 @@
 #else
 // Linux
 
-#  include "mozilla/RefPtr.h"
 #  include "mozilla/GuardObjects.h"
 #  include <stdint.h>
 #  include <setjmp.h>
 
-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,32 @@ class MOZ_RAII MmapAccessScope {
 
  private:
   void* mBuf;
+  const char* mFilename;
   uint32_t mBufLen;
-  RefPtr<nsZipHandle> mZipHandle;
   MmapAccessScope* mPreviousScope;
 };
 
-#  define MMAP_FAULT_HANDLER_BEGIN_HANDLE(fd) \
-    {                                         \
-      MmapAccessScope mmapScope(fd);          \
+// Gets around warnings for null-checking in a macro.
+template <typename T>
+inline bool ValidFD(T fd) {
+  return !!fd;
+}
+
+#  define MMAP_FAULT_HANDLER_BEGIN_HANDLE(fd)                  \
+    {                                                          \
+      void* mmapScopeBuf = nullptr;                            \
+      nsCString mmapScopeFilename;                             \
+      uint32_t mmapScopeBufLen = 0;                            \
+      if (ValidFD(fd) && fd->mMap) {                           \
+        mmapScopeBuf = (void*)fd->mFileStart;                  \
+        mmapScopeBufLen = fd->mTotalLen;                       \
+      }                                                        \
+      if (ValidFD(fd) && fd->mFile) {                          \
+        nsCOMPtr<nsIFile> 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)   \
     {                                                    \

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',

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"
@@ -34,6 +35,10 @@
 #include "nsIProtocolHandler.h"
 #include "GeckoProfiler.h"
 
+#if defined(XP_WIN)
+#  include <windows.h>
+#endif
+
 #ifdef IS_BIG_ENDIAN
 #  define SC_ENDIAN "big"
 #else
@@ -256,6 +261,8 @@ Result<Ok, nsresult> StartupCache::LoadArchive() {
   auto data = mCacheData.get<uint8_t>();
   auto end = data + size;
 
+  MMAP_FAULT_HANDLER_BEGIN_BUFFER(data.get(), size)
+
   if (memcmp(MAGIC, data.get(), sizeof(MAGIC))) {
     return Err(NS_ERROR_UNEXPECTED);
   }
@@ -329,6 +336,8 @@ Result<Ok, nsresult> StartupCache::LoadArchive() {
     cleanup.release();
   }
 
+  MMAP_FAULT_HANDLER_CATCH(Err(NS_ERROR_UNEXPECTED))
+
   return Ok();
 }
 
@@ -375,6 +384,8 @@ nsresult StartupCache::GetBuffer(const char* id, const char** outbuf,
     value.mData = MakeUnique<char[]>(value.mUncompressedSize);
     Span<char> 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(
@@ -390,6 +401,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;
   }
 
@@ -644,8 +657,11 @@ void StartupCache::ThreadedPrefetch(void* aClosure) {
   NS_SetCurrentThreadName("StartupCache");
   mozilla::IOInterposer::RegisterCurrentThread();
   StartupCache* startupCacheObj = static_cast<StartupCache*>(aClosure);
-  PrefetchMemory(startupCacheObj->mCacheData.get<uint8_t>().get(),
-                 startupCacheObj->mCacheData.size());
+  uint8_t* buf = startupCacheObj->mCacheData.get<uint8_t>().get();
+  size_t size = startupCacheObj->mCacheData.size();
+  MMAP_FAULT_HANDLER_BEGIN_BUFFER(buf, size)
+  PrefetchMemory(buf, size);
+  MMAP_FAULT_HANDLER_CATCH()
   mozilla::IOInterposer::UnregisterCurrentThread();
 }