bugfix(contain): Restore retail compatibility after crash fix in OpenContain::processDamageToContained

[Caball009]

• Closes Restore retail compatibility after crash fix in OpenContain::processDamageToContained #2223
• Follow up for [GEN][ZH] Fix crash in OpenContain::processDamageToContained() when killed occupants kill the host container #1019

The way the crash fix was implemented in #1019 assumes the container size is either unchanged or 0. This is not the case in the attached replay in #2223.

Gameplay without this PR:

processDamageToContained_before2.mp4

Gameplay with this PR:

processDamageToContained_after2.mp4

Due to a different bug, a GLA Worker enters the unmanned Emperor Overlord but becomes part of the crew instead of the pilot. When the Emperor gets destroyed it first removes its Gattling turret and so the container size has changed. The Worker is still inside, though, so breaking out of the loop is not allowed.

TODO:

• Replicate in Generals.

[greptile-apps]

Greptile Summary

This PR is a follow-up bugfix to PR #1019, targeting the RETAIL_COMPATIBLE_CRC iteration path inside OpenContain::processDamageToContained. The previous fix stopped looping (via DEBUG_ASSERTCRASH + break) whenever the container list shrank mid-iteration, which was correct for the Battle Bus scenario (all occupants flushed at once → size drops to 0) but incorrectly skipped damage delivery when only some occupants were removed — as happens when an Emperor Overlord's Gattling turret is ejected before the loop finishes.

Key changes:

• Removes the DEBUG_ASSERTCRASH(listSize == 0, …) assertion that was firing in the new edge case.
• Adds a continue when items->size() != 0, so remaining occupants are still damaged; only breaks when the list is fully drained to zero.

Notable concern:

• After *it++, it already points to the next (not-yet-processed) element. If the list modification that shrinks the size removes the element it now points to, calling it != items->end() on the continue path is undefined behaviour for std::list. The fix is safe for the described scenario (turret is removed while it has already advanced past it), but the safety relies on an implicit ordering assumption not enforced or documented in the code.

Confidence Score: 3/5

• The fix resolves the documented regression but carries an implicit iterator-ordering assumption that could cause undefined behaviour in untested orderings.
• The change is small and well-motivated, and works correctly for the described Emperor Overlord + Worker scenario. However, because it is advanced via *it++ before damage is applied, continuing the loop after a list-size change risks dereferencing an invalidated iterator if the removed element is the one it currently references. This is not a guaranteed crash but is a latent undefined-behaviour path that warrants a comment or an additional safety check.
• GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp — specifically the continue path on line 1506.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Replaces unconditional break (via removed DEBUG_ASSERTCRASH) with a continue when the container list shrinks but is not yet empty, restoring damage delivery to remaining occupants. The fix is correct for the documented scenario but carries a latent iterator-invalidation risk if the removed element happens to be the one currently pointed to by it.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["processDamageToContained(percentDamage)"] --> B{items != nullptr?}
    B -- No --> Z[Return]
    B -- Yes --> C["Capture listSize = items->size()\nit = items->begin()"]
    C --> D{it != items->end?}
    D -- No --> Z
    D -- Yes --> E["object = *it++\n(advance iterator)"]
    E --> F["Apply damage to object\nattemptDamage / kill"]
    F --> G{listSize != items->size?}
    G -- No --> D
    G -- Yes --> H{items->size != 0?}
    H -- Yes --> I["continue\n(re-check loop condition)\n⚠️ it may point to removed node"]
    H -- No --> J[break]
    I --> D

Loading

_{Last reviewed commit: b533974}

[greptile-apps]

Potential iterator invalidation on continue

ContainedItemsList is a std::list<Object*>, so erasing a node invalidates only the iterator pointing to that node — all other iterators remain valid.

