fix: crash when removing items during iteration with ContainerIterator (

opentibiabr#2901)


The enhancements to the ContainerIterator address critical issues
related to deeply nested and cyclic container structures. By
implementing cycle detection, traversal depth limitation, the iterator
becomes a more reliable and efficient tool for managing complex
container hierarchies. These improvements are essential for maintaining
the system's performance and stability as it scales and evolves.

OBS: Added a talkaction that will make it easier to test (both counting
items/subcontainers, which will call the getItems function, which
internally calls the ContainerIterator). This will make it easier to
know if the modification was done correctly.

config.lua.dist

@@ -66,8 +66,6 @@ serverMotd = "Welcome to the OTServBR-Global!"
 statusTimeout = 5 * 1000
 replaceKickOnLogin = true
 maxPacketsPerSecond = 25
-maxItem = 2000
-maxContainer = 100
 maxPlayersOnlinePerAccount = 1
 maxPlayersOutsidePZPerAccount = 1
 
@@ -81,6 +79,12 @@ freeDepotLimit = 2000
 premiumDepotLimit = 10000
 depotBoxes = 20
 
+-- Item and containers limit
+-- NOTE: 'maxContainerDepth' defines the maximum depth to which containers can be nested
+maxItem = 5000
+maxContainer = 500
+maxContainerDepth = 200
+
 -- Augments System (Get more info in: https://github.com/opentibiabr/canary/pull/2602)
 -- NOTE: the following values are for all weapons and equipments that have type of "increase damage", "powerful impact" and "strong impact".
 -- To customize the percentage of a particular item with these augment types, please add to the item "augments" section on items.xml as the example above.

data/scripts/talkactions/god/test.lua

@@ -36,3 +36,49 @@ end
 testLog:separator(" ")
 testLog:groupType("god")
 testLog:register()
+
+-- @module ContainerTalkAction
+-- @function Handles the "!testcontainer" TalkAction command for testing the ContainerIterator functionality.
+--
+-- This module defines a TalkAction that allows players to inspect their backpack containers.
+-- It can optionally remove items from the container based on the provided parameter.
+-- The command logs the total number of items and subcontainers found in the backpack.
+-- This is primarily used to verify the correctness of changes made to the ContainerIterator.
+local containerTalkAction = TalkAction("!testcontainer")
+
+function containerTalkAction.onSay(player, words, param)
+	local container = player:getSlotItem(CONST_SLOT_BACKPACK)
+	if not container then
+		player:sendCancelMessage("Your backpack does not contain a valid container.")
+		logger.error("[!container] - Player: {} has a backpack without a valid container.", player:getName())
+		return true
+	end
+
+	local shouldRemove = (param and param:lower() == "remove") and true or false
+	local items = container:getItems(true)
+	local totalItems = 0
+	local totalSubContainers = 0
+	for i, item in pairs(items) do
+		if shouldRemove and item then
+			item:remove()
+		end
+
+		if item:getContainer() then
+			totalSubContainers = totalSubContainers + 1
+		else
+			totalItems = totalItems + 1
+		end
+	end
+
+	local actionMessage = shouldRemove and "removed " or "have "
+	local playerMessage = actionMessage .. totalItems .. " items and " .. totalSubContainers .. " subcontainers from your backpack."
+	local finalMessage = string.format("[!testcontainer] - Player: %s, %s items from backpack: %d, subcontainers count: %d", player:getName(), actionMessage, totalItems, totalSubContainers)
+
+	logger.info(finalMessage)
+	player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You " .. playerMessage)
+	return true
+end
+
+containerTalkAction:separator(" ")
+containerTalkAction:groupType("god")
+containerTalkAction:register()

src/config/config_enums.hpp

@@ -142,6 +142,7 @@ enum ConfigKey_t : uint16_t {
 	MAX_ALLOWED_ON_A_DUMMY,
 	MAX_CONTAINER_ITEM,
 	MAX_CONTAINER,
+	MAX_CONTAINER_DEPTH,
 	MAX_DAMAGE_REFLECTION,
 	MAX_ELEMENTAL_RESISTANCE,
 	MAX_MARKET_OFFERS_AT_A_TIME_PER_PLAYER,

src/config/configmanager.cpp

@@ -269,6 +269,7 @@ bool ConfigManager::load() {
 	loadIntConfig(L, MAX_ALLOWED_ON_A_DUMMY, "maxAllowedOnADummy", 1);
 	loadIntConfig(L, MAX_CONTAINER_ITEM, "maxItem", 5000);
 	loadIntConfig(L, MAX_CONTAINER, "maxContainer", 500);
+	loadIntConfig(L, MAX_CONTAINER_DEPTH, "maxContainerDepth", 200);
 	loadIntConfig(L, MAX_DAMAGE_REFLECTION, "maxDamageReflection", 200);
 	loadIntConfig(L, MAX_ELEMENTAL_RESISTANCE, "maxElementalResistance", 200);
 	loadIntConfig(L, MAX_MARKET_OFFERS_AT_A_TIME_PER_PLAYER, "maxMarketOffersAtATimePerPlayer", 100);

src/creatures/players/wheel/player_wheel.cpp

@@ -143,9 +143,7 @@ namespace {
 } // namespace
 
 PlayerWheel::PlayerWheel(Player &initPlayer) :
-	m_player(initPlayer) {
-	auto pointsPerLevel = (uint16_t)g_configManager().getNumber(WHEEL_POINTS_PER_LEVEL);
-	m_pointsPerLevel = pointsPerLevel > 0 ? pointsPerLevel : 1;
+	m_pointsPerLevel(g_configManager().getNumber(WHEEL_POINTS_PER_LEVEL)), m_player(initPlayer) {
 }
 
 bool PlayerWheel::canPlayerSelectPointOnSlot(WheelSlots_t slot, bool recursive) const {

src/items/containers/container.cpp

@@ -506,8 +506,8 @@ ReturnValue Container::queryAdd(int32_t addIndex, const std::shared_ptr<Thing> &
 		return RETURNVALUE_CONTAINERNOTENOUGHROOM;
 	}
 
-	if (const auto topParentContainer = getTopParentContainer()) {
-		if (const auto addContainer = item->getContainer()) {
+	if (const auto &topParentContainer = getTopParentContainer()) {
+		if (const auto &addContainer = item->getContainer()) {
 			uint32_t addContainerCount = addContainer->getContainerHoldingCount() + 1;
 			uint32_t maxContainer = static_cast<uint32_t>(g_configManager().getNumber(MAX_CONTAINER));
 			if (addContainerCount + topParentContainer->getContainerHoldingCount() > maxContainer) {
@@ -917,12 +917,7 @@ uint16_t Container::getFreeSlots() {
 }
 
 ContainerIterator Container::iterator() {
-	ContainerIterator cit;
-	if (!itemlist.empty()) {
-		cit.over.push_back(getContainer());
-		cit.cur = itemlist.begin();
-	}
-	return cit;
+	return { getContainer(), static_cast<size_t>(g_configManager().getNumber(MAX_CONTAINER_DEPTH)) };
 }
 
 void Container::removeItem(std::shared_ptr<Thing> thing, bool sendUpdateToClient /* = false*/) {
@@ -947,39 +942,125 @@ void Container::removeItem(std::shared_ptr<Thing> thing, bool sendUpdateToClient
 	}
 }
 
-std::shared_ptr<Item> ContainerIterator::operator*() {
-	return *cur;
+uint32_t Container::getOwnerId() const {
+	uint32_t ownerId = Item::getOwnerId();
+	if (ownerId > 0) {
+		return ownerId;
+	}
+	for (const auto &item : itemlist) {
+		ownerId = item->getOwnerId();
+		if (ownerId > 0) {
+			return ownerId;
+		}
+	}
+	return 0;
+}
+
+/**
+ * ContainerIterator
+ * @brief Iterator for iterating over the items in a container
+ */
+ContainerIterator::ContainerIterator(const std::shared_ptr<Container> &container, size_t maxDepth) :
+	maxTraversalDepth(maxDepth) {
+	if (container) {
+		states.reserve(maxDepth);
+		visitedContainers.reserve(g_configManager().getNumber(MAX_CONTAINER));
+		(void)states.emplace_back(container, 0, 1);
+		(void)visitedContainers.insert(container);
+	}
+}
+
+bool ContainerIterator::hasNext() const {
+	while (!states.empty()) {
+		const auto &top = states.back();
+		const auto &container = top.container.lock();
+		if (!container) {
+			// Container has been deleted
+			states.pop_back();
+		} else if (top.index < container->itemlist.size()) {
+			return true;
+		} else {
+			states.pop_back();
+		}
+	}
+	return false;
 }
 
 void ContainerIterator::advance() {
-	if (std::shared_ptr<Item> i = *cur) {
-		if (std::shared_ptr<Container> c = i->getContainer()) {
-			if (!c->empty()) {
-				over.push_back(c);
+	if (states.empty()) {
+		return;
+	}
+
+	auto &top = states.back();
+	const auto &container = top.container.lock();
+	if (!container) {
+		// Container has been deleted
+		states.pop_back();
+		return;
+	}
+
+	if (top.index >= container->itemlist.size()) {
+		states.pop_back();
+		return;
+	}
+
+	auto currentItem = container->itemlist[top.index];
+	if (currentItem) {
+		auto subContainer = currentItem->getContainer();
+		if (subContainer && !subContainer->itemlist.empty()) {
+			size_t newDepth = top.depth + 1;
+			if (newDepth <= maxTraversalDepth) {
+				if (visitedContainers.find(subContainer) == visitedContainers.end()) {
+					states.emplace_back(subContainer, 0, newDepth);
+					visitedContainers.insert(subContainer);
+				} else {
+					if (!m_cycleDetected) {
+						g_logger().trace("[{}] Cycle detected in container: {}", __FUNCTION__, subContainer->getName());
+						m_cycleDetected = true;
+					}
+				}
+			} else {
+				if (!m_maxDepthReached) {
+					g_logger().trace("[{}] Maximum iteration depth reached", __FUNCTION__);
+					m_maxDepthReached = true;
+				}
 			}
 		}
 	}
 
-	++cur;
+	++top.index;
+}
+
+std::shared_ptr<Item> ContainerIterator::operator*() const {
+	if (states.empty()) {
+		return nullptr;
+	}
 
-	if (cur == over.front()->itemlist.end()) {
-		over.pop_front();
-		if (!over.empty()) {
-			cur = over.front()->itemlist.begin();
+	const auto &top = states.back();
+	if (const auto &container = top.container.lock()) {
+		if (top.index < container->itemlist.size()) {
+			return container->itemlist[top.index];
 		}
 	}
+	return nullptr;
 }
 
-uint32_t Container::getOwnerId() const {
-	uint32_t ownerId = Item::getOwnerId();
-	if (ownerId > 0) {
-		return ownerId;
+bool ContainerIterator::hasReachedMaxDepth() const {
+	return m_maxDepthReached;
+}
+
+std::shared_ptr<Container> ContainerIterator::getCurrentContainer() const {
+	if (states.empty()) {
+		return nullptr;
 	}
-	for (const auto &item : itemlist) {
-		ownerId = item->getOwnerId();
-		if (ownerId > 0) {
-			return ownerId;
-		}
+	const auto &top = states.back();
+	return top.container.lock();
+}
+
+size_t ContainerIterator::getCurrentIndex() const {
+	if (states.empty()) {
+		return 0;
 	}
-	return 0;
+	const auto &top = states.back();
+	return top.index;
 }

src/items/containers/container.hpp

@@ -21,16 +21,111 @@ class Reward;
 
 class ContainerIterator {
 public:
-	bool hasNext() const {
-		return !over.empty();
-	}
-
+	/**
+	 * @brief Constructs a ContainerIterator with a specified container and maximum traversal depth.
+	 *
+	 * This constructor initializes the iterator to start iterating over the specified container
+	 * and ensures that it will not traverse deeper than the specified maxDepth.
+	 *
+	 * @param container The root container to start iterating from.
+	 * @param maxDepth The maximum depth of nested containers to traverse.
+	 */
+	ContainerIterator(const std::shared_ptr<Container> &container, size_t maxDepth);
+
+	/**
+	 * @brief Checks if there are more items to iterate over in the container.
+	 *
+	 * This function checks if there are more items to iterate over in the current container or
+	 * in any of the nested sub-containers. If no items are left, the function will return false.
+	 *
+	 * @return true if there are more items to iterate over; false otherwise.
+	 */
+	bool hasNext() const;
+
+	/**
+	 * @brief Advances the iterator to the next item in the container.
+	 *
+	 * This function moves the iterator to the next item. If the current item is a sub-container,
+	 * it adds that sub-container to the stack to iterate over its items as well.
+	 * It also handles maximum depth and cycle detection to prevent infinite loops.
+	 */
 	void advance();
-	std::shared_ptr<Item> operator*();
+
+	/**
+	 * @brief Returns the current item pointed to by the iterator.
+	 *
+	 * This function returns the current item in the container that the iterator points to.
+	 * If there are no more items to iterate over, it returns nullptr.
+	 *
+	 * @return A shared pointer to the current item, or nullptr if no items are left.
+	 */
+	std::shared_ptr<Item> operator*() const;
+
+	bool hasReachedMaxDepth() const;
+
+	std::shared_ptr<Container> getCurrentContainer() const;
+	size_t getCurrentIndex() const;
 
 private:
-	std::list<std::shared_ptr<Container>> over;
-	ItemDeque::const_iterator cur;
+	/**
+	 * @brief Represents the state of the iterator at a given point in time.
+	 *
+	 * This structure is used to keep track of the current container,
+	 * the index of the current item within that container, and the depth
+	 * of traversal for nested containers. It is primarily used in the
+	 * ContainerIterator to manage the state of the iteration as it traverses
+	 * through containers and their sub-containers.
+	 */
+	struct IteratorState {
+		/**
+		 * @brief The container being iterated over.
+		 */
+		std::weak_ptr<Container> container;
+
+		/**
+		 * @brief The current index within the container's item list.
+		 */
+		size_t index;
+
+		/**
+		 * @brief The depth of traversal, indicating how deep the iteration is
+		 * within nested sub-containers.
+		 */
+		size_t depth;
+
+		/**
+		 * @brief Constructs an IteratorState with the given container, index, and depth.
+		 *
+		 * @param c The container to iterate over.
+		 * @param i The starting index within the container.
+		 * @param d The depth of traversal.
+		 */
+		IteratorState(std::shared_ptr<Container> c, size_t i, size_t d) :
+			container(c), index(i), depth(d) { }
+	};
+
+	/**
+	 * @brief Stack of IteratorState objects representing the state of the iteration.
+	 *
+	 * This stack keeps track of the current position in each container and is
+	 * used to traverse containers and their nested sub-containers in a depth-first manner.
+	 * Each element in the stack represents a different level in the container hierarchy.
+	 */
+	mutable std::vector<IteratorState> states;
+
+	/**
+	 * @brief Set of containers that have already been visited during the iteration.
+	 *
+	 * This set is used to keep track of all containers that have been visited
+	 * to avoid revisiting them and causing infinite loops or cycles. It ensures
+	 * that each container is processed only once, preventing redundant processing
+	 * and potential crashes due to cyclic references.
+	 */
+	mutable std::unordered_set<std::shared_ptr<Container>> visitedContainers;
+	size_t maxTraversalDepth = 0;
+
+	bool m_maxDepthReached = false;
+	bool m_cycleDetected = false;
 
 	friend class Container;
 };