Fix depot memory leak (#4879)

Author: nekiro

src/container.cpp

@@ -692,6 +692,15 @@ void Container::postRemoveNotification(Thing* thing, const Cylinder* newParent,
 	}
 }
 
+void Container::internalRemoveThing(Thing* thing)
+{
+	auto cit = std::find(itemlist.begin(), itemlist.end(), thing);
+	if (cit == itemlist.end()) {
+		return;
+	}
+	itemlist.erase(cit);
+}
+
 void Container::internalAddThing(Thing* thing) { internalAddThing(0, thing); }
 
 void Container::internalAddThing(uint32_t, Thing* thing)

src/container.h

@@ -111,6 +111,7 @@ class Container : public Item, public Cylinder
 	void postRemoveNotification(Thing* thing, const Cylinder* newParent, int32_t index,
 	                            cylinderlink_t link = LINK_OWNER) override;
 
+	void internalRemoveThing(Thing* thing) override final;
 	void internalAddThing(Thing* thing) override final;
 	void internalAddThing(uint32_t index, Thing* thing) override final;
 	void startDecaying() override final;

src/cylinder.cpp

@@ -22,6 +22,11 @@ std::map<uint32_t, uint32_t>& Cylinder::getAllItemTypeCount(std::map<uint32_t, u
 
 Thing* Cylinder::getThing(size_t) const { return nullptr; }
 
+void Cylinder::internalRemoveThing(Thing*)
+{
+	//
+}
+
 void Cylinder::internalAddThing(Thing*)
 {
 	//

src/cylinder.h

@@ -174,6 +174,12 @@ class Cylinder : virtual public Thing
 	 */
 	virtual std::map<uint32_t, uint32_t>& getAllItemTypeCount(std::map<uint32_t, uint32_t>& countMap) const;
 
+	/**
+	 * Removes an object from the cylinder without sending to the client(s)
+	 * \param thing is the object to add
+	 */
+	virtual void internalRemoveThing(Thing* thing);
+
 	/**
 	 * Adds an object to the cylinder without sending to the client(s)
 	 * \param thing is the object to add

src/depotchest.h

@@ -6,6 +6,9 @@
 
 #include "container.h"
 
+class DepotChest;
+using DepotChest_ptr = std::shared_ptr<DepotChest>;
+
 class DepotChest final : public Container
 {
 public:

src/game.cpp

@@ -5565,7 +5565,7 @@ std::vector<Item*> Game::getMarketItemList(uint16_t wareId, uint16_t sufficientC
 
 	for (const auto& chest : player.depotChests) {
 		if (!chest.second->empty()) {
-			containers.push_front(chest.second);
+			containers.push_front(chest.second.get());
 		}
 	}
 

src/iologindata.cpp

@@ -410,8 +410,7 @@ bool IOLoginData::loadPlayer(Player* player, DBResult_ptr result)
 
 			int32_t pid = pair.second;
 			if (pid >= 0 && pid < 100) {
-				DepotChest* depotChest = player->getDepotChest(pid, true);
-				if (depotChest) {
+				if (const auto& depotChest = player->getDepotChest(pid, true)) {
 					depotChest->internalAddThing(item);
 				}
 			} else {

src/luascript.cpp

@@ -9087,10 +9087,10 @@ int LuaScriptInterface::luaPlayerGetDepotChest(lua_State* L)
 
 	uint32_t depotId = tfs::lua::getNumber<uint32_t>(L, 2);
 	bool autoCreate = tfs::lua::getBoolean(L, 3, false);
-	DepotChest* depotChest = player->getDepotChest(depotId, autoCreate);
+	const auto& depotChest = player->getDepotChest(depotId, autoCreate);
 	if (depotChest) {
-		tfs::lua::pushUserdata<Item>(L, depotChest);
-		tfs::lua::setItemMetatable(L, -1, depotChest);
+		pushSharedPtr(L, depotChest);
+		tfs::lua::setItemMetatable(L, -1, depotChest.get());
 	} else {
 		tfs::lua::pushBoolean(L, false);
 	}

src/player.cpp

@@ -57,6 +57,14 @@ Player::~Player()
 		}
 	}
 
+	for (const auto& [_, depot] : depotChests) {
+		if (Cylinder* parent = depot->getRealParent()) {
+			// remove chest from depot locker, because of possible double free when shared_ptr decides to free up the
+			// resource
+			parent->internalRemoveThing(depot.get());
+		}
+	}
+
 	if (depotLocker) {
 		depotLocker->removeInbox(inbox.get());
 	}
@@ -808,7 +816,7 @@ bool Player::isNearDepotBox() const
 	return false;
 }
 
-DepotChest* Player::getDepotChest(uint32_t depotId, bool autoCreate)
+DepotChest_ptr Player::getDepotChest(uint32_t depotId, bool autoCreate)
 {
 	auto it = depotChests.find(depotId);
 	if (it != depotChests.end()) {
@@ -824,9 +832,10 @@ DepotChest* Player::getDepotChest(uint32_t depotId, bool autoCreate)
 		return nullptr;
 	}
 
-	it = depotChests.emplace(depotId, new DepotChest(depotItemId)).first;
-	it->second->setMaxDepotItems(getMaxDepotItems());
-	return it->second;
+	const DepotChest_ptr& depotChest =
+	    depotChests.emplace(depotId, std::make_shared<DepotChest>(depotItemId)).first->second;
+	depotChest->setMaxDepotItems(getMaxDepotItems());
+	return depotChest;
 }
 
 DepotLocker& Player::getDepotLocker()
@@ -839,8 +848,8 @@ DepotLocker& Player::getDepotLocker()
 		DepotChest* depotChest = new DepotChest(ITEM_DEPOT, false);
 		// adding in reverse to align them from first to last
 		for (int16_t depotId = depotChest->capacity(); depotId >= 0; --depotId) {
-			if (DepotChest* box = getDepotChest(depotId, true)) {
-				depotChest->internalAddThing(box);
+			if (const auto& box = getDepotChest(depotId, true)) {
+				depotChest->internalAddThing(box.get());
 			}
 		}
 
@@ -3199,7 +3208,7 @@ void Player::postRemoveNotification(Thing* thing, const Cylinder* newParent, int
 					bool isOwner = false;
 
 					for (const auto& it : depotChests) {
-						if (it.second == depotChest) {
+						if (it.second.get() == depotChest) {
 							isOwner = true;
 							onSendContainer(container);
 						}

src/player.h

@@ -6,6 +6,7 @@
 
 #include "creature.h"
 #include "cylinder.h"
+#include "depotchest.h"
 #include "depotlocker.h"
 #include "enums.h"
 #include "groups.h"
@@ -15,7 +16,6 @@
 #include "town.h"
 #include "vocation.h"
 
-class DepotChest;
 class House;
 struct Mount;
 class NetworkMessage;
@@ -363,7 +363,7 @@ class Player final : public Creature, public Cylinder
 	void addConditionSuppressions(uint32_t conditions);
 	void removeConditionSuppressions(uint32_t conditions);
 
-	DepotChest* getDepotChest(uint32_t depotId, bool autoCreate);
+	DepotChest_ptr getDepotChest(uint32_t depotId, bool autoCreate);
 	DepotLocker& getDepotLocker();
 	void onReceiveMail() const;
 	bool isNearDepotBox() const;
@@ -1174,7 +1174,7 @@ class Player final : public Creature, public Cylinder
 	std::unordered_set<uint32_t> VIPList;
 
 	std::map<uint8_t, OpenContainer> openContainers;
-	std::map<uint32_t, DepotChest*> depotChests;
+	std::map<uint32_t, DepotChest_ptr> depotChests;
 
 	std::map<uint16_t, uint8_t> outfits;
 	std::unordered_set<uint16_t> mounts;

src/protocolgame.cpp

@@ -2104,7 +2104,7 @@ void ProtocolGame::sendMarketEnter()
 
 	for (const auto& chest : player->depotChests) {
 		if (!chest.second->empty()) {
-			containerList.push_front(chest.second);
+			containerList.push_front(chest.second.get());
 		}
 	}