bugfix(contain): Prevent objects from being added to destroyed container object

[Caball009]

• Closes China Assault Troop Crawler can become a firing ghost unit after Reinforcement Pad drop #2741
• Related PR bugfix(contain): Add workaround for use-after-free bug that may crash the game #2747

When a reinforcement plane with an Infantry General Troop Crawler is destroyed en route, the plane and vehicle are destroyed, but the infantry spawned and remains in an invalid state (see issue for more details).

Beside being a gameplay and logic bug, there's also another issue because the infantry objects will hold on the container pointer even though that object has been destroyed. This can lead to use-after-free bugs and a crash (see related PR).

This PR makes 4 changes:

1. Objects that are spawned in such a state are now visible.
2. Added code to prevent the creation of the transport payload (e.g. the Mini-Gunners) if the container object is destroyed (non-retail only).
3. Added an early return to OpenContain::addToContain to prevent units from being added to a destroyed object (non-retail only).
4. Added debug assertions for dead container objects or occupants.

TODO:

• Replicate in Generals.

[greptile-apps]

Greptile Summary

This PR fixes a bug where a reinforcement plane destroyed in flight could still spawn its infantry payload into the now-destroyed container, leaving units in an invisible, invalid state and exposing a use-after-free via a dangling container pointer.

• Non-retail builds: TransportContain::update now skips createPayload() when the container is destroyed, and OpenContain::addToContain gains a matching early-return guard; both paths emit DEBUG_CRASH to surface violations during development.
• Retail builds: Because early returns would break CRC compatibility, the mitigation is softer — addOrRemoveObjFromWorld no longer hides occupants when it detects the owning container is effectively dead or destroyed, so stranded infantry remain visible rather than vanishing.

Confidence Score: 5/5

Safe to merge. Both changed files apply narrow, well-guarded fixes that do not alter retail CRC behaviour.

All new code paths are gated behind RETAIL_COMPATIBLE_CRC or !RETAIL_COMPATIBLE_CRC so that retail-build behaviour is only touched by the visibility fix in addOrRemoveObjFromWorld, which is a defensive no-op when the container is alive. The non-retail guards prevent the invalid state rather than working around it, and the DEBUG_CRASH additions are inert in retail builds. No logic errors, data-loss paths, or memory-safety regressions are introduced.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/OpenContain.cpp	Two guarded changes: (1) in addOrRemoveObjFromWorld, prevents hiding occupants when the container is dead/destroyed (retail-only fix); (2) in addToContain, adds an early return plus DEBUG_CRASH when the container is already destroyed (non-retail only) and a DEBUG_CRASH to the pre-existing rider-is-destroyed guard.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TransportContain.cpp	In TransportContain::update, skips createPayload() when the owning object is already destroyed (non-retail only), preventing infantry from being spawned into an invalid/freed container. Retail path is unchanged for CRC compatibility.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[TransportContain::update called] --> B{m_payloadCreated == FALSE?}
    B -- No --> Z[Continue normal update]
    B -- Yes --> C{RETAIL_COMPATIBLE_CRC?}
    C -- Yes/Retail --> D[createPayload]
    C -- No/Non-Retail --> E{getObject isDestroyed?}
    E -- Yes --> F[Skip payload creation]
    E -- No --> D
    D --> G[OpenContain::addToContain for each payload unit]
    G --> H{RETAIL_COMPATIBLE_CRC?}
    H -- No/Non-Retail --> I{getObject isDestroyed?}
    I -- Yes --> J[DEBUG_CRASH + early return]
    I -- No --> K{rider isDestroyed?}
    H -- Yes/Retail --> K
    K -- Yes --> L[DEBUG_CRASH + return]
    K -- No --> M[Add rider to contain list]
    M --> N{isEnclosingContainerFor rider?}
    N -- Yes --> O[addOrRemoveObjFromWorld rider=false]
    O --> P{RETAIL_COMPATIBLE_CRC?}
    P -- Yes/Retail --> Q{Container isEffectivelyDead or isDestroyed?}
    Q -- Yes --> R[Skip setDrawableHidden - unit visible]
    Q -- No --> S[setDrawableHidden true]
    P -- No/Non-Retail --> S
    N -- No --> Z

