Skip to content

Commit

Permalink
Cherry-pick 64bcd93. rdar://117463447
Browse files Browse the repository at this point in the history 
    jsc_fuz/wktr: heap-use-after-free in WebCore::IDBServer::MemoryObjectStore::takeIndexByIdentifier(unsigned long long) MemoryObjectStore.cpp:128.
    https://bugs.webkit.org/show_bug.cgi?id=264180.
    rdar://117463447.

    Reviewed by Sihui Liu.

    MemoryIndex now keeps WeakPtr to MemoryObjectStore 'm_objectStore' and checks it's validity before using it. Also RefPtr conversion from WekPtr using get() API as applicable.

    * LayoutTests/storage/indexeddb/abort-index-rename-crash-expected.txt: Added the test expected file.
    * LayoutTests/storage/indexeddb/abort-index-rename-crash.html: Added the test case.
    * Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp: Checks the validity of MemoryObjectStore pointer before using.
    (WebCore::IDBServer::MemoryBackingStoreTransaction::objectStoreDeleted):
    (WebCore::IDBServer::MemoryBackingStoreTransaction::indexRenamed):
    (WebCore::IDBServer::MemoryBackingStoreTransaction::abort):
    * Source/WebCore/Modules/indexeddb/server/MemoryIndex.cpp: Changed direct reference to WeakPtr. Also used RefPtr conversion using get() API as applicable.
    (WebCore::IDBServer::MemoryIndex::objectStoreCleared):
    (WebCore::IDBServer::MemoryIndex::clearIndexValueStore):
    (WebCore::IDBServer::MemoryIndex::replaceIndexValueStore):
    (WebCore::IDBServer::MemoryIndex::getResultForKeyRange const):
    (WebCore::IDBServer::MemoryIndex::getAllRecords const):
    * Source/WebCore/Modules/indexeddb/server/MemoryIndex.h: Changed direct reference to WeakPtr.
    (WebCore::IDBServer::MemoryIndex::objectStore):
    * Source/WebCore/Modules/indexeddb/server/MemoryIndexCursor.cpp: Used RefPtr conversion using get() API for MemoryIndex based MemoryObjectStore object.
    (WebCore::IDBServer::MemoryIndexCursor::currentData):
    * Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.h:

    Canonical link: https://commits.webkit.org/267815.545@safari-7617-branch
  • Loading branch information
