Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Caball009 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,7 +1501,10 @@ void OpenContain::processDamageToContained(Real percentDamage)
// GLA Battle Bus with at least 2 half damaged GLA Terrorists inside.
if (listSize != items->size())
{
DEBUG_ASSERTCRASH( listSize == 0, ("List is expected empty") );
// TheSuperHackers @bugfix Caball009 09/03/2026 Continue loop if not all occupants are removed.
if (items->size() != 0)
continue;
Comment on lines +1505 to +1506
Copy link

@greptile-apps greptile-apps bot Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


break;
}
}
Expand Down
Loading