fix: loop in movement "stepin/out" and "addItem/remove" events (opent…

…ibiabr#742)

A potential infinite loop was discovered in specific scenarios involving teleport squares or items that teleport players to specific locations (such as when the player logs in). This pull request addresses the issue by disabling the onStepIn event during login and implementing additional security checks to prevent similar bugs from occurring in the future.

data/npclib/npc_system/modules.lua

@@ -223,16 +223,12 @@ if Modules == nil then
 		else
 			npcHandler:removeInteraction(npc, player)
 			npcHandler:say(parameters.text or "Set the sails!", npc, player)
-			playerPosition:sendMagicEffect(CONST_ME_TELEPORT)
 
 			local destination = parameters.destination
 			if type(destination) == 'function' then
 				destination = destination(player)
 			end
 
-			player:teleportTo(destination)
-			playerPosition:sendMagicEffect(CONST_ME_TELEPORT)
-
 			player:setStorageValue(NpcExhaust, 3 + os.time())
 			player:teleportTo(destination)
 			playerPosition:sendMagicEffect(CONST_ME_TELEPORT)

src/game/game.cpp

@@ -2135,7 +2135,9 @@ ReturnValue Game::internalTeleport(Thing* thing, const Position& newPos, bool pu
 		return RETURNVALUE_NOTPOSSIBLE;
 	}
 
-	if (thing->isRemoved()) {
+	if (newPos == thing->getPosition()) {
+		return RETURNVALUE_CONTACTADMINISTRATOR;
+	} else if (thing->isRemoved()) {
 		return RETURNVALUE_NOTPOSSIBLE;
 	}
 

src/lua/creature/movement.cpp

@@ -262,7 +262,6 @@ MoveEvent* MoveEvents::getEvent(Tile& tile, MoveEvent_t eventType) {
 
 uint32_t MoveEvents::onCreatureMove(Creature& creature, Tile& tile, MoveEvent_t eventType) {
 	const Position& pos = tile.getPosition();
-
 	uint32_t ret = 1;
 
 	MoveEvent *moveEvent = getEvent(tile, eventType);
@@ -283,7 +282,12 @@ uint32_t MoveEvents::onCreatureMove(Creature& creature, Tile& tile, MoveEvent_t
 
 		moveEvent = getEvent(*tileItem, eventType);
 		if (moveEvent) {
-			ret &= moveEvent->fireStepEvent(creature, tileItem, pos);
+			auto step = moveEvent->fireStepEvent(creature, tileItem, pos);
+			// If there is any problem in the function, we will kill the loop
+			if (step == 0) {
+				break;
+			}
+			ret &= step;
 		}
 	}
 	return ret;
@@ -341,7 +345,11 @@ uint32_t MoveEvents::onItemMove(Item& item, Tile& tile, bool isAdd) {
 
 		moveEvent = getEvent(*tileItem, eventType2);
 		if (moveEvent) {
-			ret &= moveEvent->fireAddRemItem(item, *tileItem, tile.getPosition());
+			auto moveItem = moveEvent->fireAddRemItem(item, *tileItem, tile.getPosition());
+			// If there is any problem in the function, we will kill the loop
+			if (moveItem == 0) {
+				break;
+			}
 		}
 
 	}
@@ -649,6 +657,23 @@ uint32_t MoveEvent::fireStepEvent(Creature& creature, Item* item, const Position
 bool MoveEvent::executeStep(Creature& creature, Item* item, const Position& pos) const {
 	//onStepIn(creature, item, pos, fromPosition)
 	//onStepOut(creature, item, pos, fromPosition)
+
+	// Check if the new position is the same as the old one
+	// If it is, log a warning and either teleport the player to their temple position if item type is an teleport
+	auto fromPosition = creature.getLastPosition();
+	if (auto player = creature.getPlayer(); item && fromPosition == pos && getEventType() == MOVE_EVENT_STEP_IN)
+	{
+		if (const ItemType& itemType = Item::items[item->getID()]; player && itemType.isTeleport())
+		{
+			SPDLOG_WARN("[{}] cannot teleport player: {}, to the same position: {} of fromPosition: {}", __FUNCTION__, player->getName(), pos.toString(), fromPosition.toString());
+			g_game().internalTeleport(player, player->getTemplePosition());
+			player->sendMagicEffect(player->getTemplePosition(), CONST_ME_TELEPORT);
+			player->sendCancelMessage(getReturnMessage(RETURNVALUE_CONTACTADMINISTRATOR));
+		}
+
+		return false;
+	}
+
 	if (!getScriptInterface()->reserveScriptEnv()) {
 		if (item != nullptr) {
 			SPDLOG_ERROR("[MoveEvent::executeStep - Creature {} item {}, position {}] "
@@ -674,7 +699,7 @@ bool MoveEvent::executeStep(Creature& creature, Item* item, const Position& pos)
 	LuaScriptInterface::setCreatureMetatable(L, -1, &creature);
 	LuaScriptInterface::pushThing(L, item);
 	LuaScriptInterface::pushPosition(L, pos);
-	LuaScriptInterface::pushPosition(L, creature.getLastPosition());
+	LuaScriptInterface::pushPosition(L, fromPosition);
 
 	return getScriptInterface()->callFunction(4);
 }

src/lua/functions/creatures/creature_functions.cpp

@@ -749,9 +749,25 @@ int CreatureFunctions::luaCreatureTeleportTo(lua_State* L) {
 		return 1;
 	}
 
+	const Player *player = creature->getPlayer();
+	if (!player) {
+		return 1;
+	}
+
 	const Position oldPosition = creature->getPosition();
-	if (g_game().internalTeleport(creature, position, pushMovement) != RETURNVALUE_NOERROR) {
-		SPDLOG_WARN("[{}] - Cannot teleport creature with name: {}, fromPosition {}, toPosition {}", __FUNCTION__, creature->getName(), oldPosition.toString(), position.toString());
+	if (oldPosition == position) {
+		SPDLOG_WARN("[{}] - Cannot teleport creature: {}, fromPosition: {}, as same of toPosition: {}", __FUNCTION__, player->getName(), oldPosition.toString(), position.toString());
+		player->sendCancelMessage(getReturnMessage(RETURNVALUE_CONTACTADMINISTRATOR));
+		pushBoolean(L, false);
+		reportErrorFunc("The new position is the same as the old one.");
+		return 1;
+	}
+
+	if (auto ret = g_game().internalTeleport(creature, position, pushMovement);
+		ret != RETURNVALUE_NOERROR)
+	{
+		player->sendCancelMessage(ret);
+		SPDLOG_ERROR("[{}] Failed to teleport player, error code: {}", __FUNCTION__, getReturnMessage(ret));
 		pushBoolean(L, false);
 		return 1;
 	}