fix: crash when removing items during iteration with ContainerIterator

[dudantas]

Description

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.

Behaviour

Actual

Removing items from a container during iteration causes a crash due to invalidated iterators.
Example: During an event or action that removes items while iterating, the server crashes.

Expected

Items can be safely removed from a container during iteration without causing a crash.
The iteration continues correctly, and the application remains stable.

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested

We conducted tests to verify that the crash no longer occurs when items are removed during iteration:

Test A: Used a command (!container) that iterates over a player's backpack and removes items during iteration. Confirmed that no crash occurs and all items are removed safely.

void Game::playerSay(uint32_t playerId, uint16_t channelId, SpeakClasses type, const std::string &receiver, const std::string &text) {
	std::shared_ptr<Player> player = getPlayerByID(playerId);
	if (!player) {
		return;
	}

	if (text == "!container") {
		auto item = player->getInventoryItem(CONST_SLOT_BACKPACK);
		auto container = item ? item->getContainer(): nullptr;
		if (!container) {
			player->sendCancelMessage("No container.");
			return;
		}

		for (ContainerIterator it = container->iterator(); it.hasNext(); it.advance()) {
			std::shared_ptr<Item> item = *it;
			if (item) {
				container->removeItem(item);
			}
		}

		player->sendTextMessage(MESSAGE_EVENT_ADVANCE, "Talkaction called");
		return;
	}
}

Test the command before this implementation, it will crash. This can happen in various contexts, for example, during the reading of the "itemlist" in a loop, and the player removes an item from the container being read, it will cause a crash because it invalidates all iterators.

After implementing the fix, the crash will no longer occur with the command above, and it likely won't happen in other scenarios either.

NOTE: In the code, the "container->removeItem(item)" function will disrupt the iterator's count, but it will neither crash nor cause any errors. This happens because the iterator becomes unreliable when the container is modified during iteration. However, it effectively prevents a crash, which is our primary objective. If you want to test only the amount of items scanned, comment out the "removeItem.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I checked the PR checks reports
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works

Fixes this crash:
crash-container-iterator.log

[github-actions]

Qodana for C/C++

1007 new problems were found

Inspection name	Severity	Problems
misra-cpp2008-5-0-11	🔶 Warning	890
err33-c	🔶 Warning	37
static-accessed-through-instance	🔶 Warning	16
misra-cpp2008-5-0-5	🔶 Warning	13
misra-cpp2008-5-2-12	🔶 Warning	5
function-cognitive-complexity	🔶 Warning	5
narrowing-conversions	🔶 Warning	3
unnecessary-value-param	🔶 Warning	3
misra-cpp2008-0-1-7	🔶 Warning	3
implicit-bool-conversion	🔶 Warning	3
use-starts-ends-with	🔶 Warning	3
magic-numbers	🔶 Warning	3
misra-cpp2008-5-3-1	🔶 Warning	2
misra-cpp2008-5-0-13	🔶 Warning	2
unnecessary-copy-initialization	🔶 Warning	2
for-range-copy	🔶 Warning	2
id-dependent-backward-branch	🔶 Warning	2
duplicate-include	🔶 Warning	2
simplify-boolean-expr	🔶 Warning	2
misra-cpp2008-6-4-5	🔶 Warning	1
misra-cpp2008-4-5-2	🔶 Warning	1
no-automatic-move	🔶 Warning	1
unroll-loops	🔶 Warning	1
too-small-loop-variable	🔶 Warning	1
convert-member-functions-to-static	🔶 Warning	1
pass-by-value	🔶 Warning	1
inconsistent-declaration-parameter-name	🔶 Warning	1
redundant-casting	🔶 Warning	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

• Register at Qodana Cloud and configure the action
• Use GitHub Code Scanning with Qodana
• Host Qodana report at GitHub Pages
• Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2024.1.11
        with:
          upload-result: true

Contact Qodana team

Contact us at qodana-support@jetbrains.com

• Or via our issue tracker: https://jb.gg/qodana-issue
• Or share your feedback: https://jb.gg/qodana-discussions

[gesior]

I've installed this on 100+ server that goes into infinite ContainerIterator::advance loop few times a week.
I will post, if crashes are fixed in few days.

[dudantas]

I've installed this on 100+ server that goes into infinite ContainerIterator::advance loop few times a week. I will post, if crashes are fixed in few days.

There is a server with 100+ users testing for over 5 days, and so far no issues.

[phacUFPE]

Tested and working.