[CRASH FIX] Various crash fixes in regard to defensive checks around unit/targets and finer grained checks on tele ack (#1951)

Some basic defense checks on mainly unit/targets used in public functions, and some minor tweaks with teleport ack. https://github.com/mod-playerbots/mod-playerbots/issues/1934 https://github.com/mod-playerbots/mod-playerbots/issues/1957
2026-01-13 00:58:33 +00:00 · 2026-01-07 15:22:36 +01:00
parent c9cc4324d3
commit 3d9623f119
2 changed files with 149 additions and 60 deletions
--- a/src/PlayerbotAI.cpp
+++ b/src/PlayerbotAI.cpp
@@ -437,7 +437,7 @@ void PlayerbotAI::UpdateAIGroupMaster()

 void PlayerbotAI::UpdateAIInternal([[maybe_unused]] uint32 elapsed, bool minimal)
 {
-    if (bot->IsBeingTeleported() || !bot->IsInWorld())
+    if (!bot || bot->IsBeingTeleported() || !bot->IsInWorld())
        return;

    std::string const mapString = WorldPosition(bot).isOverworld() ? std::to_string(bot->GetMapId()) : "I";
@@ -516,23 +516,37 @@ void PlayerbotAI::UpdateAIInternal([[maybe_unused]] uint32 elapsed, bool minimal
 void PlayerbotAI::HandleCommands()
 {
    ExternalEventHelper helper(aiObjectContext);
+
    for (auto it = chatCommands.begin(); it != chatCommands.end();)
    {
        time_t& checkTime = it->GetTime();
-        if (checkTime && time(0) < checkTime)
+        if (checkTime && time(nullptr) < checkTime)
        {
            ++it;
            continue;
        }

-        const std::string& command = it->GetCommand();
        Player* owner = it->GetOwner();
+        if (!owner)
+        {
+            it = chatCommands.erase(it);
+            continue;
+        }
+
+        const std::string& command = it->GetCommand();
+        if (command.empty())
+        {
+            it = chatCommands.erase(it);
+            continue;
+        }
+
        if (!helper.ParseChatCommand(command, owner) && it->GetType() == CHAT_MSG_WHISPER)
        {
            // ostringstream out; out << "Unknown command " << command;
            // TellPlayer(out);
            // helper.ParseChatCommand("help");
        }
+
        it = chatCommands.erase(it);
    }
 }
@@ -540,6 +554,9 @@ void PlayerbotAI::HandleCommands()
 std::map<std::string, ChatMsg> chatMap;
 void PlayerbotAI::HandleCommand(uint32 type, const std::string& text, Player& fromPlayer, const uint32 lang)
 {
+    if (!bot)
+        return;
+
    std::string filtered = text;

    if (!IsAllowedCommand(filtered) && !GetSecurity()->CheckLevelFor(PlayerbotSecurityLevel::PLAYERBOT_SECURITY_INVITE,
@@ -711,65 +728,82 @@ void PlayerbotAI::HandleCommand(uint32 type, const std::string& text, Player& fr

 void PlayerbotAI::HandleTeleportAck()
 {
+    if (!bot || !bot->GetSession())
+        return;
+
+    // only for bots
    if (IsRealPlayer())
        return;

-    // Clearing motion generators and stopping movement which prevents
-    // conflicts between teleport and any active motion (walk, run, swim, flight, etc.)
-    bot->GetMotionMaster()->Clear(true);
-    bot->StopMoving();
-
-    // Near teleport (within map/instance)
-    if (bot->IsBeingTeleportedNear())
-    {
-        // Previous versions manually added the bot to the map if it was not in the world.
-        // not needed: HandleMoveTeleportAckOpcode() safely attaches the player to the map
-        // and clears IsBeingTeleportedNear().
-
-        Player* plMover = bot->m_mover->ToPlayer();
-        if (!plMover)
-            return;
-
-        // Send the near teleport ACK packet
-        WorldPacket p(MSG_MOVE_TELEPORT_ACK, 20);
-        p << plMover->GetPackGUID();
-        p << uint32(0);
-        p << uint32(0);
-        bot->GetSession()->HandleMoveTeleportAck(p);
-
-        // Simulate teleport latency and prevent AI from running too early (used cmangos delays)
-        SetNextCheckDelay(urand(1000, 2000));
-    }
-
-    // Far teleport (worldport / different map)
+    /*
+     * FAR TELEPORT (worldport / map change)
+     * Player may NOT be in world or grid here.
+     * Handle this FIRST.
+     */
    if (bot->IsBeingTeleportedFar())
    {
-        // Handle far teleport ACK:
-        // Moves the bot to the new map, clears IsBeingTeleportedFar(), updates session/map references
        bot->GetSession()->HandleMoveWorldportAck();

-        // Ensure bot now has a valid map. If this fails, there is a core/session bug?
+        // after worldport ACK the player should be in a valid map
        if (!bot->GetMap())
        {
            LOG_ERROR("playerbot", "Bot {} has no map after worldport ACK", bot->GetGUID().ToString());
+            return;
        }

-        // Instance strategies after teleport
+        // apply instance-related strategies after map attach
        if (sPlayerbotAIConfig->applyInstanceStrategies)
            ApplyInstanceStrategies(bot->GetMapId(), true);

-        // healer DPS strategies if restrictions are enabled
        if (sPlayerbotAIConfig->restrictHealerDPS)
            EvaluateHealerDpsStrategy();

-        // Reset AI state to to before teleport conditions
+        // reset AI state after teleport
        Reset(true);

-        // Slightly longer delay to simulate far teleport latency (used cmangos delays)
+        // clear movement only AFTER teleport is finalized and bot is in world
+        if (bot->IsInWorld() && bot->GetMotionMaster())
+        {
+            bot->GetMotionMaster()->Clear(true);
+            bot->StopMoving();
+        }
+
+        // simulate far teleport latency (cmangos-style)
        SetNextCheckDelay(urand(2000, 5000));
+        return;
    }

-    SetNextCheckDelay(sPlayerbotAIConfig->globalCoolDown);
+    /*
+     * NEAR TELEPORT (same map / instance)
+     * Player MUST be in world (and in grid).
+     */
+    if (bot->IsBeingTeleportedNear())
+    {
+        if (!bot->IsInWorld())
+            return;
+
+        Player* plMover = bot->m_mover ? bot->m_mover->ToPlayer() : nullptr;
+        if (!plMover)
+            return;
+
+        WorldPacket p(MSG_MOVE_TELEPORT_ACK, 20);
+        p << plMover->GetPackGUID();
+        p << uint32(0);  // flags
+        p << uint32(0);  // time
+
+        bot->GetSession()->HandleMoveTeleportAck(p);
+
+        // clear movement after successful relocation
+        if (bot->GetMotionMaster())
+        {
+            bot->GetMotionMaster()->Clear(true);
+            bot->StopMoving();
+        }
+
+        // simulate near teleport latency
+        SetNextCheckDelay(urand(1000, 2000));
+        return;
+    }
 }

 void PlayerbotAI::Reset(bool full)
@@ -1286,7 +1320,12 @@ void PlayerbotAI::SpellInterrupted(uint32 spellid)
        Spell* spell = bot->GetCurrentSpell((CurrentSpellTypes)type);
        if (!spell)
            continue;
-        if (spell->GetSpellInfo()->Id == spellid)
+
+        SpellInfo const* spellInfo = spell->GetSpellInfo();
+        if (!spellInfo)
+            continue;
+
+        if (spellInfo->Id == spellid)
            bot->InterruptSpell((CurrentSpellTypes)type);
    }
    // LastSpellCast& lastSpell = aiObjectContext->GetValue<LastSpellCast&>("last spell cast")->Get();
@@ -1728,6 +1767,7 @@ bool PlayerbotAI::IsRanged(Player* player, bool bySpec)
            }
            break;
    }
+
    return true;
 }

@@ -1821,10 +1861,9 @@ bool PlayerbotAI::IsRangedDpsAssistantOfIndex(Player* player, int index)

 bool PlayerbotAI::HasAggro(Unit* unit)
 {
-    if (!unit)
-    {
+    if (!IsValidUnit(unit))
        return false;
-    }
+
    bool isMT = IsMainTank(bot);
    Unit* victim = unit->GetVictim();
    if (victim && (victim->GetGUID() == bot->GetGUID() || (!isMT && victim->ToPlayer() && IsTank(victim->ToPlayer()))))
@@ -2822,6 +2861,9 @@ bool PlayerbotAI::TellMaster(std::string const text, PlayerbotSecurityLevel secu

 bool IsRealAura(Player* bot, AuraEffect const* aurEff, Unit const* unit)
 {
+    if (!unit || !unit->IsInWorld() || unit->IsDuringRemoveFromWorld())
+        return false;
+
    if (!aurEff)
        return false;

@@ -2829,6 +2871,8 @@ bool IsRealAura(Player* bot, AuraEffect const* aurEff, Unit const* unit)
        return true;

    SpellInfo const* spellInfo = aurEff->GetSpellInfo();
+    if (!spellInfo)
+        return false;

    uint32 stacks = aurEff->GetBase()->GetStackAmount();
    if (stacks >= spellInfo->StackAmount)
@@ -2844,7 +2888,7 @@ bool IsRealAura(Player* bot, AuraEffect const* aurEff, Unit const* unit)
 bool PlayerbotAI::HasAura(std::string const name, Unit* unit, bool maxStack, bool checkIsOwner, int maxAuraAmount,
                          bool checkDuration)
 {
-    if (!unit)
+    if (!IsValidUnit(unit))
        return false;

    std::wstring wnamepart;
@@ -2940,7 +2984,7 @@ bool PlayerbotAI::HasAura(uint32 spellId, Unit const* unit)

 Aura* PlayerbotAI::GetAura(std::string const name, Unit* unit, bool checkIsOwner, bool checkDuration, int checkStack)
 {
-    if (!unit)
+    if (!IsValidUnit(unit))
        return nullptr;

    std::wstring wnamepart;
@@ -2958,6 +3002,9 @@ Aura* PlayerbotAI::GetAura(std::string const name, Unit* unit, bool checkIsOwner
        for (AuraEffect const* aurEff : auras)
        {
            SpellInfo const* spellInfo = aurEff->GetSpellInfo();
+            if (!spellInfo)
+                continue;
+
            std::string const& auraName = spellInfo->SpellName[0];

            // Directly skip if name mismatch (both length and content)
@@ -3038,6 +3085,9 @@ bool PlayerbotAI::CanCastSpell(uint32 spellid, Unit* target, bool checkHasSpell,
    if (!target)
        target = bot;

+     if (!IsValidUnit(target))
+        return false;
+
    if (Pet* pet = bot->GetPet())
        if (pet->HasSpell(spellid))
            return true;
@@ -3299,6 +3349,9 @@ bool PlayerbotAI::CanCastSpell(uint32 spellid, float x, float y, float z, bool c

 bool PlayerbotAI::CastSpell(std::string const name, Unit* target, Item* itemTarget)
 {
+    if (!IsValidUnit(target))
+        return false;
+
    bool result = CastSpell(aiObjectContext->GetValue<uint32>("spell id", name)->Get(), target, itemTarget);
    if (result)
    {
@@ -3311,15 +3364,19 @@ bool PlayerbotAI::CastSpell(std::string const name, Unit* target, Item* itemTarg
 bool PlayerbotAI::CastSpell(uint32 spellId, Unit* target, Item* itemTarget)
 {
    if (!spellId)
-    {
        return false;
-    }

    if (!target)
        target = bot;

-    Pet* pet = bot->GetPet();
+    if (!IsValidUnit(target))
+        return false;
+
    SpellInfo const* spellInfo = sSpellMgr->GetSpellInfo(spellId);
+    if (!spellInfo)
+        return false;
+
+    Pet* pet = bot->GetPet();
    if (pet && pet->HasSpell(spellId))
    {
        // List of spell IDs for which we do NOT want to toggle auto-cast or send message
@@ -3722,6 +3779,9 @@ bool PlayerbotAI::CanCastVehicleSpell(uint32 spellId, Unit* target)
    if (!spellId)
        return false;

+    if (!IsValidUnit(target))
+        return false;
+
    Vehicle* vehicle = bot->GetVehicle();
    if (!vehicle)
        return false;
@@ -3732,12 +3792,12 @@ bool PlayerbotAI::CanCastVehicleSpell(uint32 spellId, Unit* target)
        return false;

    Unit* vehicleBase = vehicle->GetBase();
-
    Unit* spellTarget = target;
+
    if (!spellTarget)
        spellTarget = vehicleBase;

-    if (!spellTarget)
+    if (!IsValidUnit(spellTarget))
        return false;

    if (vehicleBase->HasSpellCooldown(spellId))
@@ -3804,6 +3864,9 @@ bool PlayerbotAI::CastVehicleSpell(uint32 spellId, Unit* target)
    if (!spellId)
        return false;

+    if (!IsValidUnit(target))
+        return false;
+
    Vehicle* vehicle = bot->GetVehicle();
    if (!vehicle)
        return false;
@@ -3814,12 +3877,12 @@ bool PlayerbotAI::CastVehicleSpell(uint32 spellId, Unit* target)
        return false;

    Unit* vehicleBase = vehicle->GetBase();
-
    Unit* spellTarget = target;
+
    if (!spellTarget)
        spellTarget = vehicleBase;

-    if (!spellTarget)
+    if (!IsValidUnit(spellTarget))
        return false;

    SpellInfo const* spellInfo = sSpellMgr->GetSpellInfo(spellId);
@@ -3972,9 +4035,13 @@ bool PlayerbotAI::IsInVehicle(bool canControl, bool canCast, bool canAttack, boo

 void PlayerbotAI::WaitForSpellCast(Spell* spell)
 {
+    if (!spell)
+        return;
+
    SpellInfo const* spellInfo = spell->GetSpellInfo();
    uint32 castTime = spell->GetCastTime();
-    if (spellInfo->IsChanneled())
+
+    if (spellInfo && spellInfo->IsChanneled())
    {
        int32 duration = spellInfo->GetDuration();
        bot->ApplySpellMod(spellInfo->Id, SPELLMOD_DURATION, duration);
@@ -4022,6 +4089,9 @@ void PlayerbotAI::RemoveAura(std::string const name)

 bool PlayerbotAI::IsInterruptableSpellCasting(Unit* target, std::string const spell)
 {
+    if (!IsValidUnit(target))
+        return false;
+
    uint32 spellid = aiObjectContext->GetValue<uint32>("spell id", spell)->Get();
    if (!spellid || !target->IsNonMeleeSpellCast(true))
        return false;
@@ -4050,17 +4120,25 @@ bool PlayerbotAI::IsInterruptableSpellCasting(Unit* target, std::string const sp

 bool PlayerbotAI::HasAuraToDispel(Unit* target, uint32 dispelType)
 {
-    if (!target->IsInWorld())
-    {
+    if (!IsValidUnit(target) || !target->IsAlive())
        return false;
-    }
+
+    if (!IsValidPlayer(bot))
+        return false;
+
    bool isFriend = bot->IsFriendlyTo(target);
+
    Unit::VisibleAuraMap const* visibleAuras = target->GetVisibleAuras();
+    if (!visibleAuras)
+        return false;
+
    for (Unit::VisibleAuraMap::const_iterator itr = visibleAuras->begin(); itr != visibleAuras->end(); ++itr)
    {
-        Aura* aura = itr->second->GetBase();
+        if (!itr->second)
+            continue;

-        if (aura->IsPassive())
+        Aura* aura = itr->second->GetBase();
+        if (!aura || aura->IsPassive() || aura->IsRemoved())
            continue;

        if (sPlayerbotAIConfig->dispelAuraDuration && aura->GetDuration() &&
@@ -4068,6 +4146,8 @@ bool PlayerbotAI::HasAuraToDispel(Unit* target, uint32 dispelType)
            continue;

        SpellInfo const* spellInfo = aura->GetSpellInfo();
+        if (!spellInfo)
+            continue;

        bool isPositiveSpell = spellInfo->IsPositive();
        if (isPositiveSpell && isFriend)
@@ -4079,6 +4159,7 @@ bool PlayerbotAI::HasAuraToDispel(Unit* target, uint32 dispelType)
        if (canDispel(spellInfo, dispelType))
            return true;
    }
+
    return false;
 }

@@ -5709,7 +5790,7 @@ void PlayerbotAI::ImbueItem(Item* item) { ImbueItem(item, TARGET_FLAG_NONE, Obje
 //  item on unit
 void PlayerbotAI::ImbueItem(Item* item, Unit* target)
 {
-    if (!target)
+    if (!IsValidUnit(target))
        return;

    ImbueItem(item, TARGET_FLAG_UNIT, target->GetGUID());
--- a/src/PlayerbotAI.h
+++ b/src/PlayerbotAI.h
@@ -616,7 +616,15 @@ private:
    void HandleCommands();
    void HandleCommand(uint32 type, const std::string& text, Player& fromPlayer, const uint32 lang = LANG_UNIVERSAL);
    bool _isBotInitializing = false;
-
+    inline bool IsValidUnit(const Unit* unit) const
+    {
+        return unit && unit->IsInWorld() && !unit->IsDuringRemoveFromWorld();
+    }
+    inline bool IsValidPlayer(const Player* player) const
+    {
+        return player && player->GetSession() && player->IsInWorld() && !player->IsDuringRemoveFromWorld() &&
+               !player->IsBeingTeleported();
+    }
 protected:
    Player* bot;
    Player* master;