-
Notifications
You must be signed in to change notification settings - Fork 74
[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() #1004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This fix was harder than it looks. I think I spent 15+ hours on this... |
c819bd4
to
a406fe6
Compare
Rebased on main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, i can see how the messaging side would have caused issues.
But i think a few places can be cleaned up a bit due to the way the ref pointers work.
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogicDispatch.cpp
Show resolved
Hide resolved
…er is selected in Replay playback
a406fe6
to
55c46d9
Compare
RefCountClass replaced with RefCountValue to not change the size of class AIGroup. Replicated in Generals. |
Compile error fixed. Forgot to return values. |
@helmutbuhler Can you please test this change against a few more replays? |
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 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 itscurrentlySelectedGroup
variable. It holds objects in a AIGroup, but that AIGroup can be deleted by external events, for exampleThere 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.
TODO