From af3813782262a75ad970e90b1f465acc139157b5 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Sun, 25 Jun 2023 16:42:48 +0200 Subject: [PATCH 1/4] Fix possible use after free in plRegistryPageNode::UnloadKeys Short explanation: the destructor of plRegistryKeyList may indirectly access other entries of fKeyLists where the plRegistryKeyList has already been deleted, but not yet removed from the map. Long explanation: * Deleting a plRegistryKeyList also deletes all plKeys inside it, which decrements the reference count of the objects they point to. * If one of the deleted keys happens to be the last reference to an object, this also deletes the object itself. * The object's destructor might in turn delete another plKey, which calls SetKeyUnused, which tries to look up the key in its page. * If this second plKey belongs to the page that is currently being unloaded, then its plRegistryKeyList may be partially or completely deleted, but still listed in the fKeyLists map. In this case, the key lookup accesses already freed memory. (ported from H-uru/Plasma@a529e35fd940543752fd74efd0fe63039a03c4a6) --- Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp | 4 +++- Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp index 58fcc8c2..cc6d35a0 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp @@ -59,8 +59,10 @@ plRegistryKeyList::~plRegistryKeyList() for (int i = 0; i < fStaticKeys.size(); i++) { plKeyImp* keyImp = fStaticKeys[i]; - if (!keyImp->ObjectIsLoaded()) + if (keyImp && !keyImp->ObjectIsLoaded()) { delete keyImp; + keyImp = nullptr; + } } } diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp index 8c8954b5..847645c5 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp @@ -204,6 +204,7 @@ void plRegistryPageNode::UnloadKeys() { plRegistryKeyList* keyList = it->second; delete keyList; + it->second = nullptr; } fKeyLists.clear(); From 5d5ba00f7d057072ebc607b0e693b89540e81401 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Sun, 25 Jun 2023 16:49:58 +0200 Subject: [PATCH 2/4] Also guard against accessing plRegistryKeyList currently being deleted Co-authored-by: Adam Johnson (ported from H-uru/Plasma@725eeaa28820423f8f36e3476d0f5c6da30921c2) --- Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp | 2 +- Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp index cc6d35a0..6f292d59 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp @@ -60,8 +60,8 @@ plRegistryKeyList::~plRegistryKeyList() { plKeyImp* keyImp = fStaticKeys[i]; if (keyImp && !keyImp->ObjectIsLoaded()) { - delete keyImp; keyImp = nullptr; + delete keyImp; } } } diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp index 847645c5..c0eacdcb 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryNode.cpp @@ -203,8 +203,8 @@ void plRegistryPageNode::UnloadKeys() for (; it != fKeyLists.end(); it++) { plRegistryKeyList* keyList = it->second; - delete keyList; it->second = nullptr; + delete keyList; } fKeyLists.clear(); From ea7e4b2ab54872f59234fe1370aa83b84e711c40 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Mon, 26 Jun 2023 00:09:03 +0200 Subject: [PATCH 3/4] Fix previous commits not actually nulling out the key I goofed while porting this from H'uru, where the variable is a reference. The previous code actually caused the keys to not get deleted at all. --- Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp index 6f292d59..8ddcb962 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plRegistryKeyList.cpp @@ -60,7 +60,7 @@ plRegistryKeyList::~plRegistryKeyList() { plKeyImp* keyImp = fStaticKeys[i]; if (keyImp && !keyImp->ObjectIsLoaded()) { - keyImp = nullptr; + fStaticKeys[i] = nullptr; delete keyImp; } } From 7d3774a732a0cecd6655499eec529e86567f3c27 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Thu, 29 Jun 2023 22:03:53 +0200 Subject: [PATCH 4/4] Unload all keys of all pages before deleting them (ported from H-uru/Plasma@04d0ac94ea25fc937931b8f4f546220ddf1cc27c) --- .../Plasma/PubUtilLib/plResMgr/plResManager.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/Sources/Plasma/PubUtilLib/plResMgr/plResManager.cpp b/Sources/Plasma/PubUtilLib/plResMgr/plResManager.cpp index cb30f6cb..bd7edfd6 100644 --- a/Sources/Plasma/PubUtilLib/plResMgr/plResManager.cpp +++ b/Sources/Plasma/PubUtilLib/plResMgr/plResManager.cpp @@ -219,14 +219,20 @@ void plResManager::IShutdown() // Shut down the registry (finally!) ILockPages(); - PageSet::const_iterator it; - for (it = fAllPages.begin(); it != fAllPages.end(); it++) + // Unload all keys before actually deleting the pages. + // When a key's refcount drops to zero, IKeyUnreffed looks up the key's page. + // If the page is already deleted at that point, this causes a use after free and potential crash. + for (PageSet::const_iterator it = fAllPages.begin(); it != fAllPages.end(); it++) { + (*it)->UnloadKeys(); + } + fLoadedPages.clear(); + fLastFoundPage = nil; + for (PageSet::const_iterator it = fAllPages.begin(); it != fAllPages.end(); it++) { delete *it; + } fAllPages.clear(); - fLoadedPages.clear(); IUnlockPages(); - fLastFoundPage = nil; // Now, kill off the Dispatcher hsRefCnt_SafeUnRef(fDispatch);