Rework in MoveEvent and MoveEvents class for fix memory leak (#398)

Movements completely redone to remove a memory leak related to the registration of movements, in the luaMoveEventRegister function, where the memory was fread and then used. We also added some pointer sanity checks, preventing future crashes and fixing some known ones.

Removed old XML load
Passed some objects as a reference, to avoid nullpointer
Added nullpointer checks in some places and the code was redone to work better
Added some logs to help in case of errors
Fixed crash related to replaceable magic fields

Notes: MoveEvent::onRemoveItem function from now on it will no longer have the "tileitem" argument

data/scripts/movements/trap.lua

@@ -41,14 +41,14 @@ trap:register()
 trap = MoveEvent()
 trap:type("removeitem")
 
-function trap.onRemoveItem(moveitem, tileitem, position)
-	local itemPosition = moveitem:getPosition()
+function trap.onRemoveItem(item, position)
+	local itemPosition = item:getPosition()
 	if itemPosition:getDistance(position) > 0 then
-		tileitem:transform(tileitem.itemid - 1)
+		item:transform(tileitem.itemid - 1)
 		position:sendMagicEffect(CONST_ME_POFF)
 	end
 	return true
 end
 
-trap:id(2579)
+trap:id(3482)
 trap:register()

src/creatures/combat/combat.cpp

@@ -1562,11 +1562,11 @@ void AreaCombat::setupExtArea(const std::list<uint32_t>& list, uint32_t rows)
 
 //**********************************************************//
 
-void MagicField::onStepInField(Creature* creature)
+void MagicField::onStepInField(Creature& creature)
 {
 	//remove magic walls/wild growth
 	if (id == ITEM_MAGICWALL || id == ITEM_WILDGROWTH || id == ITEM_MAGICWALL_SAFE || id == ITEM_WILDGROWTH_SAFE || isBlocking()) {
-		if (!creature->isInGhostMode()) {
+		if (!creature.isInGhostMode()) {
 			g_game().internalRemoveItem(this, 1);
 		}
 
@@ -1589,7 +1589,7 @@ void MagicField::onStepInField(Creature* creature)
 				}
 			}
 
-			Player* targetPlayer = creature->getPlayer();
+			Player* targetPlayer = creature.getPlayer();
 			if (targetPlayer) {
 				const Player* attackerPlayer = g_game().getPlayerByID(ownerId);
 				if (attackerPlayer) {
@@ -1599,11 +1599,11 @@ void MagicField::onStepInField(Creature* creature)
 				}
 			}
 
-			if (!harmfulField || (OTSYS_TIME() - createTime <= 5000) || creature->hasBeenAttacked(ownerId)) {
+			if (!harmfulField || (OTSYS_TIME() - createTime <= 5000) || creature.hasBeenAttacked(ownerId)) {
 				conditionCopy->setParam(CONDITION_PARAM_OWNER, ownerId);
 			}
 		}
 
-		creature->addCondition(conditionCopy);
+		creature.addCondition(conditionCopy);
 	}
 }

src/creatures/combat/combat.h

@@ -343,7 +343,7 @@ class MagicField final : public Item
 			}
 			return 0;
 		}
-		void onStepInField(Creature* creature);
+		void onStepInField(Creature& creature);
 
 	private:
 		int64_t createTime;

src/creatures/players/player.cpp

@@ -1510,7 +1510,7 @@ void Player::onCreatureAppear(Creature* creature, bool isLogin)
 			Item* item = inventory[slot];
 			if (item) {
 				item->startDecaying();
-				g_moveEvents().onPlayerEquip(this, item, static_cast<Slots_t>(slot), false);
+				g_moveEvents().onPlayerEquip(*this, *item, static_cast<Slots_t>(slot), false);
 			}
 		}
 
