bugfix(contain): Add workaround for use-after-free bug that may crash the game

[Caball009]

• Closes Destroyed Troop Crawler from reinforcement pad may crash the game due to use-after-free bug #2467
• Related PR bugfix(contain): Prevent objects from being added to destroyed container object #2746

This PR provides a retail specific workaround for use-after-free bug that could crash the game under very specific conditions (see issue and related PR).

The actual crash would happen when e.g. OpenContain::m_containList is accessed after it's been destructed. STLPort and MSVC implementations for std::list rely on dynamically allocated memory, so accessing the begin / end iterator after the destruction of the list is not just undefined behavior, it crashes the game. See minimal crash reproduction: https://godbolt.org/z/T9sEb4MsK

TODO:

• Verify that resetting the contain pointer is not an alternative (not retail compatible).
• Replicate in Generals.

[greptile-apps]

Greptile Summary

This PR renames m_xferContainedByID to m_containedByID and expands its role: under RETAIL_COMPATIBLE_CRC, the field is kept continuously in sync with the container's liveness so that onDestroy can safely detect whether m_containedBy has become a dangling pointer, without dereferencing it.

• onContainedBy now sets m_containedByID to the container's ID (or INVALID_ID if the container is already destroyed) under RETAIL_COMPATIBLE_CRC, establishing the key invariant the workaround relies on.
• onRemovedFrom clears m_containedByID to INVALID_ID, keeping the field in sync.
• onDestroy checks m_containedByID == INVALID_ID as a signal that m_containedBy is dangling; if so, removeFromContain is skipped to prevent accessing a freed std::list; in the safe path, findObjectByID confirms the container is alive before calling removeFromContain.
• xfer save path is now guarded with !RETAIL_COMPATIBLE_CRC so it does not overwrite the already-maintained m_containedByID with a stale re-derivation (a previously flagged issue, now fixed).

Confidence Score: 5/5

The change is safe to merge; it introduces a well-scoped invariant (INVALID_ID equals dangling pointer) that eliminates the crash-prone removeFromContain call without touching any other code paths.

All four modification sites cooperate correctly to maintain the new invariant. The previously flagged xfer-save re-derivation hole is now properly guarded. The workaround is limited to RETAIL_COMPATIBLE_CRC builds, leaving the non-retail path unchanged.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Core implementation of the workaround; onContainedBy, onRemovedFrom, onDestroy, and xfer all updated correctly; previously flagged xfer-save re-derivation issue is now guarded.
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Mirrors the GeneralsMD changes; all four modified functions are consistent with the Zero Hour copy.
GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h	Member renamed from m_xferContainedByID to m_containedByID with updated documentation comment reflecting its broadened purpose.
Generals/Code/GameEngine/Include/GameLogic/Object.h	Same header rename as GeneralsMD; no logic changes.

Sequence Diagram

sequenceDiagram
    participant A as Object A (Container)
    participant B as Object B (Contained)
    participant GL as GameLogic
    participant OC as OpenContain (A's module)

    Note over A,B: Normal containment setup
    A->>B: onContainedBy(A)
    B->>B: "m_containedBy = A, m_containedByID = A.getID()"

    Note over A,OC: Container destruction begins (bug scenario)
    GL->>A: destroyObject(A)
    A->>OC: onDelete() — m_containList already freed!
    OC->>B: "onContainedBy(A) where A.isDestroyed()==true"
    B->>B: "m_containedBy = A (dangling!), m_containedByID = INVALID_ID"

    Note over B,GL: Contained object is also destroyed
    GL->>B: destroyObject(B)
    B->>B: onDestroy()
    B->>B: "m_containedBy != null — enter block"
    alt "m_containedByID == INVALID_ID (container already destroyed)"
        B->>B: DEBUG_CRASH / skip removeFromContain (safe)
    else ID valid (normal path)
        B->>GL: findObjectByID(m_containedByID)
        GL-->>B: returns m_containedBy (confirmed alive)
        B->>OC: removeFromContain(B) — safe
    end

Loading

_{Reviews (7): Last reviewed commit: "Replicated in Generals." | Re-trigger Greptile}

[Caball009]

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.

The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

Line 1119 in 92df977

Object *containedBy = obj->getContainedBy();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

Line 4940 in 92df977

Object *containedBy = source->getContainedBy();

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

Line 1903 in 92df977

const ContainModuleInterface *theirContain = source->getContainedBy()->getContain();

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

Line 656 in 92df977

const Object *containedBy = source->getContainedBy();

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

Line 3227 in 92df977

const Object *containedBy = getContainedBy();

An early return if m_containedByID == INVALID_ID in the following function would be needed:

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

Lines 732 to 742 in 92df977

	const Object* Object::getEnclosingContainedBy() const
	{
	for (const Object* child = this, *container = getContainedBy(); container; child = container, container = container->getContainedBy())
	{
	ContainModuleInterface* containModule = container->getContain();
	if (containModule && containModule->isEnclosingContainerFor(child))
	return container;
	}
	return nullptr;
	}

[xezon]

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.

The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

Is this for a follow up or what do we do with this?

[Caball009]

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.
The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

Is this for a follow up or what do we do with this?

Nothing as far as I'm concerned. It's just an explicit acknowledgement that the 'damage' for this bug is so extensive that I don't see how it can be fixed with retail compatibility, but at least it shouldn't crash.

[xezon]

Needs replication

[Caball009]

Replicated in Generals.