bugfix(object): Avoid potential crash due to dangling contain module when Troop Crawler is destroyed before reaching Reinforcement Pad

[Caball009]

• Follow up for bugfix(object): Avoid crash with dangling contain module in Object::onDestroy() when Reinforcement Pad is destroyed before Troop Crawler drop #2747
• Follow up for bugfix(contain): Restore retail compatibility after changes to Object::m_containedByID #2868
• Fixes issue Destroyed Troop Crawler from reinforcement pad may crash the game due to use-after-free bug #2467

The purpose of the two pull requests was to avoid a crash while retaining retail compatibility, but the fix is rather convoluted. Additionally, this is the second time I came across one or more replays that mismatch because of those changes. So the purpose of this PR is to revert the previous two and fix the crash with a different implementation.

Here's a replay that mismatches with the current implementation. It mismatches because it relied on use-after-free bugs, which was intentionally prevented to avoid potential crashes:
18-11-57_2v4_0_24_HardAI_HardAI_HardAI_HardAI.zip

If a troop crawler that's en route (by plane) to the reinforcement pad gets destroyed, the occupants are left in a state of limbo (see issue). They hold on to the pointer of the destroyed troop crawler, which leads to use-after-free bugs. When the occupants' objects eventually get destroyed (e.g. when a player surrenders or the game ends), the use-after-free bug may crash the game when OpenContain::m_containList is accessed after its destruction.

---

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Line 437 in a40cdb6

ContainedItemsList::iterator it = std::find(m_containList.begin(), m_containList.end(), rider);

Here's a minimal crash reproduction that shows why the crash happens during the call to std::find:
https://godbolt.org/z/znjGGrTvG
https://godbolt.org/z/MzYz6eeMb

See commits for cleaner diff.

TODO:

• Replicate to Generals.
• Combine commits, change commit names and add pull request link.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Prevent crash from dangling containment modules during destruction

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Prevents destruction crashes when occupants retain a destroyed reinforcement transport container.
• Guards retail-compatible containment removal before traversing a destructed occupant list.
• Reverts container-ID tracking changes that caused replay mismatches.

Diagram

