From e3cce0949e4fb4e97d423b81b8d7d933cbe8eb93 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 17 May 2020 17:24:47 +0200 Subject: [PATCH 1/4] Replace condition that may lead to UB by assert If mPackages is empty it means package is a pointer to a deleted object at line . We can assume it couldn't happen because execute is always called next for this object at line 289. --- apps/openmw/mwmechanics/aisequence.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/apps/openmw/mwmechanics/aisequence.cpp b/apps/openmw/mwmechanics/aisequence.cpp index 5f3931fcf..201701634 100644 --- a/apps/openmw/mwmechanics/aisequence.cpp +++ b/apps/openmw/mwmechanics/aisequence.cpp @@ -265,16 +265,15 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac } } - if (!mPackages.empty()) - { - if (nearestDist < std::numeric_limits::max() && mPackages.begin() != itActualCombat) - { - // move combat package with nearest target to the front - mPackages.splice(mPackages.begin(), mPackages, itActualCombat); - } + assert(!mPackages.empty()); - package = mPackages.front(); + if (nearestDist < std::numeric_limits::max() && mPackages.begin() != itActualCombat) + { + // move combat package with nearest target to the front + mPackages.splice(mPackages.begin(), mPackages, itActualCombat); } + + package = mPackages.front(); } try From ca93f8ee39483cfb3fd4ee3f55527a65373a2b70 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 17 May 2020 17:38:10 +0200 Subject: [PATCH 2/4] Compare initialized iterator Comparsion of untilialized iterator is UB. Initialize with packages end. --- apps/openmw/mwmechanics/aisequence.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/openmw/mwmechanics/aisequence.cpp b/apps/openmw/mwmechanics/aisequence.cpp index 201701634..269468090 100644 --- a/apps/openmw/mwmechanics/aisequence.cpp +++ b/apps/openmw/mwmechanics/aisequence.cpp @@ -223,7 +223,7 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac // if active package is combat one, choose nearest target if (packageTypeId == AiPackage::TypeIdCombat) { - std::list::iterator itActualCombat; + auto itActualCombat = mPackages.end(); float nearestDist = std::numeric_limits::max(); osg::Vec3f vActorPos = actor.getRefData().getPosition().asVec3(); @@ -269,6 +269,7 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac if (nearestDist < std::numeric_limits::max() && mPackages.begin() != itActualCombat) { + assert(itActualCombat != mPackages.end()); // move combat package with nearest target to the front mPackages.splice(mPackages.begin(), mPackages, itActualCombat); } From d48eead0384b64ef878d3629556b662d55093c7a Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 17 May 2020 22:34:11 +0200 Subject: [PATCH 3/4] Check type id of current package If package is changed the following usage of it is not consistent. --- apps/openmw/mwmechanics/aisequence.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/openmw/mwmechanics/aisequence.cpp b/apps/openmw/mwmechanics/aisequence.cpp index 269468090..d33a0a3f8 100644 --- a/apps/openmw/mwmechanics/aisequence.cpp +++ b/apps/openmw/mwmechanics/aisequence.cpp @@ -275,6 +275,7 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac } package = mPackages.front(); + packageTypeId = package->getTypeId(); } try From a4fbcb8a1092eb57664f4576e74f11bb92680ec2 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 17 May 2020 22:37:44 +0200 Subject: [PATCH 4/4] Remember package iterator to erase it from list without find call --- apps/openmw/mwmechanics/aisequence.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/openmw/mwmechanics/aisequence.cpp b/apps/openmw/mwmechanics/aisequence.cpp index d33a0a3f8..00d44202a 100644 --- a/apps/openmw/mwmechanics/aisequence.cpp +++ b/apps/openmw/mwmechanics/aisequence.cpp @@ -212,7 +212,8 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac return; } - MWMechanics::AiPackage* package = mPackages.front(); + auto packageIt = mPackages.begin(); + MWMechanics::AiPackage* package = *packageIt; if (!package->alwaysActive() && outOfRange) return; @@ -274,7 +275,8 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac mPackages.splice(mPackages.begin(), mPackages, itActualCombat); } - package = mPackages.front(); + packageIt = mPackages.begin(); + package = *packageIt; packageTypeId = package->getTypeId(); } @@ -290,9 +292,7 @@ void AiSequence::execute (const MWWorld::Ptr& actor, CharacterController& charac } // To account for the rare case where AiPackage::execute() queued another AI package // (e.g. AiPursue executing a dialogue script that uses startCombat) - std::list::iterator toRemove = - std::find(mPackages.begin(), mPackages.end(), package); - mPackages.erase(toRemove); + mPackages.erase(packageIt); delete package; if (isActualAiPackage(packageTypeId)) mDone = true;