Make LiveCellRef to remove itself from PtrRegistry in destructor (to prevent potential use after free); Update ContainerStore::mPtr after copying container/actor.

apps/openmw/engine.cpp

@@ -740,6 +740,9 @@ void OMW::Engine::prepareEngine()
     // Create the world
     mWorld = std::make_unique<MWWorld::World>(
         mResourceSystem.get(), mActivationDistanceOverride, mCellName, mCfgMgr.getUserDataPath());
+    mEnvironment.setWorld(*mWorld);
+    mEnvironment.setWorldModel(mWorld->getWorldModel());
+    mEnvironment.setESMStore(mWorld->getStore());
 
     Loading::Listener* listener = MWBase::Environment::get().getWindowManager()->getLoadingScreen();
     Loading::AsyncListener asyncListener(*listener);
@@ -763,12 +766,9 @@ void OMW::Engine::prepareEngine()
     listener->loadingOff();
 
     mWorld->init(mViewer, rootNode, mWorkQueue.get(), *mUnrefQueue);
+    mEnvironment.setWorldScene(mWorld->getWorldScene());
     mWorld->setupPlayer();
     mWorld->setRandomSeed(mRandomSeed);
-    mEnvironment.setWorld(*mWorld);
-    mEnvironment.setWorldModel(mWorld->getWorldModel());
-    mEnvironment.setWorldScene(mWorld->getWorldScene());
-    mEnvironment.setESMStore(mWorld->getStore());
 
     const MWWorld::Store<ESM::GameSetting>* gmst = &mWorld->getStore().get<ESM::GameSetting>();
     mL10nManager->setGmstLoader(

apps/openmw/mwclass/container.cpp

@@ -302,8 +302,13 @@ namespace MWClass
     MWWorld::Ptr Container::copyToCellImpl(const MWWorld::ConstPtr& ptr, MWWorld::CellStore& cell) const
     {
         const MWWorld::LiveCellRef<ESM::Container>* ref = ptr.get<ESM::Container>();
-
-        return MWWorld::Ptr(cell.insert(ref), &cell);
+        MWWorld::Ptr newPtr(cell.insert(ref), &cell);
+        if (newPtr.getRefData().getCustomData())
+        {
+            MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
+            newPtr.getContainerStore()->setPtr(newPtr);
+        }
+        return newPtr;
     }
 
     void Container::readAdditionalState(const MWWorld::Ptr& ptr, const ESM::ObjectState& state) const

apps/openmw/mwclass/creature.cpp

@@ -694,8 +694,13 @@ namespace MWClass
     MWWorld::Ptr Creature::copyToCellImpl(const MWWorld::ConstPtr& ptr, MWWorld::CellStore& cell) const
     {
         const MWWorld::LiveCellRef<ESM::Creature>* ref = ptr.get<ESM::Creature>();
-
-        return MWWorld::Ptr(cell.insert(ref), &cell);
+        MWWorld::Ptr newPtr(cell.insert(ref), &cell);
+        if (newPtr.getRefData().getCustomData())
+        {
+            MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
+            newPtr.getContainerStore()->setPtr(newPtr);
+        }
+        return newPtr;
     }
 
     bool Creature::isBipedal(const MWWorld::ConstPtr& ptr) const

apps/openmw/mwclass/npc.cpp

@@ -1334,8 +1334,13 @@ namespace MWClass
     MWWorld::Ptr Npc::copyToCellImpl(const MWWorld::ConstPtr& ptr, MWWorld::CellStore& cell) const
     {
         const MWWorld::LiveCellRef<ESM::NPC>* ref = ptr.get<ESM::NPC>();
-
-        return MWWorld::Ptr(cell.insert(ref), &cell);
+        MWWorld::Ptr newPtr(cell.insert(ref), &cell);
+        if (newPtr.getRefData().getCustomData())
+        {
+            MWBase::Environment::get().getWorldModel()->registerPtr(newPtr);
+            newPtr.getContainerStore()->setPtr(newPtr);
+        }
+        return newPtr;
     }
 
     float Npc::getSkill(const MWWorld::Ptr& ptr, ESM::RefId id) const

apps/openmw/mwlua/luamanagerimp.cpp

@@ -475,9 +475,6 @@ namespace MWLua
         scripts->setSerializer(mLocalSerializer.get());
         scripts->setSavedDataDeserializer(mLocalLoader.get());
         scripts->load(data);
-
-        // LiveCellRef is usually copied after loading, so this Ptr will become invalid and should be deregistered.
-        MWBase::Environment::get().getWorldModel()->deregisterPtr(ptr);
     }
 
     void LuaManager::reloadAllScripts()

apps/openmw/mwmechanics/summoning.cpp