@@ -2976,6 +2976,7 @@ ReturnValue Player::queryAdd(int32_t index, const Thing& thing, uint32_t count,
 {
 	const Item* item = thing.getItem();
 	if (item == nullptr) {
+		SPDLOG_ERROR("[Player::queryAdd] - Item is nullptr");
 		return RETURNVALUE_NOTPOSSIBLE;
 	}
 
@@ -3189,7 +3190,7 @@ ReturnValue Player::queryAdd(int32_t index, const Thing& thing, uint32_t count,
 			return RETURNVALUE_NOTENOUGHCAPACITY;
 		}
 
-		if (!g_moveEvents().onPlayerEquip(const_cast<Player*>(this), const_cast<Item*>(item), static_cast<Slots_t>(index), true)) {
+		if (!g_moveEvents().onPlayerEquip(const_cast<Player&>(*this), const_cast<Item&>(*item), static_cast<Slots_t>(index), true)) {
 			return RETURNVALUE_CANNOTBEDRESSED;
 		}
 	}
@@ -3795,7 +3796,7 @@ void Player::postAddNotification(Thing* thing, const Cylinder* oldParent, int32_
 {
 	if (link == LINK_OWNER) {
 		//calling movement scripts
-		g_moveEvents().onPlayerEquip(this, thing->getItem(), static_cast<Slots_t>(index), false);
+		g_moveEvents().onPlayerEquip(*this, *thing->getItem(), static_cast<Slots_t>(index), false);
 	}
 
 	bool requireListUpdate = true;
@@ -3850,7 +3851,7 @@ void Player::postRemoveNotification(Thing* thing, const Cylinder* newParent, int
 {
 	if (link == LINK_OWNER) {
 		//calling movement scripts
-		g_moveEvents().onPlayerDeEquip(this, thing->getItem(), static_cast<Slots_t>(index));
+		g_moveEvents().onPlayerDeEquip(*this, *thing->getItem(), static_cast<Slots_t>(index));
 	}
 
 	bool requireListUpdate = true;

src/game/game.cpp

@@ -2204,7 +2204,7 @@ Item* Game::transformItem(Item* item, uint16_t newId, int32_t newCount /*= -1*/)
 					internalRemoveItem(item);
 					return nullptr;
 				} else if (newItemId != newId) {
-					//Replacing the the old item with the new while maintaining the old position
+					// Replacing the the old item with the new while maintaining the old position
 					Item* newItem = Item::CreateItem(newItemId, 1);
 					if (newItem == nullptr) {
 						return nullptr;
@@ -5385,6 +5385,32 @@ void Game::changeSpeed(Creature* creature, int32_t varSpeedDelta)
 	}
 }
 
+void Game::changePlayerSpeed(Player& player, int32_t varSpeedDelta)
+{
+	int32_t varSpeed = player.getSpeed() - player.getBaseSpeed();
+	varSpeed += varSpeedDelta;
+
+	player.setSpeed(varSpeed);
+
+	// Send new player speed to the spectators
+	SpectatorHashSet spectators;
+	map.getSpectators(spectators, player.getPosition(), false, true);
+	for (Creature* creatureSpectator : spectators) {
+		if (creatureSpectator == nullptr) {
+			SPDLOG_ERROR("[Game::changePlayerSpeed] - Creature spectator is nullptr");
+			continue;
+		}
+
+		const Player *playerSpectator = creatureSpectator->getPlayer();
+		if (playerSpectator == nullptr) {
+			SPDLOG_ERROR("[Game::changePlayerSpeed] - Player spectator is nullptr");
+			continue;
+		}
+
+		playerSpectator->sendChangeSpeed(&player, player.getStepSpeed());
+	}
+}
+
 void Game::internalCreatureChangeOutfit(Creature* creature, const Outfit_t& outfit)
 {
 	if (!g_events().eventCreatureOnChangeOutfit(creature, outfit)) {
@@ -8617,7 +8643,7 @@ bool Game::reload(ReloadTypes_t reloadType)
 			// commented out stuff is TODO, once we approach further in revscriptsys
 			g_actions().clear(true);
 			g_creatureEvents().clear(true);
-			g_moveEvents().clear(true);
+			g_moveEvents().clear();
 			g_talkActions().clear(true);
 			g_globalEvents().clear(true);
 			g_weapons().clear(true);
@@ -8657,11 +8683,6 @@ bool Game::reload(ReloadTypes_t reloadType)
 	return true;
 }
 
-bool Game::itemidHasMoveevent(uint32_t itemid)
-{
-	return g_moveEvents().isRegistered(itemid);
-}
-
 bool Game::hasEffect(uint8_t effectId) {
 	for (uint8_t i = CONST_ME_NONE; i <= CONST_ME_LAST; i++) {
 		MagicEffectClasses effect = static_cast<MagicEffectClasses>(i);

src/game/game.h

@@ -404,6 +404,7 @@ class Game
 		bool isSightClear(const Position& fromPos, const Position& toPos, bool sameFloor) const;
 
 		void changeSpeed(Creature* creature, int32_t varSpeedDelta);
+		void changePlayerSpeed(Player& player, int32_t varSpeedDelta);
 		void internalCreatureChangeOutfit(Creature* creature, const Outfit_t& oufit);
 		void internalCreatureChangeVisible(Creature* creature, bool visible);
 		void changeLight(const Creature* creature);
@@ -490,7 +491,6 @@ class Game
 
 		bool reload(ReloadTypes_t reloadType);
 
-		bool itemidHasMoveevent(uint32_t itemid);
 		bool hasEffect(uint8_t effectId);
 		bool hasDistanceEffect(uint8_t effectId);
 

src/items/tile.cpp

@@ -1426,9 +1426,9 @@ void Tile::postAddNotification(Thing* thing, const Cylinder* oldParent, int32_t
 
 		//calling movement scripts
 		if (creature) {
-			g_moveEvents().onCreatureMove(creature, this, MOVE_EVENT_STEP_IN);
+			g_moveEvents().onCreatureMove(*creature, *this, MOVE_EVENT_STEP_IN);
 		} else if (item) {
-			g_moveEvents().onItemMove(item, this, true);
+			g_moveEvents().onItemMove(*item, *this, true);
 		}
 	}
 
@@ -1456,11 +1456,11 @@ void Tile::postRemoveNotification(Thing* thing, const Cylinder* newParent, int32
 	//calling movement scripts
 	Creature* creature = thing->getCreature();
 	if (creature) {
-		g_moveEvents().onCreatureMove(creature, this, MOVE_EVENT_STEP_OUT);
+		g_moveEvents().onCreatureMove(*creature, *this, MOVE_EVENT_STEP_OUT);
 	} else {
 		Item* item = thing->getItem();
 		if (item) {
-			g_moveEvents().onItemMove(item, this, false);
+			g_moveEvents().onItemMove(*item, *this, false);
 		}
 	}
 }

src/lua/creature/actions.cpp

@@ -27,7 +27,6 @@
 #include "creatures/combat/spells.h"
 #include "items/containers/rewards/rewardchest.h"
 
-
 Actions::Actions() :
 	scriptInterface("Action Interface") {
 	scriptInterface.initState();