Fix crashes and potential memory leaks related to adding aura holders

Return value of Unit::AddSpellAuraHolder actually used. Also sync player and pet aura loaders. Original credit for finding the source of the problem goes to @Laizerox
cmangos · Mar 21, 2017 · 7514365 · 7514365
1 parent baf2b44
commit 7514365
Show file tree

Hide file tree

Showing 7 changed files with 37 additions and 18 deletions.
diff --git a/src/game/GridNotifiersImpl.h b/src/game/GridNotifiersImpl.h
@@ -184,7 +184,8 @@ inline void MaNGOS::DynamicObjectUpdater::VisitHelper(Unit* target)
         holder = CreateSpellAuraHolder(spellInfo, target, i_dynobject.GetCaster());
         PersistentAreaAura* Aur = new PersistentAreaAura(spellInfo, eff_index, nullptr, holder, target, i_dynobject.GetCaster());
         holder->AddAura(Aur, eff_index);
-        target->AddSpellAuraHolder(holder);
+        if (!target->AddSpellAuraHolder(holder))
+            delete holder;
     }
 
     i_dynobject.AddAffected(target);

diff --git a/src/game/Level3.cpp b/src/game/Level3.cpp
@@ -4035,7 +4035,8 @@ bool ChatHandler::HandleAuraCommand(char* args)
             holder->AddAura(aur, SpellEffectIndex(i));
         }
     }
-    target->AddSpellAuraHolder(holder);
+    if (!target->AddSpellAuraHolder(holder))
+        delete holder;
 
     return true;
 }

diff --git a/src/game/Pet.cpp b/src/game/Pet.cpp
@@ -1466,8 +1466,21 @@ void Pet::_LoadAuras(uint32 timediff)
                 holder->AddAura(aura, SpellEffectIndex(i));
             }
 
-            if (!holder->IsEmptyHolder())
-                AddSpellAuraHolder(holder);
+            const bool empty = holder->IsEmptyHolder();
+            if (!empty)
+            {
+                // reset stolen single target auras
+                if (casterGuid != GetObjectGuid() && holder->GetTrackedAuraType() == TRACK_AURA_TYPE_SINGLE_TARGET)
+                    holder->SetTrackedAuraType(TRACK_AURA_TYPE_NOT_TRACKED);
+
+                holder->SetState(SPELLAURAHOLDER_STATE_DB_LOAD); // Safeguard mechanism against some actions
+            }
+
+            if (!empty && AddSpellAuraHolder(holder))
+            {
+                holder->SetState(SPELLAURAHOLDER_STATE_READY);
+                DETAIL_LOG("Added pet auras from spellid %u", spellproto->Id);
+            }
             else
                 delete holder;
         }

diff --git a/src/game/Player.cpp b/src/game/Player.cpp
@@ -15998,16 +15998,20 @@ void Player::_LoadAuras(QueryResult* result, uint32 timediff)
                 holder->AddAura(aura, SpellEffectIndex(i));
             }
 
-            if (!holder->IsEmptyHolder())
+            const bool empty = holder->IsEmptyHolder();
+            if (!empty)
             {
                 // reset stolen single target auras
                 if (caster_guid != GetObjectGuid() && holder->GetTrackedAuraType() == TRACK_AURA_TYPE_SINGLE_TARGET)
                     holder->SetTrackedAuraType(TRACK_AURA_TYPE_NOT_TRACKED);
 
                 holder->SetState(SPELLAURAHOLDER_STATE_DB_LOAD); // Safeguard mechanism against some actions
-                AddSpellAuraHolder(holder);
+            }
+
+            if (!empty && AddSpellAuraHolder(holder))
+            {
                 holder->SetState(SPELLAURAHOLDER_STATE_READY);
-                DETAIL_LOG("Added auras from spellid %u", spellproto->Id);
+                DETAIL_LOG("Added player auras from spellid %u", spellproto->Id);
             }
             else
                 delete holder;

diff --git a/src/game/Spell.cpp b/src/game/Spell.cpp
@@ -1405,7 +1405,11 @@ void Spell::DoSpellHitOnUnit(Unit* unit, uint32 effectMask)
                 m_spellAuraHolder->SetAuraDuration(duration);
             }
 
