mirror/openmw
1
0
Fork 0
mirror of https://github.com/OpenMW/openmw.git synced 2025-06-25 19:11:34 +00:00
Code Issues Releases Wiki Activity

Fix use after free and possible deadlock on exit

Browse source 
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.
This commit is contained in:
elsid 2022-01-29 05:04:32 +01:00
parent 53f2dfd1c0
commit 8b7ae9afd8
No known key found for this signature in database
GPG key ID: B845CB9FEE18AB40
2 changed files with 84 additions and 39 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

99
apps/openmw/mwphysics/mtphysics.cpp
View file

@ -1,3 +1,5 @@
#include <functional>
#include <BulletCollision/BroadphaseCollision/btDbvtBroadphase.h> #include <BulletCollision/BroadphaseCollision/btDbvtBroadphase.h>
#include <BulletCollision/CollisionShapes/btCollisionShape.h> #include <BulletCollision/CollisionShapes/btCollisionShape.h>
@ -111,17 +113,49 @@ namespace
return ptr.getPosition() * interpolationFactor + ptr.getPreviousPosition() * (1.f - interpolationFactor); return ptr.getPosition() * interpolationFactor + ptr.getPreviousPosition() * (1.f - interpolationFactor);
} }
using LockedActorSimulation = std::pair<
std::shared_ptr<MWPhysics::Actor>,
std::reference_wrapper<MWPhysics::ActorFrameData>
>;
using LockedProjectileSimulation = std::pair<
std::shared_ptr<MWPhysics::Projectile>,
std::reference_wrapper<MWPhysics::ProjectileFrameData>
>;
namespace Visitors namespace Visitors
{ {
template <class Impl, template <class> class Lock>
struct WithLockedPtr
{
const Impl& mImpl;
std::shared_mutex& mCollisionWorldMutex;
const int mNumJobs;
template <class Ptr, class FrameData>
void operator()(MWPhysics::SimulationImpl<Ptr, FrameData>& 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<std::shared_mutex> lock(mCollisionWorldMutex, mNumJobs);
mImpl(arg);
}
};
struct InitPosition struct InitPosition
{ {
const btCollisionWorld* mCollisionWorld; const btCollisionWorld* mCollisionWorld;
void operator()(MWPhysics::ActorSimulation& sim) const void operator()(MWPhysics::ActorSimulation& sim) const
{ {
auto& [actorPtr, frameData] = sim; auto locked = sim.lock();
const auto actor = actorPtr.lock(); if (!locked.has_value())
if (actor == nullptr)
return; return;
auto& [actor, frameDataRef] = *locked;
auto& frameData = frameDataRef.get();
actor->applyOffsetChange(); actor->applyOffsetChange();
frameData.mPosition = actor->getPosition(); frameData.mPosition = actor->getPosition();
if (frameData.mWaterCollision && frameData.mPosition.z() < frameData.mWaterlevel && actor->canMoveToWaterSurface(frameData.mWaterlevel, mCollisionWorld)) if (frameData.mWaterCollision && frameData.mPosition.z() < frameData.mWaterlevel && actor->canMoveToWaterSurface(frameData.mWaterlevel, mCollisionWorld))
@ -138,7 +172,7 @@ namespace
frameData.mStuckFrames = actor->getStuckFrames(); frameData.mStuckFrames = actor->getStuckFrames();
frameData.mLastStuckPosition = actor->getLastStuckPosition(); frameData.mLastStuckPosition = actor->getLastStuckPosition();
} }
void operator()(MWPhysics::ProjectileSimulation& sim) const void operator()(MWPhysics::ProjectileSimulation& /*sim*/) const
{ {
} }
}; };
@ -146,11 +180,11 @@ namespace
struct PreStep struct PreStep
{ {
btCollisionWorld* mCollisionWorld; btCollisionWorld* mCollisionWorld;
void operator()(MWPhysics::ActorSimulation& sim) const void operator()(const LockedActorSimulation& sim) const
{ {
MWPhysics::MovementSolver::unstuck(sim.second, mCollisionWorld); 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 struct UpdatePosition
{ {
btCollisionWorld* mCollisionWorld; btCollisionWorld* mCollisionWorld;
void operator()(MWPhysics::ActorSimulation& sim) const void operator()(const LockedActorSimulation& sim) const
{ {
auto& [actorPtr, frameData] = sim; auto& [actor, frameDataRef] = sim;
const auto actor = actorPtr.lock(); auto& frameData = frameDataRef.get();
if (actor == nullptr)
return;
if (actor->setPosition(frameData.mPosition)) if (actor->setPosition(frameData.mPosition))
{ {
frameData.mPosition = actor->getPosition(); // account for potential position change made by script frameData.mPosition = actor->getPosition(); // account for potential position change made by script
@ -171,12 +203,10 @@ namespace
mCollisionWorld->updateSingleAabb(actor->getCollisionObject()); mCollisionWorld->updateSingleAabb(actor->getCollisionObject());
} }
} }
void operator()(MWPhysics::ProjectileSimulation& sim) const void operator()(const LockedProjectileSimulation& sim) const
{ {
auto& [projPtr, frameData] = sim; auto& [proj, frameDataRef] = sim;
const auto proj = projPtr.lock(); auto& frameData = frameDataRef.get();
if (proj == nullptr)
return;
proj->setPosition(frameData.mPosition); proj->setPosition(frameData.mPosition);
proj->updateCollisionObjectPosition(); proj->updateCollisionObjectPosition();
mCollisionWorld->updateSingleAabb(proj->getCollisionObject()); mCollisionWorld->updateSingleAabb(proj->getCollisionObject());
@ -188,11 +218,11 @@ namespace
const float mPhysicsDt; const float mPhysicsDt;
const btCollisionWorld* mCollisionWorld; const btCollisionWorld* mCollisionWorld;
const MWPhysics::WorldFrameData& mWorldFrameData; const MWPhysics::WorldFrameData& mWorldFrameData;
void operator()(MWPhysics::ActorSimulation& sim) const void operator()(const LockedActorSimulation& sim) const
{ {
MWPhysics::MovementSolver::move(sim.second, mPhysicsDt, mCollisionWorld, mWorldFrameData); 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); MWPhysics::MovementSolver::move(sim.second, mPhysicsDt, mCollisionWorld);
} }
@ -206,10 +236,11 @@ namespace
const MWPhysics::PhysicsTaskScheduler* scheduler; const MWPhysics::PhysicsTaskScheduler* scheduler;
void operator()(MWPhysics::ActorSimulation& sim) const void operator()(MWPhysics::ActorSimulation& sim) const
{ {
auto& [actorPtr, frameData] = sim; auto locked = sim.lock();
const auto actor = actorPtr.lock(); if (!locked.has_value())
if (actor == nullptr)
return; return;
auto& [actor, frameDataRef] = *locked;
auto& frameData = frameDataRef.get();
auto ptr = actor->getPtr(); auto ptr = actor->getPtr();
MWMechanics::CreatureStats& stats = ptr.getClass().getCreatureStats(ptr); MWMechanics::CreatureStats& stats = ptr.getClass().getCreatureStats(ptr);
@ -241,10 +272,10 @@ namespace
} }
void operator()(MWPhysics::ProjectileSimulation& sim) const void operator()(MWPhysics::ProjectileSimulation& sim) const
{ {
auto& [projPtr, frameData] = sim; auto locked = sim.lock();
const auto proj = projPtr.lock(); if (!locked.has_value())
if (proj == nullptr)
return; return;
auto& [proj, frameData] = *locked;
proj->setSimulationPosition(::interpolateMovements(*proj, mTimeAccum, mPhysicsDt)); proj->setSimulationPosition(::interpolateMovements(*proj, mTimeAccum, mPhysicsDt));
} }
}; };
@ -612,12 +643,10 @@ namespace MWPhysics
void PhysicsTaskScheduler::updateActorsPositions() void PhysicsTaskScheduler::updateActorsPositions()
{ {
const Visitors::UpdatePosition vis{mCollisionWorld}; const Visitors::UpdatePosition impl{mCollisionWorld};
for (auto& sim : mSimulations) const Visitors::WithLockedPtr<Visitors::UpdatePosition, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mNumThreads};
{ for (Simulation& sim : mSimulations)
MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
std::visit(vis, sim); std::visit(vis, sim);
}
} }
bool PhysicsTaskScheduler::hasLineOfSight(const Actor* actor1, const Actor* actor2) bool PhysicsTaskScheduler::hasLineOfSight(const Actor* actor1, const Actor* actor2)
@ -641,12 +670,10 @@ namespace MWPhysics
{ {
mPreStepBarrier->wait([this] { afterPreStep(); }); mPreStepBarrier->wait([this] { afterPreStep(); });
int job = 0; int job = 0;
const Visitors::Move vis{mPhysicsDt, mCollisionWorld, *mWorldFrameData}; const Visitors::Move impl{mPhysicsDt, mCollisionWorld, *mWorldFrameData};
const Visitors::WithLockedPtr<Visitors::Move, MaybeLock> vis{impl, mCollisionWorldMutex, mNumThreads};
while ((job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs) while ((job = mNextJob.fetch_add(1, std::memory_order_relaxed)) < mNumJobs)
{
MaybeLock lockColWorld(mCollisionWorldMutex, mNumThreads);
std::visit(vis, mSimulations[job]); std::visit(vis, mSimulations[job]);
}
mPostStepBarrier->wait([this] { afterPostStep(); }); mPostStepBarrier->wait([this] { afterPostStep(); });
} }
@ -697,12 +724,10 @@ namespace MWPhysics
updateAabbs(); updateAabbs();
if (!mRemainingSteps) if (!mRemainingSteps)
return; return;
const Visitors::PreStep vis{mCollisionWorld}; const Visitors::PreStep impl{mCollisionWorld};
const Visitors::WithLockedPtr<Visitors::PreStep, MaybeExclusiveLock> vis{impl, mCollisionWorldMutex, mNumThreads};
for (auto& sim : mSimulations) for (auto& sim : mSimulations)
{
MaybeExclusiveLock lock(mCollisionWorldMutex, mNumThreads);
std::visit(vis, sim); std::visit(vis, sim);
}
} }
void PhysicsTaskScheduler::afterPostStep() void PhysicsTaskScheduler::afterPostStep()

24
apps/openmw/mwphysics/physicssystem.hpp
View file

@ -8,6 +8,8 @@
#include <unordered_map> #include <unordered_map>
#include <algorithm> #include <algorithm>
#include <variant> #include <variant>
#include <optional>
#include <functional>
#include <osg/Quat> #include <osg/Quat>
#include <osg/BoundingBox> #include <osg/BoundingBox>
@ -117,8 +119,26 @@ namespace MWPhysics
osg::Vec3f mStormDirection; osg::Vec3f mStormDirection;
}; };
using ActorSimulation = std::pair<std::weak_ptr<Actor>, ActorFrameData>; template <class Ptr, class FrameData>
using ProjectileSimulation = std::pair<std::weak_ptr<Projectile>, ProjectileFrameData>; class SimulationImpl
{
public:
explicit SimulationImpl(const std::weak_ptr<Ptr>& ptr, FrameData&& data) : mPtr(ptr), mData(data) {}
std::optional<std::pair<std::shared_ptr<Ptr>, std::reference_wrapper<FrameData>>> lock()
{
if (auto locked = mPtr.lock())
return {{std::move(locked), std::ref(mData)}};
return std::nullopt;
}
private:
std::weak_ptr<Ptr> mPtr;
FrameData mData;
};
using ActorSimulation = SimulationImpl<Actor, ActorFrameData>;
using ProjectileSimulation = SimulationImpl<Projectile, ProjectileFrameData>;
using Simulation = std::variant<ActorSimulation, ProjectileSimulation>; using Simulation = std::variant<ActorSimulation, ProjectileSimulation>;
class PhysicsSystem : public RayCastingInterface class PhysicsSystem : public RayCastingInterface