From faa8a492f7dfb6e6521bde31e947658582641a9c Mon Sep 17 00:00:00 2001 From: Chris Miles Date: Fri, 24 Jan 2025 12:22:58 -0600 Subject: [PATCH] [Memory Leak] Fix leak in Client::RemoveDuplicateLore (#4614) --- common/inventory_profile.h | 2 + zone/client.h | 2 +- zone/client_process.cpp | 2 +- zone/inventory.cpp | 143 +++++++++++++++---------------------- 4 files changed, 62 insertions(+), 87 deletions(-) diff --git a/common/inventory_profile.h b/common/inventory_profile.h index b1d1e4850..8e364aa5a 100644 --- a/common/inventory_profile.h +++ b/common/inventory_profile.h @@ -218,6 +218,8 @@ namespace EQ std::map& GetPersonal() { return m_inv; } int16 HasEvolvingItem(uint64 evolve_unique_id, uint8 quantity, uint8 where); + inline int16 PushItem(int16 slot_id, ItemInstance* inst) { return _PutItem(slot_id, inst); } + protected: /////////////////////////////// // Protected Methods diff --git a/zone/client.h b/zone/client.h index d7a61f662..49c691657 100644 --- a/zone/client.h +++ b/zone/client.h @@ -1175,7 +1175,7 @@ public: void Escape(); //keep or quest function void DisenchantSummonedBags(bool client_update = true); void RemoveNoRent(bool client_update = true); - void RemoveDuplicateLore(bool client_update = true); + void RemoveDuplicateLore(); void MoveSlotNotAllowed(bool client_update = true); virtual bool RangedAttack(Mob* other, bool CanDoubleAttack = false); virtual void ThrowingAttack(Mob* other, bool CanDoubleAttack = false); diff --git a/zone/client_process.cpp b/zone/client_process.cpp index 2f70dcb2d..7a61db35e 100644 --- a/zone/client_process.cpp +++ b/zone/client_process.cpp @@ -764,7 +764,7 @@ void Client::BulkSendInventoryItems() RemoveNoRent(false); } - RemoveDuplicateLore(false); + RemoveDuplicateLore(); MoveSlotNotAllowed(false); EQ::OutBuffer ob; diff --git a/zone/inventory.cpp b/zone/inventory.cpp index f89c1005c..e031d5208 100644 --- a/zone/inventory.cpp +++ b/zone/inventory.cpp @@ -2979,107 +2979,72 @@ void Client::RemoveNoRent(bool client_update) } // Two new methods to alleviate perpetual login desyncs -void Client::RemoveDuplicateLore(bool client_update) +void Client::RemoveDuplicateLore() { - for (auto slot_id = EQ::invslot::EQUIPMENT_BEGIN; slot_id <= EQ::invslot::EQUIPMENT_END; ++slot_id) { - if ((((uint64)1 << slot_id) & GetInv().GetLookup()->PossessionsBitmask) == 0) + for (auto slot_id : GetInventorySlots()) { + if ((((uint64) 1 << slot_id) & GetInv().GetLookup()->PossessionsBitmask) == 0) { continue; - - auto inst = m_inv.PopItem(slot_id); - if (inst == nullptr) { continue; } - if(CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from slot [{}]", inst->GetItem()->Name, slot_id); - database.SaveInventory(character_id, nullptr, slot_id); } - else { - m_inv.PutItem(slot_id, *inst); - } - safe_delete(inst); - } - for (auto slot_id = EQ::invslot::GENERAL_BEGIN; slot_id <= EQ::invslot::GENERAL_END; ++slot_id) { - if ((((uint64)1 << slot_id) & GetInv().GetLookup()->PossessionsBitmask) == 0) + // ignore shared bank slots + if (slot_id >= EQ::invslot::SHARED_BANK_BEGIN && slot_id <= EQ::invslot::SHARED_BANK_END) { continue; + } + if (slot_id >= EQ::invbag::SHARED_BANK_BAGS_BEGIN && slot_id <= EQ::invbag::SHARED_BANK_BAGS_END) { + continue; + } + + // slot gets handled in a queue + if (slot_id == EQ::invslot::slotCursor) { + continue; + } + + // temporarily move the item off of the slot auto inst = m_inv.PopItem(slot_id); - if (inst == nullptr) { continue; } + if (!inst) { + continue; + } + if (CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from slot [{}]", inst->GetItem()->Name, slot_id); + LogError( + "Lore Duplication Error | Deleting [{}] ({}) from slot [{}] client [{}]", + inst->GetItem()->Name, + inst->GetItem()->ID, + slot_id, + GetCleanName() + ); database.SaveInventory(character_id, nullptr, slot_id); + safe_delete(inst); } - else { - m_inv.PutItem(slot_id, *inst); - } - safe_delete(inst); + + // if no lore conflict, put the item back in the slot + m_inv.PushItem(slot_id, inst); } - for (auto slot_id = EQ::invbag::GENERAL_BAGS_BEGIN; slot_id <= EQ::invbag::CURSOR_BAG_END; ++slot_id) { - auto temp_slot = EQ::invslot::GENERAL_BEGIN + ((slot_id - EQ::invbag::GENERAL_BAGS_BEGIN) / EQ::invbag::SLOT_COUNT); - if ((((uint64)1 << temp_slot) & GetInv().GetLookup()->PossessionsBitmask) == 0) - continue; - - auto inst = m_inv.PopItem(slot_id); - if (inst == nullptr) { continue; } - if(CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from slot [{}]", inst->GetItem()->Name, slot_id); - database.SaveInventory(character_id, nullptr, slot_id); - } - else { - m_inv.PutItem(slot_id, *inst); - } - safe_delete(inst); - } - - for (auto slot_id = EQ::invslot::BANK_BEGIN; slot_id <= EQ::invslot::BANK_END; ++slot_id) { - if ((slot_id - EQ::invslot::BANK_BEGIN) >= GetInv().GetLookup()->InventoryTypeSize.Bank) - continue; - - auto inst = m_inv.PopItem(slot_id); - if (inst == nullptr) { continue; } - if(CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from slot [{}]", inst->GetItem()->Name, slot_id); - database.SaveInventory(character_id, nullptr, slot_id); - } - else { - m_inv.PutItem(slot_id, *inst); - } - safe_delete(inst); - } - - for (auto slot_id = EQ::invbag::BANK_BAGS_BEGIN; slot_id <= EQ::invbag::BANK_BAGS_END; ++slot_id) { - auto temp_slot = (slot_id - EQ::invbag::BANK_BAGS_BEGIN) / EQ::invbag::SLOT_COUNT; - if (temp_slot >= GetInv().GetLookup()->InventoryTypeSize.Bank) - continue; - - auto inst = m_inv.PopItem(slot_id); - if (inst == nullptr) { continue; } - if(CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from slot [{}]", inst->GetItem()->Name, slot_id); - database.SaveInventory(character_id, nullptr, slot_id); - } - else { - m_inv.PutItem(slot_id, *inst); - } - safe_delete(inst); - } - - // Shared Bank and Shared Bank Containers are not checked due to their allowing duplicate lore items - if (!m_inv.CursorEmpty()) { std::list local_1; std::list local_2; while (!m_inv.CursorEmpty()) { auto inst = m_inv.PopItem(EQ::invslot::slotCursor); - if (inst == nullptr) { continue; } + if (!inst) { + continue; + } local_1.push_back(inst); } - for (auto iter = local_1.begin(); iter != local_1.end(); ++iter) { - auto inst = *iter; - if (inst == nullptr) { continue; } + for (auto inst: local_1) { + if (!inst) { + continue; + } if (CheckLoreConflict(inst->GetItem())) { - LogInventory("Lore Duplication Error: Deleting [{}] from `Limbo`", inst->GetItem()->Name); + LogError( + "Lore Duplication Error | Deleting [{}] ({}) from `Limbo` client [{}]", + inst->GetItem()->Name, + inst->GetItem()->ID, + GetCleanName() + ); safe_delete(inst); } else { @@ -3088,17 +3053,25 @@ void Client::RemoveDuplicateLore(bool client_update) } local_1.clear(); - for (auto iter = local_2.begin(); iter != local_2.end(); ++iter) { - auto inst = *iter; - if (inst == nullptr) { continue; } + for (auto inst: local_2) { + if (!inst) { + continue; + } if (!inst->GetItem()->LoreFlag || - ((inst->GetItem()->LoreGroup == -1) && (m_inv.HasItem(inst->GetID(), 0, invWhereCursor) == INVALID_INDEX)) || - (inst->GetItem()->LoreGroup && (~inst->GetItem()->LoreGroup) && (m_inv.HasItemByLoreGroup(inst->GetItem()->LoreGroup, invWhereCursor) == INVALID_INDEX)) + ((inst->GetItem()->LoreGroup == -1) && + (m_inv.HasItem(inst->GetID(), 0, invWhereCursor) == INVALID_INDEX)) || + (inst->GetItem()->LoreGroup && (~inst->GetItem()->LoreGroup) && + (m_inv.HasItemByLoreGroup(inst->GetItem()->LoreGroup, invWhereCursor) == INVALID_INDEX)) ) { m_inv.PushCursor(*inst); } else { - LogInventory("Lore Duplication Error: Deleting [{}] from `Limbo`", inst->GetItem()->Name); + LogError( + "Lore Duplication Error | Deleting [{}] ({}) from `Limbo` client [{}]", + inst->GetItem()->Name, + inst->GetItem()->ID, + GetCleanName() + ); } safe_delete(inst); }