@MyahCobbs
MyahCobbs committed Nov 8, 2023
1 parent e7e6ce5 commit c9941f3
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 26 deletions.
1 change: 1 addition & 0 deletions LayoutTests/storage/indexeddb/abort-index-rename-crash-expected.txt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This test passes if it doesn't crash.
27 changes: 27 additions & 0 deletions LayoutTests/storage/indexeddb/abort-index-rename-crash.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<!-- webkit-test-runner [ useEphemeralSession=true ] -->
<body>
<script>
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}
indexedDB.open('abort-index-rename').onupgradeneeded = (event) => {
request = event.target;
transaction = request.transaction;
transaction.onabort = () => {
if (window.testRunner)
testRunner.notifyDone();
}
database = request.result;
objectStore = database.createObjectStore('os');
index = objectStore.createIndex('foo', 'foo');
index.name = 'bar';
database.deleteObjectStore('os');
objectStore2 = database.createObjectStore('os2');
objectStore2.add('value', 'key').onsuccess = () => {
transaction.abort();
}
};
</script>
<p>This test passes if it doesn't crash.</p>
</body>
27 changes: 14 additions & 13 deletions Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ void MemoryBackingStoreTransaction::objectStoreDeleted(Ref<MemoryObjectStore>&&
if (auto addedObjectStore = m_versionChangeAddedObjectStores.take(&objectStore.get())) {
// We don't need to track its indexes either.
m_deletedIndexes.removeIf([identifier = objectStore->info().identifier()](auto& entry) {
return entry.value->objectStore().info().identifier() == identifier;
return entry.value->objectStore()->info().identifier() == identifier;
});
return;
}
Expand Down Expand Up @@ -162,7 +162,7 @@ void MemoryBackingStoreTransaction::objectStoreRenamed(MemoryObjectStore& object

void MemoryBackingStoreTransaction::indexRenamed(MemoryIndex& index, const String& oldName)
{
ASSERT(m_objectStores.contains(&index.objectStore()));
ASSERT(!index.objectStore() || m_objectStores.contains(index.objectStore().get()));
ASSERT(m_info.mode() == IDBTransactionMode::Versionchange);

// We only care about the first rename in a given transaction, because if the transaction is aborted we want
Expand Down Expand Up @@ -225,15 +225,16 @@ void MemoryBackingStoreTransaction::abort()
break;
}
}
if (indexToDelete)
indexToDelete->objectStore().deleteIndex(*this, indexToDelete->info().identifier());

auto& objectStore = index->objectStore();
auto indexToReRegister = objectStore.takeIndexByIdentifier(identifier).releaseNonNull();
objectStore.info().deleteIndex(identifier);
index->rename(originalName);
objectStore.info().addExistingIndex(index->info());
objectStore.registerIndex(WTFMove(indexToReRegister));
if (indexToDelete && indexToDelete->objectStore())
indexToDelete->objectStore()->deleteIndex(*this, indexToDelete->info().identifier());

if (auto objectStore = index->objectStore()) {
auto indexToReRegister = objectStore->takeIndexByIdentifier(identifier).releaseNonNull();
objectStore->info().deleteIndex(identifier);
index->rename(originalName);
objectStore->info().addExistingIndex(index->info());
objectStore->registerIndex(WTFMove(indexToReRegister));
}
}
m_originalIndexNames.clear();

Expand All @@ -244,7 +245,7 @@ void MemoryBackingStoreTransaction::abort()
for (const auto& objectStore : m_versionChangeAddedObjectStores)
m_backingStore.removeObjectStoreForVersionChangeAbort(*objectStore);
m_deletedIndexes.removeIf([&](auto& entry) {
return m_versionChangeAddedObjectStores.contains(&entry.value->objectStore());
return m_versionChangeAddedObjectStores.contains(entry.value->objectStore().get());
});
m_versionChangeAddedObjectStores.clear();

Expand Down Expand Up @@ -288,7 +289,7 @@ void MemoryBackingStoreTransaction::abort()

for (auto& index : m_deletedIndexes.values()) {
RELEASE_ASSERT(m_backingStore.hasObjectStore(index->info().objectStoreIdentifier()));
index->objectStore().maybeRestoreDeletedIndex(*index);
index->objectStore()->maybeRestoreDeletedIndex(*index);
}
m_deletedIndexes.clear();

Expand Down
16 changes: 8 additions & 8 deletions Source/WebCore/Modules/indexeddb/server/MemoryIndex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ void MemoryIndex::cursorDidBecomeDirty(MemoryIndexCursor& cursor)