graph TD
  A["Container destroyed"] -->|calls| B["OpenContain destructor"] -->|retail mode| C["Drain occupant list"] -->|later| D["Occupant destroyed"] -->|calls removal| E{"List empty?"}
  E -->|Yes| F["Skip list search"]
  E -->|No| G["Remove occupant"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Clear occupant links eagerly

• ➕ Eliminates dangling container pointers at their source.
• ➕ Provides conventional and safer object-lifetime semantics.
• ➖ Changes simulation behavior and can mismatch retail-compatible replays.
• ➖ Requires reliable enumeration and cleanup of every affected occupant.

2. Validate containers by ObjectID

• ➕ Avoids dereferencing a container after it leaves the object registry.
• ➕ Makes pointer validity explicit during occupant destruction.
• ➖ The prior implementation was complex and caused replay mismatches.
• ➖ Requires synchronized pointer and ID handling across containment implementations.

3. Retain containment modules longer

• ➕ Keeps module storage valid until dependent occupants are cleaned up.
• ➕ Avoids relying on residual list state after destruction.
• ➖ Introduces deferred destruction and broader ownership changes.
• ➖ Carries substantially greater compatibility and lifecycle risk.

Recommendation: Use the PR's narrowly scoped guard for retail-compatible Zero Hour builds. Eager link cleanup would be structurally safer, but the reverted ObjectID strategy demonstrates that changing containment state can break replay compatibility; the explicit list drain and early empty check preserve existing simulation behavior with less disruption.

Files changed (6) +41 / -163

Bug fix (3) +37 / -119

Object.cppRevert container validity tracking in Object lifecycle +9/-65

Revert container validity tracking in Object lifecycle

• Removes runtime container-ID bookkeeping and restores direct containment cleanup during destruction. Save/load processing now uses the explicitly transfer-only container ID.

Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp

OpenContain.cppGuard removal through a destructed contain list +18/-0

Guard removal through a destructed contain list

• Explicitly drains the containment list during retail-compatible destruction so its empty state remains observable. Removal now returns before calling std::find when a dangling module points to the already-emptied list.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp

Object.cppRestore compatible Object containment lifecycle +10/-54

Restore compatible Object containment lifecycle

• Reverts pointer-validity tracking through a mirrored ObjectID and restores direct containment removal in onDestroy. Serialization retains a dedicated ID solely for save/load pointer reconstruction.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Refactor (3) +4 / -44

Object.hRestore pointer-only containment state +2/-6

Restore pointer-only containment state

• Inlines the containment pointer setter and removes the runtime container-ID synchronization API. Renames the serialized ID field to clarify that it is used only for transfer and load reconstruction.

Generals/Code/GameEngine/Include/GameLogic/Object.h

Object.hSeparate serialized containment ID from runtime state +1/-5

Separate serialized containment ID from runtime state

• Removes the retail-only container-ID setter and renames the field to indicate its exclusive use by transfer serialization.

GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h

HelixContain.cppRevert Helix container-ID synchronization +1/-33

Revert Helix container-ID synchronization

• Removes retail-compatible ID maintenance for portable structures and restores the original pointer-oriented containment behavior. Non-retail builds continue clearing the containment pointer on removal.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (1) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. onDestroy() dereferences dangling m_containedBy 📎 Requirement gap ⛨ Security

Description

Object::onDestroy() dereferences m_containedBy (via m_containedBy->getContain() and then
removeFromContain()) without any safe-liveness validation, even though contained objects can
retain a dangling m_containedBy after their container is destroyed (e.g., the Troop Crawler
reinforcement-pad destruction scenario). Because the use-after-free can occur before any
OpenContain-level guard executes, the original headless-mode crash path remains and the ticket’s
requirement to remove the underlying lifetime/ownership issue is not met.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[R771-774]

+	if( m_containedBy && m_containedBy->getContain() )
{
-#if RETAIL_COMPATIBLE_CRC
-		if (m_containedByID == INVALID_ID)
-		{
-			// TheSuperHackers @bugfix Caball009 25/05/2026 Due to a potential use-after-free bug that cannot be fixed
-			// with retail compatibility, the 'contained by' pointer of this object may point to an already destroyed object.
-			// Avoid removing this object from the contain list, because it could crash the game,
-			// as the begin / end iterator for STLPort and MSVC std::list implementations depends on dynamically allocated memory.
-			DEBUG_CRASH(("container object must be valid; this looks like use-after-free"));
-		}
-		else
-		{
-			DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy,
-				("contained by pointer is out of sync with contained by ID"));
-
-			if (ContainModuleInterface* contain = m_containedBy->getContain())
-			{
-				contain->removeFromContain(this);
-			}
-		}
-#else
-		if (ContainModuleInterface* contain = m_containedBy->getContain())
-		{
-			contain->removeFromContain(this);
-		}
-#endif
+		m_containedBy->getContain()->removeFromContain( this );
}

Evidence