After *it++, it already refers to the next element to be processed. If the list modification that caused listSize != items->size() removed the element that it is now pointing to (e.g. the unprocessed occupant is the one ejected by the host's death handler), then calling it != items->end() on the continue path invokes undefined behaviour.

The fix is safe in the specific scenario described in the PR description (turret is removed first in the list, so it has already advanced past it to the Worker). However, if the ordering were reversed — Worker first, turret second — and the turret's removal is triggered while the Worker is being processed, it would point to the now-invalidated turret node and the continue would UB.

It may be worth adding a guard to re-verify that the element it refers to is still in the list, or adding an explanatory comment noting the assumed ordering invariant that makes this safe.

Prompt To Fix With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
Line: 1505-1506

Comment:
**Potential iterator invalidation on `continue`**

`ContainedItemsList` is a `std::list<Object*>`, so erasing a node invalidates **only the iterator pointing to that node** — all other iterators remain valid.

After `*it++`, `it` already refers to the *next* element to be processed. If the list modification that caused `listSize != items->size()` removed the element that `it` is now pointing to (e.g. the unprocessed occupant is the one ejected by the host's death handler), then calling `it != items->end()` on the `continue` path invokes undefined behaviour.

The fix is safe in the specific scenario described in the PR description (turret is removed first in the list, so `it` has already advanced past it to the Worker). However, if the ordering were reversed — Worker first, turret second — and the turret's removal is triggered while the Worker is being processed, `it` would point to the now-invalidated turret node and the `continue` would UB.

It may be worth adding a guard to re-verify that the element `it` refers to is still in the list, or adding an explanatory comment noting the assumed ordering invariant that makes this safe.

How can I resolve this? If you propose a fix, please make it concise.

[Caball009]

Code

void fn01()
{
	std::list<int> lst{ 1, 2, 3, 4 };

	for (auto it1 = lst.begin(); it1 != lst.end(); ) 
	{
		const auto val = *it1++;

		if (val == 2)
		{
			// imitate 'object->attemptDamage'
			lst.clear(); // invalidates it1
		}

		if (lst.empty())
			break;
	}
}

void fn02()
{
	std::list<int> lst{ 1, 2 };

	for (auto it1 = lst.begin(); it1 != lst.end(); ) 
	{
		const auto val = *it1++;

		if (val == 1)
		{
			// imitate 'object->attemptDamage'
			const auto it2 = std::find(lst.begin(), lst.end(), 1);
			if (it2 != lst.end())
				lst.erase(it2);
		}

		if (lst.empty())
			break;
	}
}

void fn03()
{
	std::list<int> lst{ 1, 2 };

	for (auto it1 = lst.begin(); it1 != lst.end(); )
	{
		const auto val = *it1++;

		if (val == 2)
		{
			// imitate 'object->attemptDamage'
			const auto it2 = std::find(lst.begin(), lst.end(), 1);
			if (it2 != lst.end())
				lst.erase(it2);
		}

		if (lst.empty())
			break;
	}
}

void fn04()
{
	std::list<int> lst{ 1, 2 };

	for (auto it1 = lst.begin(); it1 != lst.end(); )
	{
		const auto val = *it1++;

		if (val == 1)
		{
			// imitate 'object->attemptDamage'
			const auto it2 = std::find(lst.begin(), lst.end(), 2);
			if (it2 != lst.end())
				lst.erase(it2); // invalidates it1
		}

		if (lst.empty())
			break;
	}
}

int main()
{
	fn01(); // battlebus + terrorists -> safe
	fn02(); // emperor + worker       -> safe
	fn03(); // ?                      -> safe
	fn04(); // ?                      -> unsafe
}

This is my understanding of both bugs. If object->attemptDamage can arbitrarily remove items from the container, the current implementation is unsafe (see fn04).

We may have to do this instead (assumes all items are unique). This may still fail if multiple but not all items are removed from the list (e.g. if the current and next object have been removed from the list):

if (items->empty())
		break;

// find current iterator to get the next valid iterator
ContainedItemsList::const_iterator newIt = std::find(items->begin(), items->end(), object);
if (newIt != items->end())
		it = ++newIt;