void MemoryIndex::objectStoreCleared()
{
auto transaction = m_objectStore.writeTransaction();
auto transaction = m_objectStore->writeTransaction();
ASSERT(transaction);

transaction->indexCleared(*this, WTFMove(m_records));
Expand All @@ -89,16 +89,16 @@ void MemoryIndex::notifyCursorsOfAllRecordsChanged()

void MemoryIndex::clearIndexValueStore()
{
ASSERT(m_objectStore.writeTransaction());
ASSERT(m_objectStore.writeTransaction()->isAborting());
ASSERT(m_objectStore->writeTransaction());
ASSERT(m_objectStore->writeTransaction()->isAborting());

m_records = nullptr;
}

void MemoryIndex::replaceIndexValueStore(std::unique_ptr<IndexValueStore>&& valueStore)
{
ASSERT(m_objectStore.writeTransaction());
ASSERT(m_objectStore.writeTransaction()->isAborting());
ASSERT(m_objectStore->writeTransaction());
ASSERT(m_objectStore->writeTransaction()->isAborting());

m_records = WTFMove(valueStore);
}
Expand All @@ -124,7 +124,7 @@ IDBGetResult MemoryIndex::getResultForKeyRange(IndexedDB::IndexRecordType type,
if (!keyValue)
return { };

return type == IndexedDB::IndexRecordType::Key ? IDBGetResult(*keyValue) : IDBGetResult(*keyValue, m_objectStore.valueForKeyRange(*keyValue), m_objectStore.info().keyPath());
return type == IndexedDB::IndexRecordType::Key ? IDBGetResult(*keyValue) : IDBGetResult(*keyValue, m_objectStore->valueForKeyRange(*keyValue), m_objectStore->info().keyPath());
}

uint64_t MemoryIndex::countForKeyRange(const IDBKeyRangeData& inRange)
Expand Down Expand Up @@ -154,7 +154,7 @@ void MemoryIndex::getAllRecords(const IDBKeyRangeData& keyRangeData, std::option
{
LOG(IndexedDB, "MemoryIndex::getAllRecords");

result = { type, m_objectStore.info().keyPath() };
result = { type, m_objectStore->info().keyPath() };

if (!m_records)
return;
Expand All @@ -179,7 +179,7 @@ void MemoryIndex::getAllRecords(const IDBKeyRangeData& keyRangeData, std::option
for (auto& keyValue : allValues) {
result.addKey(IDBKeyData(keyValue));
if (type == IndexedDB::GetAllType::Values)
result.addValue(m_objectStore.valueForKeyRange(keyValue));
result.addValue(m_objectStore->valueForKeyRange(keyValue));
}

currentCount += allValues.size();
Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/Modules/indexeddb/server/MemoryIndex.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class MemoryIndex : public RefCounted<MemoryIndex> {

IndexValueStore* valueStore() { return m_records.get(); }

MemoryObjectStore& objectStore() { return m_objectStore; }
WeakPtr<MemoryObjectStore> objectStore() { return m_objectStore; }

void cursorDidBecomeClean(MemoryIndexCursor&);
void cursorDidBecomeDirty(MemoryIndexCursor&);
Expand All @@ -96,7 +96,7 @@ class MemoryIndex : public RefCounted<MemoryIndex> {
void notifyCursorsOfAllRecordsChanged();

IDBIndexInfo m_info;
MemoryObjectStore& m_objectStore;
WeakPtr<MemoryObjectStore> m_objectStore;

std::unique_ptr<IndexValueStore> m_records;

Expand Down
4 changes: 2 additions & 2 deletions Source/WebCore/Modules/indexeddb/server/MemoryIndexCursor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ void MemoryIndexCursor::currentData(IDBGetResult& getResult)
if (m_info.cursorType() == IndexedDB::CursorType::KeyOnly)
getResult = { m_currentKey, m_currentPrimaryKey };
else {
IDBValue value = { m_index.objectStore().valueForKey(m_currentPrimaryKey), { }, { } };
getResult = { m_currentKey, m_currentPrimaryKey, WTFMove(value), m_index.objectStore().info().keyPath() };
IDBValue value = { m_index.objectStore()->valueForKey(m_currentPrimaryKey), { }, { } };
getResult = { m_currentKey, m_currentPrimaryKey, WTFMove(value), m_index.objectStore()->info().keyPath() };
}
}

Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class MemoryBackingStoreTransaction;

typedef HashMap<IDBKeyData, ThreadSafeDataBuffer, IDBKeyDataHash, IDBKeyDataHashTraits> KeyValueMap;

class MemoryObjectStore : public RefCounted<MemoryObjectStore> {
class MemoryObjectStore : public RefCounted<MemoryObjectStore>, public CanMakeWeakPtr<MemoryObjectStore> {
public:
static Ref<MemoryObjectStore> create(const IDBObjectStoreInfo&);

Expand Down

0 comments on commit c9941f3

Please sign in to comment.