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 purpose beyond xfer serialization: under RETAIL_COMPATIBLE_CRC, it is now kept continuously in sync by onContainedBy and onRemovedFrom to provide a safe sentinel that guards against the use-after-free crash in onDestroy when a container's m_containList has already been freed.

• Rename + broadened semantics: m_xferContainedByID → m_containedByID; it now doubles as a "is-the-container-still-alive" flag under RETAIL_COMPATIBLE_CRC.
• Use-after-free guard: onDestroy skips removeFromContain (and emits DEBUG_CRASH) when m_containedByID == INVALID_ID while m_containedBy is non-null, indicating a dangling container pointer.
• Xfer save path hardened: The re-derivation of the contained-by ID from the raw pointer is now skipped under RETAIL_COMPATIBLE_CRC (fixing the previously flagged unsafe dereference in the save path).

Confidence Score: 5/5

Safe to merge — the workaround correctly avoids the crashing removeFromContain call when the container's memory has already been freed, and the previously flagged unsafe dereference in the xfer save path is now properly guarded.

Both previously raised concerns are resolved: m_containedByID is kept continuously in sync, and the save path no longer re-derives the ID from a potentially dangling m_containedBy pointer under RETAIL_COMPATIBLE_CRC. The onDestroy sentinel check is logically sound.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h	Simple rename of m_xferContainedByID to m_containedByID with an updated comment clarifying the field's expanded role.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Core of the workaround: onContainedBy and onRemovedFrom now maintain m_containedByID continuously under RETAIL_COMPATIBLE_CRC; onDestroy uses it as a sentinel to skip the unsafe removeFromContain call; the xfer save path is guarded to avoid re-deriving the ID from a potentially dangling pointer.

Sequence Diagram

sequenceDiagram
    participant Container
    participant Contained as ContainedObject
    participant Logic as GameLogic

    Note over Container,Contained: Normal containment flow
    Container->>Contained: onContainedBy(container)
    Contained->>Contained: "m_containedBy = container"
    Contained->>Contained: "m_containedByID = container->getID()"

    Note over Container,Contained: Use-after-free scenario
    Container->>Logic: onDestroy() frees m_containList
    Logic->>Contained: onContainedBy(destroyedContainer)
    Contained->>Contained: "m_containedBy = destroyedContainer dangling"
    Contained->>Contained: "m_containedByID = INVALID_ID sentinel set"

    Note over Contained: ContainedObject also destroyed
    Logic->>Contained: onDestroy()
    alt INVALID_ID - container was freed
        Contained->>Contained: DEBUG_CRASH skip removeFromContain
    else Valid ID
        Contained->>Logic: findObjectByID verify in sync
        Contained->>Container: "contain->removeFromContain(this)"
    end

Loading

_{Reviews (6): Last reviewed commit: "Addressed feedback." | Re-trigger Greptile}

[xezon]

I think this use-after-free be avoided now as well. After all, if m_containedBy is not null then we can expect m_containedBy->getContain() not null too.

[Caball009]

I think this use-after-free be avoided now as well.

Good point, changed.

After all, if m_containedBy is not null then we can expect m_containedBy->getContain() not null too.

That makes sense, but I'd like to stick to the original code that also has this check on m_containedBy->getContain().

[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?