Loading

_{Reviews (6): Last reviewed commit: "Moved call to 'Drawable::setDrawableHidd..." | Re-trigger Greptile}

[xezon]

Is this code path only relevant for RETAIL_COMPATIBLE_CRC ?

[Caball009]

The getObject()->isEffectivelyDead() would still be relevant without retail compatibility.

I'm not sure in what scenario that would get triggered, but that's kind of the point: if there's a similar bug, it becomes more noticeable if the occupant is visible at least.

[xezon]

This looks like it should be behind RTS_DEBUG then. Otherwise we may end up with players complaining about ghost objects on the map if this happened.

Also, addOrRemoveObjFromWorld explicitly sets setDrawableHidden(true). Perhaps move this debug code into addOrRemoveObjFromWorld? Also maybe draw a colorful circle below the affected unit so that it is extra obvious.

[Caball009]

This looks like it should be behind RTS_DEBUG then. Otherwise we may end up with players complaining about ghost objects on the map if this happened.

Quite the opposite imo. The Mini-Gunners should be visible because they'll attack nearby enemy units. It also makes it more likely that issues like this one don't fly under the radar but are easily noticed and reported by players.

Also, addOrRemoveObjFromWorld explicitly sets setDrawableHidden(true). Perhaps move this debug code into addOrRemoveObjFromWorld?

I'll check it out.

Also maybe draw a colorful circle below the affected unit so that it is extra obvious.

I'm not sure how to do that.

[xezon]

Quite the opposite imo. The Mini-Gunners should be visible because they'll attack nearby enemy units. It also makes it more likely that issues like this one don't fly under the radar but are easily noticed and reported by players.

In this case yes, but what about cases where the Ghost Object would be just visual, with no function? Many units can't do anything inside transports. Minigunner can shoot out of some transporters. Having ghost objects drawn to players just to identify an issue that should be for QA testers to find is awful design.

I'm not sure how to do that.

There are functions somewhere to draw circles. Other debugs already use it, such as the bounding box debug on infantry.

[Caball009]

Having ghost objects drawn to players just to identify an issue that should be for QA testers to find is awful design.

It would be if this were regular game development, but it's not. The most likely scenario is that a bug like this flies under the radar for years until someone notices it, grabs the replay and creates an issue for it. That'd still be acceptable if RTS_DEBUG (and debug builds in general) were compatible with builds that regular players use, but it's not. Developers would then be discouraged to check with RTS_DEBUG enabled and the bug would stay hidden.

There are functions somewhere to draw circles. Other debugs already use it, such as the bounding box debug on infantry.

It would have to get drawn for every frame, right? Could perhaps be done in drawablePostDraw, but it would need to check if the contained by pointer and ID mismatch, which would require a get function for containedByID. It feels a bit like too much work imo.

[xezon]

You want to essentially add a permanent debug in Release, to identify potential bugs that may not even exist. This is not a good way to deal with it. If you want to have debugs in Release, then it would still need to go behind a macro that is default enabled in Release. Many developers test with RTS_DEBUG so bugs will be identified with its debugs this way. Players will find obvious bugs, and if they are not obvious, then there is no need to make them obvious for players, because it will add annoyance for the duration they are not fixed.

What would be ok to add is a Release log event, because that will be subtle and not be a detriment for the player experience.

Is there any precedence for debugs activated in Retail game? I cannot think of any.

[xezon]

contained == containee (rider) or container?

[Caball009]

Object* m_containedBy ?

[xezon]

containedBy refers to the container.

[Caball009]

I'm not sure what you're asking then.

[xezon]

Oh ok I did not realize "contained by" refers to "m_containedBy".

[xezon]

This looks like it should be behind RTS_DEBUG then. Otherwise we may end up with players complaining about ghost objects on the map if this happened.

Also, addOrRemoveObjFromWorld explicitly sets setDrawableHidden(true). Perhaps move this debug code into addOrRemoveObjFromWorld? Also maybe draw a colorful circle below the affected unit so that it is extra obvious.