@@ -90,17 +90,15 @@ namespace MWMechanics
             {
                 auto world = MWBase::Environment::get().getWorld();
                 MWWorld::ManualRef ref(world->getStore(), creatureID, 1);
+                MWWorld::Ptr placed = world->safePlaceObject(ref.getPtr(), summoner, summoner.getCell(), 0, 120.f);
 
-                MWMechanics::CreatureStats& summonedCreatureStats
-                    = ref.getPtr().getClass().getCreatureStats(ref.getPtr());
+                MWMechanics::CreatureStats& summonedCreatureStats = placed.getClass().getCreatureStats(placed);
 
                 // Make the summoned creature follow its master and help in fights
                 AiFollow package(summoner);
-                summonedCreatureStats.getAiSequence().stack(package, ref.getPtr());
+                summonedCreatureStats.getAiSequence().stack(package, placed);
                 creatureActorId = summonedCreatureStats.getActorId();
 
-                MWWorld::Ptr placed = world->safePlaceObject(ref.getPtr(), summoner, summoner.getCell(), 0, 120.f);
-
                 MWRender::Animation* anim = world->getAnimation(placed);
                 if (anim)
                 {

apps/openmw/mwworld/containerstore.cpp

@@ -157,20 +157,6 @@ MWWorld::ContainerStore::ContainerStore()
 {
 }
 
-MWWorld::ContainerStore::~ContainerStore()
-{
-    try
-    {
-        MWWorld::WorldModel* worldModel = MWBase::Environment::get().getWorldModel();
-        for (MWWorld::ContainerStoreIterator iter(begin()); iter != end(); ++iter)
-            worldModel->deregisterPtr(*iter);
-    }
-    catch (const std::exception& e)
-    {
-        Log(Debug::Error) << "Failed to deregister container store: " << e.what();
-    }
-}
-
 MWWorld::ConstContainerStoreIterator MWWorld::ContainerStore::cbegin(int mask) const
 {
     return ConstContainerStoreIterator(mask, this);

apps/openmw/mwworld/containerstore.hpp

@@ -175,7 +175,7 @@ namespace MWWorld
     public:
         ContainerStore();
 
-        virtual ~ContainerStore();
+        virtual ~ContainerStore() = default;
 
         virtual std::unique_ptr<ContainerStore> clone()
         {

apps/openmw/mwworld/livecellref.cpp

@@ -13,6 +13,7 @@
 #include "class.hpp"
 #include "esmstore.hpp"
 #include "ptr.hpp"
+#include "worldmodel.hpp"
 
 MWWorld::LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM::CellRef& cref)
     : mClass(&Class::get(type))
@@ -35,6 +36,11 @@ MWWorld::LiveCellRefBase::LiveCellRefBase(unsigned int type, const ESM4::ActorCh
 {
 }
 
+MWWorld::LiveCellRefBase::~LiveCellRefBase()
+{
+    MWBase::Environment::get().getWorldModel()->deregisterLiveCellRef(*this);
+}
+
 void MWWorld::LiveCellRefBase::loadImp(const ESM::ObjectState& state)
 {
     mRef = MWWorld::CellRef(state.mRef);

apps/openmw/mwworld/livecellref.hpp

@@ -38,7 +38,7 @@ namespace MWWorld
         LiveCellRefBase(unsigned int type, const ESM4::Reference& cref);
         LiveCellRefBase(unsigned int type, const ESM4::ActorCharacter& cref);
         /* Need this for the class to be recognized as polymorphic */
-        virtual ~LiveCellRefBase() {}
+        virtual ~LiveCellRefBase();
 
         virtual void load(const ESM::ObjectState& state) = 0;
         ///< Load state into a LiveCellRef, that has already been initialised with base and class.

apps/openmw/mwworld/ptrregistry.hpp

@@ -43,10 +43,17 @@ namespace MWWorld
             ++mRevision;
         }
 
-        void remove(const Ptr& ptr)
+        void remove(const LiveCellRefBase& ref) noexcept
         {
-            mIndex.erase(ptr.getCellRef().getRefNum());
-            ++mRevision;
+            const ESM::RefNum& refNum = ref.mRef.getRefNum();
+            if (!refNum.isSet())
+                return;
+            auto it = mIndex.find(refNum);
+            if (it != mIndex.end() && it->second.getBase() == &ref)
+            {
+                mIndex.erase(it);
+                ++mRevision;
+            }
         }
 
     private:

apps/openmw/mwworld/worldimp.hpp

@@ -136,8 +136,8 @@ namespace MWWorld
         uint32_t mRandomSeed{};
 
         // not implemented
-        World(const World&);
-        World& operator=(const World&);
+        World(const World&) = delete;
+        World& operator=(const World&) = delete;
 
         void updateWeather(float duration, bool paused = false);
 

apps/openmw/mwworld/worldmodel.hpp

@@ -79,7 +79,7 @@ namespace MWWorld
 
         void registerPtr(const Ptr& ptr) { mPtrRegistry.insert(ptr); }
 
-        void deregisterPtr(const Ptr& ptr) { mPtrRegistry.remove(ptr); }
+        void deregisterLiveCellRef(const LiveCellRefBase& ref) noexcept { mPtrRegistry.remove(ref); }
 
         template <typename Fn>
         void forEachLoadedCellStore(Fn&& fn)