Skip to content

Conversation

@xezon
Copy link

@xezon xezon commented Jun 28, 2025

This change fixes a critical problem with AIGroup, that happens all the time in any match, but only crashes the Replay playback when a player is selected. The memory management of AIGroup is totally broken, and it will leak many AIGroups in AI matches (or matches that use AI scripts) and that will slow down iterating over std::list<AIGroup *> m_groupList.

The Problem

AIGroup has a fundamental memory management flaw with its AIGroup::remove function. It deletes itself when all members are removed, which means that any non-AIGroup members holding a reference to that AIGroup will dangle when it is deleted.

This happens all the time in GameLogic::logicMessageDispatcher with its currentlySelectedGroup variable. It holds objects in a AIGroup, but that AIGroup can be deleted by external events, for example

void GameLogic::logicMessageDispatcher( GameMessage *msg, void *userData )
{
...
  AIGroup* currentlySelectedGroup = NULL;

  if (isInGame())
  {
    if (msg->getType() >= GameMessage::MSG_BEGIN_NETWORK_MESSAGES && msg->getType() <= GameMessage::MSG_END_NETWORK_MESSAGES)
    {
      if (msg->getType() != GameMessage::MSG_LOGIC_CRC && msg->getType() != GameMessage::MSG_SET_REPLAY_CAMERA)
      {
        currentlySelectedGroup = TheAI->createGroup(); // can't do this outside a game - it'll cause sync errors galore.
        CRCGEN_LOG(( "Creating AIGroup %d in GameLogic::logicMessageDispatcher()\n", (currentlySelectedGroup)?currentlySelectedGroup->getID():0 ));
        thisPlayer->getCurrentSelectionAsAIGroup(currentlySelectedGroup);

...
  // process the message
  GameMessage::Type msgType = msg->getType();
  switch( msgType )
  {
...
    case GameMessage::MSG_REMOVE_BEACON:
    {
      AIGroup* allSelectedObjects = TheAI->createGroup();
      thisPlayer->getCurrentSelectionAsAIGroup(allSelectedObjects); // <----- xezon: this will delete the AIGroup pointed to by currentlySelectedGroup

There are multiple code paths like this that can delete currentlySelectedGroup.

The Solution

Properly reference count AIGroup so that an AIGroup is only destroyed when all owners let go off it. This fixes all problems, any potential leaks, crashes. It also makes the code simpler in some places.

The reference counted AIGroup is compiled out with RETAIL_COMPATIBLE_AIGROUP, because it WILL mismatch with Retail, because of all the memory leaks and whatever else happens when touching this code. However, !RETAIL_COMPATIBLE_AIGROUP is compatible with the Golden Replay 1, likely because the humans do not use all the scripts that leak AIGroups.

TODO

  • Test against VC6 Golden Replay
  • Replicate in Generals
  • Test against more replays

@xezon xezon added this to the Stability fixes milestone Jun 28, 2025
@xezon xezon added Bug Something is not working right, typically is user facing Critical Severity: Minor < Major < Critical < Blocker Performance Is a performance concern Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash, very bad NoRetail This fix or change is not applicable with Retail game compatibility labels Jun 28, 2025
@xezon xezon changed the title [GEN][ZH] Fix AIGroup memory management and game crash when a player is selected in Replay playback due heap-use-after-free in GameLogic::logicMessageDispatcher() [GEN][ZH] Fix AIGroup memory management and leaks Jun 28, 2025
Copy link

@Mauller Mauller left a comment

Choose a reason for hiding this comment

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

Horrible hacks but similar to what i had to do with the pathfinding.

@xezon
Copy link
Author

xezon commented Jun 29, 2025

@helmutbuhler Can you test this against a few replays please?

@helmutbuhler
Copy link

@helmutbuhler Can you test this against a few replays please?

I checked with about 500 replays. No mismatches.

@xezon
Copy link
Author

xezon commented Jun 30, 2025

I checked with about 500 replays. No mismatches.

Nice. Thank you.

@xezon xezon force-pushed the xezon/fix-aigroup-6 branch from be5868c to 48302f4 Compare July 1, 2025 16:52
@xezon
Copy link
Author

xezon commented Jul 1, 2025

Replicated in Generals with conflicts.

$ git diff 1ebce079804c879126b540baffccd6ae910ec106..16fc8a9b36454e44722e4c0313eaf7cc54b43a4c > changes.patch

$ git apply -p2 --directory=Generals --reject --whitespace=fix changes.patch
changes.patch:84: trailing whitespace.

changes.patch:90: trailing whitespace.

changes.patch:97: trailing whitespace.

changes.patch:123: trailing whitespace.

changes.patch:264: trailing whitespace.

Checking patch Generals/GameEngine/Include/Common/GameDefines.h...
error: Generals/GameEngine/Include/Common/GameDefines.h: No such file or directory
Checking patch Generals/Code/GameEngine/Include/Common/Player.h...
Hunk #1 succeeded at 608 (offset -26 lines).
Checking patch Generals/Code/GameEngine/Include/GameLogic/AI.h...
Hunk #3 succeeded at 273 (offset -7 lines).
Hunk #4 succeeded at 859 (offset -41 lines).
Hunk #5 succeeded at 952 (offset -42 lines).
Hunk #6 succeeded at 990 (offset -42 lines).
Hunk #7 succeeded at 1005 (offset -42 lines).
Checking patch Generals/Code/GameEngine/Include/GameLogic/Object.h...
error: while searching for:

        GeometryInfo    m_geometryInfo;

        AIGroup*                        m_group;                                                                ///< if non-NULL, we are part of this group of agents

        // These will last for my lifetime.  I will reuse them and reset them.  The truly dynamic ones are in PartitionManager
        SightingInfo            *m_partitionLastLook;                                                           ///< Where and for whom I last looked, so I can undo its effects when I stop

error: patch failed: Generals/Code/GameEngine/Include/GameLogic/Object.h:702
Checking patch Generals/Code/GameEngine/Source/GameLogic/AI/AI.cpp...
Hunk #1 succeeded at 330 (offset -3 lines).
Hunk #2 succeeded at 344 (offset -3 lines).
Hunk #3 succeeded at 448 (offset -3 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/AI/AIGroup.cpp...
Hunk #5 succeeded at 2764 (offset -73 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp...
Hunk #1 succeeded at 893 (offset -7 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp...
Hunk #1 succeeded at 4053 (offset -121 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp...
Hunk #1 succeeded at 563 (offset -82 lines).
Hunk #2 succeeded at 3840 (offset -530 lines).
Hunk #3 succeeded at 5445 (offset -838 lines).
Hunk #4 succeeded at 5459 (offset -838 lines).
Hunk #5 succeeded at 5474 (offset -838 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp...
Hunk #1 succeeded at 394 (offset -25 lines).
Hunk #2 succeeded at 759 (offset -27 lines).
Hunk #3 succeeded at 1022 (offset -35 lines).
Hunk #4 succeeded at 1271 (offset -48 lines).
Hunk #5 succeeded at 1384 (offset -48 lines).
Hunk #6 succeeded at 1420 (offset -48 lines).
Hunk #7 succeeded at 1531 (offset -48 lines).
Hunk #8 succeeded at 1571 (offset -48 lines).
Hunk #9 succeeded at 1708 (offset -7 lines).
Hunk #10 succeeded at 1760 (offset -22 lines).
Hunk #11 succeeded at 1811 (offset -23 lines).
Hunk #12 succeeded at 1645 (offset -240 lines).
Hunk #13 succeeded at 1911 (offset -70 lines).
Hunk #14 succeeded at 1936 (offset -70 lines).
Hunk #15 succeeded at 1959 (offset -70 lines).
Hunk #16 succeeded at 2000 (offset -70 lines).
Hunk #17 succeeded at 3302 (offset -96 lines).
Hunk #18 succeeded at 4145 (offset -331 lines).
Hunk #19 succeeded at 4183 (offset -331 lines).
Hunk #20 succeeded at 4221 (offset -331 lines).
Hunk #21 succeeded at 4308 (offset -331 lines).
Hunk #22 succeeded at 4524 (offset -331 lines).
Hunk #23 succeeded at 4634 (offset -331 lines).
Hunk #24 succeeded at 5023 (offset -424 lines).
Hunk #25 succeeded at 5056 (offset -424 lines).
Hunk #26 succeeded at 5208 (offset -339 lines).
Hunk #27 succeeded at 5162 (offset -428 lines).
Hunk #28 succeeded at 5123 (offset -517 lines).
Hunk #29 succeeded at 5410 (offset -282 lines).
Hunk #30 succeeded at 5362 (offset -381 lines).
Hunk #31 succeeded at 5315 (offset -479 lines).
Hunk #32 succeeded at 5268 (offset -578 lines).
Hunk #33 succeeded at 5517 (offset -463 lines).
Hunk #34 succeeded at 5637 (offset -463 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp...
Hunk #1 succeeded at 7210 (offset -723 lines).
Hunk #2 succeeded at 7301 (offset -723 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp...
error: while searching for:
                        return;
                }

                AIGroup *group = NULL;
                CRCGEN_LOG(( "Creating AIGroup in GameLogic::selectObject()\n" ));
                group = TheAI->createGroup();
                group->add(obj);

                // add all selected agents to the AI group
                if (createNewSelection)
                {
                        player->setCurrentlySelectedAIGroup(group);
                }
                else
                {
                        player->addAIGroupToCurrentSelection(group);
                }

                TheAI->destroyGroup(group);

                if( affectClient )
                {

error: patch failed: Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp:2684
Hunk #2 succeeded at 2392 (offset -345 lines).
Checking patch Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp...
Hunk #1 succeeded at 339 (offset -9 lines).
Hunk #2 succeeded at 351 (offset -9 lines).
Hunk #3 succeeded at 360 (offset -9 lines).
Hunk #4 succeeded at 456 (offset -9 lines).
Hunk #5 succeeded at 664 (offset -23 lines).
error: while searching for:
                        Object* source = TheGameLogic->findObjectByID(sourceID);
                        if (source != NULL)
                        {
                                AIGroup* theGroup = TheAI->createGroup();
                                theGroup->add(source);
                                theGroup->groupDoSpecialPowerAtLocation( specialPowerID, &targetCoord, angle, objectInWay, options );
                                TheAI->destroyGroup(theGroup);
                        }
                        else
                        {

error: patch failed: Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp:716
Hunk #7 succeeded at 745 (offset -30 lines).
Hunk #8 succeeded at 995 (offset -32 lines).
Hunk #9 succeeded at 1186 (offset -32 lines).
Hunk #10 succeeded at 1305 (offset -32 lines).
Hunk #11 succeeded at 1335 (offset -32 lines).
Hunk #12 succeeded at 1372 (offset -32 lines).
Hunk #13 succeeded at 1408 (offset -32 lines).
Hunk #14 succeeded at 1459 (offset -32 lines).
Hunk #15 succeeded at 1724 (offset -32 lines).
Hunk #16 succeeded at 1770 (offset -32 lines).
Hunk #17 succeeded at 2021 (offset -32 lines).
Hunk #18 succeeded at 2049 (offset -32 lines).
Applied patch Generals/Code/GameEngine/Include/Common/Player.h cleanly.
Applied patch Generals/Code/GameEngine/Include/GameLogic/AI.h cleanly.
Applying patch Generals/Code/GameEngine/Include/GameLogic/Object.h with 1 reject...
Hunk #1 applied cleanly.
Rejected hunk #2.
Applied patch Generals/Code/GameEngine/Source/GameLogic/AI/AI.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/AI/AIGroup.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/AI/AIPlayer.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptActions.cpp cleanly.
Applied patch Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp cleanly.
Applying patch Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.
Applying patch Generals/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp with 1 reject...
Hunk #1 applied cleanly.
Hunk #2 applied cleanly.
Hunk #3 applied cleanly.
Hunk #4 applied cleanly.
Hunk #5 applied cleanly.
Rejected hunk #6.
Hunk #7 applied cleanly.
Hunk #8 applied cleanly.
Hunk #9 applied cleanly.
Hunk #10 applied cleanly.
Hunk #11 applied cleanly.
Hunk #12 applied cleanly.
Hunk #13 applied cleanly.
Hunk #14 applied cleanly.
Hunk #15 applied cleanly.
Hunk #16 applied cleanly.
Hunk #17 applied cleanly.
Hunk #18 applied cleanly.

@xezon
Copy link
Author

xezon commented Jul 1, 2025

Compilation tested with RETAIL_COMPATIBLE_CRC 0 for both titles. Runtime tested in Zero Hour.

@xezon xezon merged commit 99a5bc5 into TheSuperHackers:main Jul 1, 2025
14 checks passed
@xezon xezon deleted the xezon/fix-aigroup-6 branch July 1, 2025 17:48
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working right, typically is user facing Crash This is a crash, very bad Critical Severity: Minor < Major < Critical < Blocker Gen Relates to Generals NoRetail This fix or change is not applicable with Retail game compatibility Performance Is a performance concern ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Game can crash when clicking on player icon in Replay Mode

3 participants