Medium firefox Race 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionPotential race conditions in IndexedDB could have caused memory corruption, leading to a potentially exploitable crash.
ComponentDOM
Bug ClassRace
Tracker1914982
Fix commit043fae320084 (firefox) +47/-32
CISA KEVNot listed
CreditedTyson Smith
Disclosed2024-10-29

Changed Functions

FunctionChangeNotes
if
dom/indexedDB/ActorsParent.cpp
modified
if
dom/indexedDB/FileInfoManager.h
modified
if
dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
modified
for
dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
modified

Files Changed

  • dom/indexedDB/ActorsParent.cpp
  • dom/indexedDB/DatabaseFileManager.h
  • dom/indexedDB/FileInfoManager.h
  • dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp
index 9579a76ffbe..a55770bd003 100644
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -6725,7 +6725,7 @@ RefPtr<mozilla::dom::quota::Client> CreateQuotaClient() {
 
 nsresult DatabaseFileManager::AsyncDeleteFile(int64_t aFileId) {
   AssertIsOnBackgroundThread();
-  MOZ_ASSERT(!mFileInfos.Contains(aFileId));
+  MOZ_ASSERT(!ContainsFileInfo(aFileId));
 
   QuotaClient* quotaClient = QuotaClient::GetInstance();
   if (quotaClient) {
@@ -11703,12 +11703,8 @@ nsresult DatabaseFileManager::Init(nsIFile* aDirectory,
         // be 0, but the dbRefCnt is non-zero, which will keep the
         // DatabaseFileInfo object alive.
         MOZ_ASSERT(dbRefCnt > 0);
-        mFileInfos.InsertOrUpdate(
-            id, MakeNotNull<DatabaseFileInfo*>(
-                    FileInfoManagerGuard{}, SafeRefPtrFromThis(), id,
-                    static_cast<nsrefcnt>(dbRefCnt)));
-
-        mLastFileId = std::max(id, mLastFileId);
+        DebugOnly ok = static_cast<bool>(CreateFileInfo(Some(id), dbRefCnt));
+        MOZ_ASSERT(ok);
 
         return Ok{};
       }));
@@ -11971,7 +11967,7 @@ Result<FileUsageType, nsresult> DatabaseFileManager::GetUsage(
 }
 
 nsresult DatabaseFileManager::SyncDeleteFile(const int64_t aId) {
-  MOZ_ASSERT(!mFileInfos.Contains(aId));
+  MOZ_ASSERT(!ContainsFileInfo(aId));
 
   if (!this->AssertValid()) {
     return NS_ERROR_UNEXPECTED;
diff --git a/dom/indexedDB/DatabaseFileManager.h b/dom/indexedDB/DatabaseFileManager.h
index fdfea06ba11..a368d5f4ee2 100644
--- a/dom/indexedDB/DatabaseFileManager.h
+++ b/dom/indexedDB/DatabaseFileManager.h
@@ -131,7 +131,9 @@ class DatabaseFileManager final
 
   MOZ_DECLARE_REFCOUNTED_TYPENAME(DatabaseFileManager)
 
-  static StaticMutex& Mutex() { return sMutex; }
+  static StaticMutex& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
   ~DatabaseFileManager() = default;
 };
diff --git a/dom/indexedDB/FileInfoManager.h b/dom/indexedDB/FileInfoManager.h
index 75842aa53fc..68f8786ca9e 100644
--- a/dom/indexedDB/FileInfoManager.h
+++ b/dom/indexedDB/FileInfoManager.h
@@ -46,27 +46,43 @@ class FileInfoManager : public FileInfoManagerBase {
   using AutoLockType = mozilla::detail::BaseAutoLock<MutexType&>;
 
   [[nodiscard]] SafeRefPtr<FileInfoType> GetFileInfo(int64_t aId) const {
-    return AcquireFileInfo([this, aId] { return mFileInfos.MaybeGet(aId); });
+    return AcquireFileInfo([this, aId]() MOZ_REQUIRES(Mutex()) {
+      return mFileInfos.MaybeGet(aId);
+    });
+  }
+
+  bool ContainsFileInfo(int64_t aFileId) {
+    AutoLockType lock(Mutex());
+    return mFileInfos.Contains(aFileId);
   }
 
-  [[nodiscard]] SafeRefPtr<FileInfoType> CreateFileInfo() {
-    return AcquireFileInfo([this] {
-      const int64_t id = ++mLastFileId;
+  [[nodiscard]] SafeRefPtr<FileInfoType> CreateFileInfo(
+      const Maybe<int64_t>& aMaybeId = Nothing(),
+      const nsrefcnt aDBRefCnt = 0) {
+    return AcquireFileInfo([this, &aMaybeId,
+                            &aDBRefCnt]() MOZ_REQUIRES(Mutex()) {
+      const int64_t id = aMaybeId.isSome() ? *aMaybeId : ++mLastFileId;
 
       auto fileInfo =
           MakeNotNull<FileInfoType*>(FileInfoManagerGuard{},
                                      SafeRefPtr{static_cast<FileManager*>(this),
                                                 AcquireStrongRefFromRawPtr{}},
-                                     id);
+                                     id, aDBRefCnt);
 
       mFileInfos.InsertOrUpdate(id, fileInfo);
+
+      if (aMaybeId.isSome()) {
+        mLastFileId = std::max(id, mLastFileId);
+      }
+
       return Some(fileInfo);
     });
   }
 
-  void RemoveFileInfo(const int64_t aId, const AutoLockType& aFileMutexLock) {
+  void RemoveFileInfo(const int64_t aId, const AutoLockType& aFileMutexLock)
+      MOZ_REQUIRES(Mutex()) {
 #ifdef DEBUG
-    aFileMutexLock.AssertOwns(FileManager::Mutex());
+    aFileMutexLock.AssertOwns(Mutex());
 #endif
     mFileInfos.Remove(aId);
   }
@@ -74,7 +90,7 @@ class FileInfoManager : public FileInfoManagerBase {
   // After calling this method, callers should not call any more methods on this
   // class.
   virtual nsresult Invalidate() {
-    AutoLockType lock(FileManager::Mutex());
+    AutoLockType lock(Mutex());
 
     FileInfoManagerBase::Invalidate();
 
@@ -92,6 +108,8 @@ class FileInfoManager : public FileInfoManagerBase {
     FileInfoManagerGuard() = default;
   };
 
+  static MutexType& Mutex() { return FileManager::MutexInstance(); }
+
  private:
   // Runs the given aFileInfoTableOp operation, which must return a FileInfo*,
   // under the FileManager lock, acquires a strong reference to the returned
@@ -107,7 +125,7 @@ class FileInfoManager : public FileInfoManagerBase {
     // We cannot simply change this to SafeRefPtr<FileInfo>, because
     // FileInfo::AddRef also acquires the FileManager::Mutex.
     auto fileInfo = [&aFileInfoTableOp]() -> RefPtr<FileInfoType> {
-      AutoLockType lock(FileManager::Mutex());
+      AutoLockType lock(Mutex());
 
       const auto maybeFileInfo = aFileInfoTableOp();
       if (maybeFileInfo) {
@@ -122,17 +140,19 @@ class FileInfoManager : public FileInfoManagerBase {
     return SafeRefPtr{std::move(fileInfo)};
   }
 
+  // Access to the following private fields must be protected by
+  // FileManager::Mutex() which is now enforced by MOZ_GUARDED_BY annotations.
+  nsTHashMap<nsUint64HashKey, NotNull<FileInfoType*>> mFileInfos
+      MOZ_GUARDED_BY(Mutex());
+
+  int64_t mLastFileId MOZ_GUARDED_BY(Mutex()) = 0;
+
  protected:
 #ifdef DEBUG
   ~FileInfoManager() { MOZ_ASSERT(mFileInfos.IsEmpty()); }
 #else
   ~FileInfoManager() = default;
 #endif
-
-  // Access to the following fields must be protected by
-  // FileManager::Mutex()
-  int64_t mLastFileId = 0;
-  nsTHashMap<nsUint64HashKey, NotNull<FileInfoType*>> mFileInfos;
 };
 
 }  // namespace mozilla::dom::indexedDB
diff --git a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
index 8971ee54fbe..3239241cde4 100644
--- a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
+++ b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
@@ -38,7 +38,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   // SimpleFileManager functions that are used by SimpleFileInfo
 
   [[nodiscard]] nsresult AsyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mAsyncDeleteFileCalls;
@@ -48,7 +48,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   }
 
   [[nodiscard]] nsresult SyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mSyncDeleteFileCalls;
@@ -64,16 +64,13 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
     for (const auto id : kDBOnlyFileInfoIds) {
       // Copied from within DatabaseFileManager::Init.
 
-      mFileInfos.InsertOrUpdate(
-          id, MakeNotNull<SimpleFileInfo*>(FileInfoManagerGuard{},
-                                           SafeRefPtrFromThis(), id,
-                                           static_cast<nsrefcnt>(1)));
-
-      mLastFileId = std::max(id, mLastFileId);
+      MOZ_RELEASE_ASSERT(CreateFileInfo(Some(id), static_cast<nsrefcnt>(1)));
     }
   }
 
-  static MutexType& Mutex() { return sMutex; }
+  static MutexType& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
index 8971ee54fbe..3239241cde4 100644
--- a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
+++ b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
@@ -38,7 +38,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   // SimpleFileManager functions that are used by SimpleFileInfo
 
   [[nodiscard]] nsresult AsyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mAsyncDeleteFileCalls;
@@ -48,7 +48,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   }
 
   [[nodiscard]] nsresult SyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mSyncDeleteFileCalls;
@@ -64,16 +64,13 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
     for (const auto id : kDBOnlyFileInfoIds) {
       // Copied from within DatabaseFileManager::Init.
 
-      mFileInfos.InsertOrUpdate(
-          id, MakeNotNull<SimpleFileInfo*>(FileInfoManagerGuard{},
-                                           SafeRefPtrFromThis(), id,
-                                           static_cast<nsrefcnt>(1)));
-
-      mLastFileId = std::max(id, mLastFileId);
+      MOZ_RELEASE_ASSERT(CreateFileInfo(Some(id), static_cast<nsrefcnt>(1)));
     }
   }
 
-  static MutexType& Mutex() { return sMutex; }
+  static MutexType& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
   static constexpr auto kDBOnlyFileInfoIds =
       std::array<int64_t, 3>{{10, 20, 30}};
Loading diff…