Possible memory leak - depots not getting deleted from memory at player deletion

[djseban]

Hey guys,

When working on one feature, I was poking around the code of TFS and in Player class destructor I spotted one thing:

Player::~Player()
{
	for (Item* item : inventory) {
		if (item) {
			item->setParent(nullptr);
			item->decrementReferenceCounter();
		}
	}

	for (const auto& it : depotLockerMap) {
		it.second->removeInbox(inbox);
		it.second->decrementReferenceCounter();
	}

	inbox->decrementReferenceCounter();

	storeInbox->setParent(nullptr);
	storeInbox->decrementReferenceCounter();

	setWriteItem(nullptr);
	setEditHouse(nullptr);
}

While in depotLockerMap the inbox is removed, the depot chests of player (depotChests std::map) remain untouched. Theoretically depot chests should get deleted due to
it.second->decrementReferenceCounter();
being called, but quick modification of decrementReferenceCounter in item.h

void decrementReferenceCounter() {
   if (getID() == ITEM_DEPOT) {
      std::cout << "decrement depot " << referenceCounter <<std::endl;
   }
   if (--referenceCounter == 0) {
      delete this;
   }
}

reveals that in fact no depot chest is getting deleted (we don't get any cout in terminal). Moreover, by accessing previously saved pointer to some Item that was in the depot of deleted player, we are able to retrieve it. The easiest way I found to reproduce this is following:

1. Place one item of choice in depot chest. The item has to remain on first slot of depot chest.
2. Execute following lua code:

local dpId = 3
local item = player:getDepotChest(dpId):getItem(0) 
local pos = player:getPosition() 
player:remove() 
addEvent(function() Tile(pos):addItemEx(item:clone()) end, 5000)

3. Lua code will throw you out of a game. Log in into the same or the other character to witness item being created on the tile you logged out with first character.
   Expected behavior of the TFS is to crash - due to accessing the item that doesn't really exist in memory, which it does, if we try the same thing, but with the inbox of a player:

local item = player:getInbox():getItem(0) 
local pos = player:getPosition() 
player:remove() 
addEvent(function() Tile(pos):addItemEx(item:clone()) end, 5000)

It's pretty hard to believe this kind of memory leak was in TFS for such long time. Obviously, it does not eat that much RAM, nor allows cloning of any items without specific code, but nevertheless I guess it's potential memory leak. Am I wrong or missing something?

[ranisalt]

The whole increment/decrementReferenceCounter smells to me what should have always been a std::shared_ptr, or probably should have been converted to.

[nekiro]

@ranisalt true, but the problem is the scale of this code, it's literally remaking whole server

[ghost]

@djseban can you please review the #3423 ?