The compliance requirement (PR Compliance ID 1 / issue #2467) is to eliminate the use-after-free in
the Troop Crawler reinforcement-pad destruction path by removing the underlying lifetime/ownership
problem. However, the updated Object::onDestroy() unconditionally dereferences m_containedBy (so
long as it is non-null) to call m_containedBy->getContain() and proceed to containment cleanup;
the codebase explicitly documents that under RETAIL_COMPATIBLE_CRC the contained-by pointer may
already be dangling, making this dereference itself a potential UAF and crash. Since the crash can
happen at the m_containedBy->getContain() dereference, any added early-return/guard inside
OpenContain::removeFromContain() cannot prevent the UAF because execution may never reach that
point.

Fix use-after-free crash when a reinforced Troop Crawler is destroyed (headless mode)
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[767-774]
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[680-687]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[767-775]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp[413-447]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Object::onDestroy()` dereferences `m_containedBy` (`m_containedBy->getContain()` and subsequent containment cleanup) even though `m_containedBy` can be a dangling pointer when the container was already destroyed (notably in the Troop Crawler reinforcement-pad destruction path, including headless replay runs). This preserves a use-after-free crash risk and fails the compliance requirement to remove the underlying lifetime/ownership issue rather than merely adding late guards.
## Issue Context
- The compliance requirement (PR Compliance ID 1 / issue #2467) expects the fix to fully eliminate the UAF in the Troop Crawler reinforcement-pad destruction scenario.
- The codebase explicitly documents that, under `RETAIL_COMPATIBLE_CRC`, `getContainedBy()` may point to an already-destroyed object, so `m_containedBy` cannot be safely dereferenced just because it is non-null.
- Any guard/early-return inside `OpenContain::removeFromContain()` is insufficient because the UAF can occur earlier at `m_containedBy->getContain()` within `Object::onDestroy()`.
## Fix Focus Areas
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[767-775]
- Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[680-687]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Portable pointer not cleared 🐞 Bug ≡ Correctness

Description

HelixContain::removeFromContain() no longer clears the portable structure's faked contained-by
pointer in RETAIL_COMPATIBLE_CRC builds, leaving a stale container pointer after removal. This can
corrupt containment state and later feed the Object::onDestroy() containment cleanup path with a
dangling pointer.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[R294-296]

+#if !RETAIL_COMPATIBLE_CRC
 portable->friend_setContainedBy(nullptr);
#endif

Evidence

HelixContain explicitly sets the portable structure's contained-by pointer when adding it, but the
retail-compatible removal path no longer clears it and does not invoke the normal removal logic, so
the portable structure can remain pointing at the helix after removal.

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[245-260]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[286-305]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
For the portable-structure special case, `HelixContain` intentionally calls `portable->friend_setContainedBy(getObject())` to fake containment, but on removal it now only clears that pointer in non-retail-compatible builds. In `RETAIL_COMPATIBLE_CRC`, the portable structure keeps a stale `m_containedBy`.
## Issue Context
This code path bypasses `TransportContain::removeFromContain()` and therefore does not call `Object::onRemovedFrom()`, so there is no other cleanup of `m_containedBy` for portable structures.
## Fix Focus Areas
- Always clear the portable structure's contained-by pointer in this removal path (or call the proper removal/onRemovedFrom flow) while preserving any CRC-compat constraints through safer means (e.g., serialize IDs for CRC instead of relying on stale pointers).
### Code pointers
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp[245-305]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[767-775]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Destroyed-container assertion removed ✓ Resolved 🐞 Bug ◔ Observability

Description

Generals Object::onContainedBy() now records a destroyed container without emitting the debug
assertion that previously identified this invalid containment state. This hides the stale-pointer
condition at its source and allows debugging builds to encounter less-local containment failures
later.

Code

Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[L637-638]

-	DEBUG_ASSERTCRASH(containedBy == nullptr || !containedBy->isDestroyed(),
-		("Object::onContainedBy - Adding into a destroyed container"));

Evidence

The Generals implementation now assigns m_containedBy and returns without validating its lifetime,
while the sibling GeneralsMD implementation explicitly checks !containedBy->isDestroyed().
DEBUG_ASSERTCRASH invokes DebugCrash when its condition fails in diagnostic builds, proving that
deleting the assertion removes concrete detection of this invalid state.

Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[615-624]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[684-695]
Core/GameEngine/Include/Common/Debug.h[186-206]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Object::onContainedBy()` in Generals lost its assertion against adding an object to an already destroyed container, removing an important diagnostic for the stale-pointer state involved in containment lifetime failures.
## Issue Context
The corresponding GeneralsMD implementation retains this check. Restore an unconditional `DEBUG_ASSERTCRASH` in the Generals implementation after assigning the containment state, without reintroducing the removed ID bookkeeping.
## Fix Focus Areas
- Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp[615-624]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[684-695]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. CRC contains stale containedBy 🐞 Bug ≡ Correctness

Description

Object::xfer() refreshes m_xferContainedByID only for XFER_SAVE, but Object::xfer is also invoked
under XFER_CRC, so the CRC snapshot can serialize a stale contained-by ID after normal containment
changes. This weakens desync detection and can cause replay/network CRC comparisons to be
inconsistent with the actual containment state.

Code

GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[R4262-4265]

if( xfer->getXferMode() == XFER_SAVE )
{
if( m_containedBy != nullptr )
-				m_containedByID = m_containedBy->getID();
+				m_xferContainedByID = m_containedBy->getID();

Evidence

GameLogic CRC generation uses XFER_CRC and snapshots every object via xferSnapshot; Object::xfer
always xferObjectID’s m_xferContainedByID but only recomputes it for XFER_SAVE, and containment
callbacks do not update it, so CRC can serialize a value that is stale relative to m_containedBy.

Core/GameEngine/Include/Common/Xfer.h[60-69]
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp[4141-4195]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[684-712]
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[4252-4272]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`m_xferContainedByID` is only recomputed when `xfer->getXferMode() == XFER_SAVE`, but the same field is still xfer'd during `XFER_CRC`. Since containment changes (`onContainedBy`/`onRemovedFrom`) do not update `m_xferContainedByID`, CRC snapshots can use stale IDs.
## Issue Context
`GameLogic::getCRC()` creates an `XferCRC` and calls `xferSnapshot(obj)` for every object, which drives `Object::xfer()` in `XFER_CRC` mode.
## Fix Focus Areas
- Recompute the contained-by ObjectID for both `XFER_SAVE` and `XFER_CRC` (and any other non-load modes), e.g.:
- use a local `ObjectID containedByID = m_containedBy ? m_containedBy->getID() : INVALID_ID; xfer->xferObjectID(&containedByID);`
- or update `m_xferContainedByID` whenever `m_containedBy` changes.
### Code pointers
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[4252-4272]
- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp[684-712]
- GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp[4141-4195]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[qodo-free-for-open-source-projects]

Code review by qodo was updated up to the latest commit 831ff73

[xezon]

It would be nice to merge this with 2 commits: 1 for revert and 1 for fixes

[Caball009]

It would be nice to merge this with 2 commits: 1 for revert and 1 for fixes

I think that would be confusing during the review process. I can do that at the end, though.

[Caball009]

It would be nice to merge this with 2 commits: 1 for revert and 1 for fixes

Do you have a suggestion for the first commit name? I intend to use the PR title as second commit name.

I assume both commit names should have the pull request link included.

[xezon]

Maybe "Revert previous fix for crash with dangling contain module in Object::onDestroy() when Reinforcement Pad is destroyed before Troop Crawler drop"

Is quite a bit long :)

[greptile-apps]

Greptile Summary

This PR reverts the live contained-by ID workaround and instead clears OpenContain's list during destruction so later cleanup can avoid traversing destroyed list internals.

• Separates the contained-by transfer ID from live containment state in both game variants.
• Adds retail-compatible empty-list checks to OpenContain removal.
• Reverts the corresponding HelixContain ID bookkeeping in Zero Hour.

Confidence Score: 4/5

The dangling-container dereference in Object::onDestroy needs to be removed before merging because the reported cleanup path can still access a freed Object.

The new list check cannot protect the first getContain() call, which reads through the same dangling container pointer the change is intended to tolerate.

Files Needing Attention: Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp and GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Removes the retail dangling-pointer validity guard and exposes a freed container dereference during occupant destruction.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Mirrors the same unsafe Object::onDestroy containment lookup in Zero Hour.
Generals/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Clears the list and checks empty state after destruction, but this guard is reached only after the unsafe container lookup.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Applies the same post-destruction list workaround to Zero Hour.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/HelixContain.cpp	Reverts retail-compatible contained-by ID bookkeeping for portable structures without an independently established new defect.
Generals/Code/GameEngine/Include/GameLogic/Object.h	Renames the serialized containment ID field to clarify its transfer-only role.
GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h	Mirrors the transfer-only containment ID rename for Zero Hour.

Sequence Diagram

sequenceDiagram
    participant C as Troop Crawler
    participant O as Occupant
    participant G as GameLogic
    C->>G: Destroy en route
    G->>C: Delete Object and OpenContain
    Note over O: m_containedBy still references C
    G->>O: Destroy during later cleanup
    O->>C: "m_containedBy->getContain()"
    Note over C,O: Freed Object is accessed before empty-list guard

Loading

Prompt To Fix All With AI

### Issue 1
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp:696
**Dangling container dereferenced first**

When an occupant retains `m_containedBy` after its en-route Troop Crawler is deleted, this condition calls `getContain()` through the freed object before reaching the new empty-list check, causing the same use-after-free crash during occupant destruction.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

_{Reviews (1): Last reviewed commit: "bugfix(object): Avoid potential crash wh..." | Re-trigger Greptile}

[Caball009]

I made the commit titles a bit shorter.

• First commit reverses PRs bugfix(object): Avoid crash with dangling contain module in Object::onDestroy() when Reinforcement Pad is destroyed before Troop Crawler drop #2747 and bugfix(contain): Restore retail compatibility after changes to Object::m_containedByID #2868.
• Second commit reintroduces some minor changes from the other two PRs and adds the OpenContain.cpp changes.

Just for the record: the issue has nothing to do with the Reinforcement Pad, just that the (transport plane with the) Troop Crawler gets destroyed while en route to the pad.

Ready for rebase and merge.