fix: crash when removing items during iteration with ContainerIterator

opentibiabr · Sep 19, 2024 · d65ce9e · d65ce9e
1 parent 687aba5
commit d65ce9e
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 36 deletions.
diff --git a/src/items/containers/container.cpp b/src/items/containers/container.cpp
@@ -924,12 +924,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 ContainerIterator(getContainer());
 }
 
 void Container::removeItem(std::shared_ptr<Thing> thing, bool sendUpdateToClient /* = false*/) {
@@ -954,29 +949,6 @@ void Container::removeItem(std::shared_ptr<Thing> thing, bool sendUpdateToClient
 	}
 }
 
-std::shared_ptr<Item> ContainerIterator::operator*() {
-	return *cur;
-}
-
-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);
-			}
-		}
-	}
-
-	++cur;
-
-	if (cur == over.front()->itemlist.end()) {
-		over.pop_front();
-		if (!over.empty()) {
-			cur = over.front()->itemlist.begin();
-		}
-	}
-}
-
 uint32_t Container::getOwnerId() const {
 	uint32_t ownerId = Item::getOwnerId();
 	if (ownerId > 0) {
@@ -990,3 +962,73 @@ uint32_t Container::getOwnerId() const {
 	}
 	return 0;
 }
+
+/**
+ * ContainerIterator
+ * @brief Iterator for iterating over the items in a container
+ */
+ContainerIterator::ContainerIterator(std::shared_ptr<Container> container, size_t maxDepth) :
+	maxTraversalDepth(maxDepth) {
+	if (container) {
+		states.emplace(container, 0, 1);
+		visitedContainers.insert(container);
+	}
+}
+
+bool ContainerIterator::hasNext() const {
+	while (!states.empty()) {
+		auto &top = states.top();
+		if (top.index < top.container->itemlist.size()) {
+			return true;
+		} else {
+			states.pop();
+		}
+	}
+	return false;
+}
+
+void ContainerIterator::advance() {
+	if (states.empty()) {
+		return;
+	}
+
+	auto &top = states.top();
+	if (top.index >= top.container->itemlist.size()) {
+		states.pop();
+		return;
+	}
+
+	auto currentItem = top.container->itemlist[top.index];
+	if (currentItem) {
+		auto subContainer = currentItem->getContainer();
+		if (subContainer && !subContainer->itemlist.empty()) {
+			size_t newDepth = top.depth + 1;
+			if (newDepth <= maxTraversalDepth) {
+				// Check if we have already visited this container to avoid cycles
+				if (visitedContainers.find(subContainer) == visitedContainers.end()) {
+					states.emplace(subContainer, 0, newDepth);
+					visitedContainers.insert(subContainer);
+				} else {
+					// Cycle detection
+					g_logger().error("[{}] Cycle detected in container: {}", __FUNCTION__, subContainer->getName());
+				}
+			} else {
+				g_logger().error("[{}] Maximum iteration depth reached", __FUNCTION__);
+			}
+		}
+	}
+
+	++top.index;
+}
+
+std::shared_ptr<Item> ContainerIterator::operator*() const {
+	if (states.empty()) {
+		return nullptr;
+	}
+
+	auto &top = states.top();
+	if (top.index < top.container->itemlist.size()) {
+		return top.container->itemlist[top.index];
+	}
+	return nullptr;
+}
diff --git a/src/items/containers/container.hpp b/src/items/containers/container.hpp
@@ -21,16 +21,26 @@ class Reward;
 
 class ContainerIterator {
 public:
-	bool hasNext() const {
-		return !over.empty();
-	}
+	ContainerIterator(std::shared_ptr<Container> container, size_t maxDepth = 1000);
+	bool hasNext() const;
 
 	void advance();
-	std::shared_ptr<Item> operator*();
+	std::shared_ptr<Item> operator*() const;
 
 private:
-	std::list<std::shared_ptr<Container>> over;
-	ItemDeque::const_iterator cur;
+	struct IteratorState {
+		std::shared_ptr<Container> container;
+		size_t index;
+		size_t depth;
+
+		IteratorState(std::shared_ptr<Container> c, size_t i, size_t d) :
+			container(c), index(i), depth(d) { }
+	};
+
+	mutable std::stack<IteratorState> states;
+	mutable std::unordered_set<std::shared_ptr<Container>> visitedContainers;
+
+	size_t maxTraversalDepth = 0;
 
 	friend class Container;
 };

diff --git a/src/items/decay/decay.cpp b/src/items/decay/decay.cpp
@@ -66,6 +66,9 @@ void Decay::startDecay(std::shared_ptr<Item> item) {
 }
 
 void Decay::stopDecay(std::shared_ptr<Item> item) {
+	if (!item) {
+		return;
+	}
 	if (item->hasAttribute(ItemAttribute_t::DECAYSTATE)) {
 		auto timestamp = item->getAttribute<int64_t>(ItemAttribute_t::DURATION_TIMESTAMP);
 		if (item->hasAttribute(ItemAttribute_t::DURATION_TIMESTAMP)) {

diff --git a/src/items/item.hpp b/src/items/item.hpp
@@ -597,7 +597,11 @@ class Item : virtual public Thing, public ItemProperties, public SharedObject {
 		count = n;
 	}
 
-	static uint32_t countByType(std::shared_ptr<Item> item, int32_t subType) {
+	static uint32_t countByType(const std::shared_ptr<Item> &item, int32_t subType) {
+		if (!item) {
+			return 0;
+		}
+
 		if (subType == -1 || subType == item->getSubType()) {
 			return item->getItemCount();
 		}