From 8b7ae9afd8ca7b253cb04dea0449d8b23117054e Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 29 Jan 2022 05:04:32 +0100 Subject: [PATCH] Fix use after free and possible deadlock on exit Lock Simulation weak_ptr in all visitors to avoid use after free. And swap order for weak_ptr locking with locking collision world mutex to avoid deadlock when underlying object tries to lock the same mutex in the destructor. Add SimulationImpl type to avoid use of FrameData without locking weak_ptr. --- apps/openmw/mwphysics/mtphysics.cpp | 99 ++++++++++++++++--------- apps/openmw/mwphysics/physicssystem.hpp | 24 +++++- 2 files changed, 84 insertions(+), 39 deletions(-) diff --git a/apps/openmw/mwphysics/mtphysics.cpp b/apps/openmw/mwphysics/mtphysics.cpp index 2bbd751f63..7de197a32a 100644 --- a/apps/openmw/mwphysics/mtphysics.cpp +++ b/apps/openmw/mwphysics/mtphysics.cpp @@ -1,3 +1,5 @@ +#include + #include #include @@ -111,17 +113,49 @@ namespace return ptr.getPosition() * interpolationFactor + ptr.getPreviousPosition() * (1.f - interpolationFactor); } + using LockedActorSimulation = std::pair< + std::shared_ptr, + std::reference_wrapper + >; + using LockedProjectileSimulation = std::pair< + std::shared_ptr, + std::reference_wrapper + >; + namespace Visitors { + template class Lock> + struct WithLockedPtr + { + const Impl& mImpl; + std::shared_mutex& mCollisionWorldMutex; + const int mNumJobs; + + template + void operator()(MWPhysics::SimulationImpl& sim) const + { + auto locked = sim.lock(); + if (!locked.has_value()) + return; + auto&& [ptr, frameData] = *std::move(locked); + // Locked shared_ptr has to be destructed after releasing mCollisionWorldMutex to avoid + // possible deadlock. Ptr destructor also acquires mCollisionWorldMutex. + const std::pair arg(std::move(ptr), frameData); + const Lock lock(mCollisionWorldMutex, mNumJobs); + mImpl(arg); + } + }; + struct InitPosition { const btCollisionWorld* mCollisionWorld; void operator()(MWPhysics::ActorSimulation& sim) const { - auto& [actorPtr, frameData] = sim; - const auto actor = actorPtr.lock(); - if (actor == nullptr) + auto locked = sim.lock(); + if (!locked.has_value()) return; + auto& [actor, frameDataRef] = *locked; + auto& frameData = frameDataRef.get(); actor->applyOffsetChange(); frameData.mPosition = actor->getPosition(); if (frameData.mWaterCollision && frameData.mPosition.z() < frameData.mWaterlevel && actor->canMoveToWaterSurface(frameData.mWaterlevel, mCollisionWorld)) @@ -138,7 +172,7 @@ namespace frameData.mStuckFrames = actor->getStuckFrames(); frameData.mLastStuckPosition = actor->getLastStuckPosition(); } - void operator()(MWPhysics::ProjectileSimulation& sim) const + void operator()(MWPhysics::ProjectileSimulation& /*sim*/) const { } }; @@ -146,11 +180,11 @@ namespace struct PreStep { btCollisionWorld* mCollisionWorld; - void operator()(MWPhysics::ActorSimulation& sim) const + void operator()(const LockedActorSimulation& sim) const { MWPhysics::MovementSolver::unstuck(sim.second, mCollisionWorld); } - void operator()(MWPhysics::ProjectileSimulation& sim) const + void operator()(const LockedProjectileSimulation& /*sim*/) const { } }; @@ -158,12 +192,10 @@ namespace struct UpdatePosition { btCollisionWorld* mCollisionWorld; - void operator()(MWPhysics::ActorSimulation& sim) const + void operator()(const LockedActorSimulation& sim) const { - auto& [actorPtr, frameData] = sim; - const auto actor = actorPtr.lock(); - if (actor == nullptr) - return; + auto& [actor, frameDataRef] = sim; + auto& frameData = frameDataRef.get(); if (actor->setPosition(frameData.mPosition)) { frameData.mPosition = actor->getPosition(); // account for potential position change made by script @@ -171,12 +203,10 @@ namespace mCollisionWorld->updateSingleAabb(actor->getCollisionObject()); } } - void operator()(MWPhysics::ProjectileSimulation& sim) const + void operator()(const LockedProjectileSimulation& sim) const { - auto& [projPtr, frameData] = sim; - const auto proj = projPtr.lock(); - if (proj == nullptr) - return; + auto& [proj, frameDataRef] = sim; + auto& frameData = frameDataRef.get(); proj->setPosition(frameData.mPosition); proj->updateCollisionObjectPosition(); mCollisionWorld->updateSingleAabb(proj->getCollisionObject()); @@ -188,11 +218,11 @@ namespace const float mPhysicsDt; const btCollisionWorld* mCollisionWorld; const MWPhysics::WorldFrameData& mWorldFrameData; - void operator()(MWPhysics::ActorSimulation& sim) const + void operator()(const LockedActorSimulation& sim) const { MWPhysics::MovementSolver::move(sim.second, mPhysicsDt, mCollisionWorld, mWorldFrameData); } - void operator()(MWPhysics::ProjectileSimulation& sim) const + void operator()(const LockedProjectileSimulation& sim) const { MWPhysics::MovementSolver::move(sim.second, mPhysicsDt, mCollisionWorld); } @@ -206,10 +236,11 @@ namespace const MWPhysics::PhysicsTaskScheduler* scheduler; void operator()(MWPhysics::ActorSimulation& sim) const { - auto& [actorPtr, frameData] = sim; - const auto actor = actorPtr.lock(); - if (actor == nullptr) + auto locked = sim.lock(); + if (!locked.has_value()) return; + auto& [actor, frameDataRef] = *locked; + auto& frameData = frameDataRef.get(); auto ptr = actor->getPtr(); MWMechanics::CreatureStats& stats = ptr.getClass().getCreatureStats(ptr); @@ -241,10 +272,10 @@ namespace } void operator()(MWPhysics::ProjectileSimulation& sim) const { - auto& [projPtr, frameData] = sim; - const auto proj = projPtr.lock(); - if (proj == nullptr) + auto locked = sim.lock(); + if (!locked.has_value()) return; + auto& [proj, frameData] = *locked; proj->setSimulationPosition(::interpolateMovements(*proj, mTimeAccum, mPhysicsDt)); } }; @@ -612,12 +643,10 @@ namespace MWPhysics void PhysicsTaskScheduler::updateActorsPositions() { - const Visitors::UpdatePosition vis{mCollisionWorld}; - for (auto& sim : mSimulations) - { - MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads); + const Visitors::UpdatePosition impl{mCollisionWorld}; + const Visitors::WithLockedPtr vis{impl, mCollisionWorldMutex, mNumThreads}; + for (Simulation& sim : mSimulations) std::visit(vis, sim); - } } bool PhysicsTaskScheduler::hasLineOfSight(const Actor* actor1, const Actor* actor2) @@ -641,12 +670,10 @@ namespace MWPhysics { mPreStepBarrier->wait([this] { afterPreStep(); }); int job = 0; - const Visitors::Move vis{mPhysicsDt, mCollisionWorld, *mWorldFrameData}; + const Visitors::Move impl{mPhysicsDt, mCollisionWorld, *mWorldFrameData}; + const Visitors::WithLockedPtr vis{impl, mCollisionWorldMutex, mNumThreads}; while ((job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs) - { - MaybeLock lockColWorld(mCollisionWorldMutex, mNumThreads); std::visit(vis, mSimulations[job]); - } mPostStepBarrier->wait([this] { afterPostStep(); }); } @@ -697,12 +724,10 @@ namespace MWPhysics updateAabbs(); if (!mRemainingSteps) return; - const Visitors::PreStep vis{mCollisionWorld}; + const Visitors::PreStep impl{mCollisionWorld}; + const Visitors::WithLockedPtr vis{impl, mCollisionWorldMutex, mNumThreads}; for (auto& sim : mSimulations) - { - MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads); std::visit(vis, sim); - } } void PhysicsTaskScheduler::afterPostStep() diff --git a/apps/openmw/mwphysics/physicssystem.hpp b/apps/openmw/mwphysics/physicssystem.hpp index d2b535c427..1606ac084c 100644 --- a/apps/openmw/mwphysics/physicssystem.hpp +++ b/apps/openmw/mwphysics/physicssystem.hpp @@ -8,6 +8,8 @@ #include #include #include +#include +#include #include #include @@ -117,8 +119,26 @@ namespace MWPhysics osg::Vec3f mStormDirection; }; - using ActorSimulation = std::pair, ActorFrameData>; - using ProjectileSimulation = std::pair, ProjectileFrameData>; + template + class SimulationImpl + { + public: + explicit SimulationImpl(const std::weak_ptr& ptr, FrameData&& data) : mPtr(ptr), mData(data) {} + + std::optional, std::reference_wrapper>> lock() + { + if (auto locked = mPtr.lock()) + return {{std::move(locked), std::ref(mData)}}; + return std::nullopt; + } + + private: + std::weak_ptr mPtr; + FrameData mData; + }; + + using ActorSimulation = SimulationImpl; + using ProjectileSimulation = SimulationImpl; using Simulation = std::variant; class PhysicsSystem : public RayCastingInterface