Firefox · DOM
CVE-2024-10468
Race in DOM
Overview
Medium
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Changed Functions
| Function | Change | Notes |
|---|---|---|
ifdom/indexedDB/ActorsParent.cpp |
modified | |
ifdom/indexedDB/FileInfoManager.h |
modified | |
ifdom/indexedDB/test/gtest/TestSimpleFileInfo.cpp |
modified | |
fordom/indexedDB/test/gtest/TestSimpleFileInfo.cpp |
modified |
Files Changed
dom/indexedDB/ActorsParent.cppdom/indexedDB/DatabaseFileManager.hdom/indexedDB/FileInfoManager.hdom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
Patch
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…
References
On This Page