-            unit->AddSpellAuraHolder(m_spellAuraHolder);
+            if (!unit->AddSpellAuraHolder(m_spellAuraHolder))
+            {
+                delete m_spellAuraHolder;
+                m_spellAuraHolder = nullptr;
+            }
         }
         else
         {

diff --git a/src/game/Unit.cpp b/src/game/Unit.cpp
@@ -4716,7 +4716,6 @@ bool Unit::AddSpellAuraHolder(SpellAuraHolder* holder)
         !IsDeathOnlySpell(aurSpellInfo) && !aurSpellInfo->HasAttribute(SPELL_ATTR_EX2_CAN_TARGET_DEAD) &&
         (GetTypeId() != TYPEID_PLAYER || !((Player*)this)->GetSession()->PlayerLoading()))
     {
-        delete holder;
         return false;
     }
 
@@ -4725,7 +4724,6 @@ bool Unit::AddSpellAuraHolder(SpellAuraHolder* holder)
         sLog.outError("Holder (spell %u) add to spell aura holder list of %s (lowguid: %u) but spell aura holder target is %s (lowguid: %u)",
                       holder->GetId(), (GetTypeId() == TYPEID_PLAYER ? "player" : "creature"), GetGUIDLow(),
                       (holder->GetTarget()->GetTypeId() == TYPEID_PLAYER ? "player" : "creature"), holder->GetTarget()->GetGUIDLow());
-        delete holder;
         return false;
     }
 
@@ -4745,7 +4743,6 @@ bool Unit::AddSpellAuraHolder(SpellAuraHolder* holder)
                 {
                     // can be created with >1 stack by some spell mods
                     foundHolder->ModStackAmount(holder->GetStackAmount());
-                    delete holder;
                     return false;
                 }
 
@@ -4832,7 +4829,6 @@ bool Unit::AddSpellAuraHolder(SpellAuraHolder* holder)
         if (!IsSpellHaveAura(aurSpellInfo, SPELL_AURA_CONTROL_VEHICLE) && !IsSpellHaveAura(aurSpellInfo, SPELL_AURA_TRIGGER_LINKED_AURA) &&
                 !RemoveNoStackAurasDueToAuraHolder(holder))
         {
-            delete holder;
             return false;                                   // couldn't remove conflicting aura with higher rank
         }
     }
@@ -4934,10 +4930,8 @@ bool Unit::AddSpellAuraHolder(SpellAuraHolder* holder)
 
     // if aura deleted before boosts apply ignore
     // this can be possible it it removed indirectly by triggered spell effect at ApplyModifier
-    if (holder->IsDeleted())
-        return false;
-
-    holder->HandleSpellSpecificBoosts(true);
+    if (!holder->IsDeleted())
+        holder->HandleSpellSpecificBoosts(true);
 
     return true;
 }
@@ -5310,7 +5304,8 @@ void Unit::RemoveAurasDueToSpellBySteal(uint32 spellId, ObjectGuid casterGuid, U
     // strange but intended behaviour: Stolen single target auras won't be treated as single targeted
     new_holder->SetTrackedAuraType(TRACK_AURA_TYPE_NOT_TRACKED);
 
-    stealer->AddSpellAuraHolder(new_holder);
+    if (!stealer->AddSpellAuraHolder(new_holder))
+        delete new_holder;
 }
 
 void Unit::RemoveAurasDueToSpellByCancel(uint32 spellId)

diff --git a/src/game/UnitAuraProcHandler.cpp b/src/game/UnitAuraProcHandler.cpp
@@ -3919,7 +3919,8 @@ SpellAuraProcResult Unit::HandleMendingAuraProc(Unit* /*pVictim*/, uint32 /*dama
                 // lock aura holder (currently SPELL_AURA_PRAYER_OF_MENDING is single target spell, so will attempt removing from old target
                 // when applied to new one)
                 triggeredByAura->SetInUse(true);
-                target->AddSpellAuraHolder(new_holder);
+                if (!target->AddSpellAuraHolder(new_holder))
+                    delete new_holder;
                 triggeredByAura->SetInUse(false);
             }
         }