Skip to content

[GEN][ZH] Fix undefined behavior with MemoryPoolObject::deleteInstance#870

Merged
xezon merged 4 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-deleteinstance
May 18, 2025
Merged

[GEN][ZH] Fix undefined behavior with MemoryPoolObject::deleteInstance#870
xezon merged 4 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-deleteinstance

Conversation

@xezon

@xezon xezon commented May 16, 2025

Copy link
Copy Markdown

Merge with Rebase

This change fixes the undefined behaviour in MemoryPoolObject::deleteInstance by making it a static function. This way null pointers are no longer dereferenced when calling it. This change was mostly applied with a python script to avoid editing 1000 lines by hand.

MemoryPoolObject::deleteInstance also gets an alias function deleteInstance for simplicity.

The MEMORY_POOL_DELETEINSTANCE_VISIBILITY macro was removed because it no longer is applicable with the static function. Instead, accidental calls to deleteInstance(Object*) are prevented with a deleted function.

@xezon xezon added this to the Stability fixes milestone May 16, 2025
@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Fix Is fixing something, but is not user facing Stability Concerns stability of the runtime labels May 16, 2025
@xezon xezon requested a review from Mauller May 16, 2025 21:43
@xezon xezon force-pushed the xezon/fix-deleteinstance branch from 0d29aab to aff2d7d Compare May 17, 2025 08:21

@Mauller Mauller left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nothing wrong with how things have been changed, just a few places where things could be cleaned up due to the change in implementation.

Comment thread Generals/Code/GameEngine/Include/Common/GameMemory.h Outdated

@Mauller Mauller May 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same heres as mentioned above, might be better to use mpo as the argument name. Although this one is talking about actual object class objects, but maybe the argument should be of type MemoryPoolObject*?

@xezon xezon May 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I actually intentionally put Object* here, because it is meant to destroy an Object, and not some other type right?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah it makes sense, i was considering that the underlying deleteInstance expects a MemoryPoolObject so that gets hidden in a way. But i think an Object is always a MemoryPoolObject so it shouldn't be an issue.

Comment thread Generals/Code/GameEngine/Include/GameLogic/Object.h Outdated
Comment thread Generals/Code/GameEngine/Source/Common/INI/INI.cpp Outdated
Comment thread Generals/Code/GameEngine/Source/GameClient/Drawable.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameClient/Snow.cpp Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

begin->second to match others

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp Outdated
Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/ObjectCreationList.cpp Outdated
Comment thread GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/Water/W3DWater.cpp Outdated
@xezon xezon force-pushed the xezon/fix-deleteinstance branch from aff2d7d to 36655fd Compare May 17, 2025 18:21
@xezon

xezon commented May 17, 2025

Copy link
Copy Markdown
Author

Addressed most comments. See fixup! commits.

@xezon xezon force-pushed the xezon/fix-deleteinstance branch from 36655fd to 3a6ea79 Compare May 17, 2025 20:28
@xezon

xezon commented May 17, 2025

Copy link
Copy Markdown
Author

Tested against VS6 golden replay. Passed that.

@xezon xezon merged commit 147e944 into TheSuperHackers:main May 18, 2025
@xezon xezon deleted the xezon/fix-deleteinstance branch May 18, 2025 06:47
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Feb 23, 2026
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Feb 23, 2026
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants