Skip to content

[ZH] Minor memory corruption and leak fixes #716

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.

Sign up for GitHub

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

Closed
helmutbuhler wants to merge 8 commits into from
Closed

[ZH] Minor memory corruption and leak fixes #716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61ac7bd
[CORE] Move matchbot tool to Core (#706)
xezon Apr 19, 2025
bcfe2e4
[GEN] Backport BlurTime functionality in WW3D2's ParticleBufferClass …
tomsons26 Apr 19, 2025
3d4b5c7
[GEN][ZH] Remove unused granny related code (#708)
Mauller Apr 19, 2025
36bd757
Fix drawable leak in ParticleUplinkCannonUpdate::createGroundToOrbitL…
helmutbuhler Apr 19, 2025
c631722
Fix wrong iterator usage (would crash on VS22)
helmutbuhler Apr 19, 2025
f5e086f
Fix uninitialized memory access in DockUpdate::loadDockPositions()
helmutbuhler Apr 19, 2025
770ae18
Fix draw assignment in StealthUpdate::changeVisualDisguise()
helmutbuhler Apr 13, 2025
48d6592
Revert "[GEN][ZH] Remove unused granny related code (#708)"
helmutbuhler Apr 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/DumbProjectileBehavior.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ Bool DumbProjectileBehavior::projectileHandleCollision( Object *other )
const ContainedItemsList* items = contain->getContainedItemsList();
if (items)
{
for (ContainedItemsList::const_iterator it = items->begin(); *it != NULL && numKilled < d->m_garrisonHitKillCount; )
for (ContainedItemsList::const_iterator it = items->begin(); it != items->end() && numKilled < d->m_garrisonHitKillCount; )

@Mauller Mauller Apr 20, 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.

Looks genuine to me, should not affect VC6 as the original behaviour was fine for STLPort.

If you can replicate this and the other iterator fix to generals as well, that would be great.

{
Object* thingToKill = *it++;
if (!thingToKill->isEffectivelyDead() && thingToKill->isKindOfMulti(d->m_garrisonHitKillKindof, d->m_garrisonHitKillKindofNot))
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/MissileAIUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ Bool MissileAIUpdate::projectileHandleCollision( Object *other )
const ContainedItemsList* items = contain->getContainedItemsList();
if (items)
{
for (ContainedItemsList::const_iterator it = items->begin(); *it != NULL && numKilled < d->m_garrisonHitKillCount; )
for (ContainedItemsList::const_iterator it = items->begin(); it != items->end() && numKilled < d->m_garrisonHitKillCount; )
{
Object* thingToKill = *it++;
if (!thingToKill->isEffectivelyDead() && thingToKill->isKindOfMulti(d->m_garrisonHitKillKindof, d->m_garrisonHitKillKindofNot))
Expand Down
9 changes: 8 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/DockUpdate/DockUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,14 @@ void DockUpdate::loadDockPositions()
if( m_numberApproachPositions != DYNAMIC_APPROACH_VECTOR_FLAG )
{
// Dynamic means no bones
Coord3D approachBones[DEFAULT_APPROACH_VECTOR_SIZE];
// TheSuperHackers @fix helmutbuhler 04/19/2025
// approachBones was originally not initialized. It needs to be initialized because
// myDrawable->getPristineBonePositions only sets a certain amount of elements
// (m_numberApproachPositionBones), but the following code copies a different amount
// (m_numberApproachPositions) into m_approachPositions (which CRC indirectly depends on).
// Luckily, just setting it to zero seems to work.

Coord3D approachBones[DEFAULT_APPROACH_VECTOR_SIZE] = {0};
m_numberApproachPositionBones = myDrawable->getPristineBonePositions( "DockWaiting", 1, approachBones, NULL, m_numberApproachPositions);
if( m_numberApproachPositions == m_approachPositions.size() )//safeguard: will always be true
{
Expand Down
5 changes: 4 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/ParticleUplinkCannonUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,10 @@ void ParticleUplinkCannonUpdate::createGroundToOrbitLaser( UnsignedInt growthFra
if( beam )
{
TheGameClient->destroyDrawable( beam );
m_orbitToTargetBeamID = INVALID_DRAWABLE_ID;

// TheSuperHackers @fix helmutbuhler 04/19/2025
// This originally mistakenly assigned to m_orbitToTargetBeamID and thus caused a leak.
m_groundToOrbitBeamID = INVALID_DRAWABLE_ID;

@Mauller Mauller Apr 20, 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.

Looks legitimate to me, will just need to check against VC6 that we don't cause a mismatch by this change,

}

if( data->m_particleBeamLaserName.isNotEmpty() )
Expand Down
4 changes: 3 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/StealthUpdate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,7 +1055,9 @@ void StealthUpdate::changeVisualDisguise()

const ThingTemplate *tTemplate = self->getTemplate();

TheThingFactory->newDrawable( tTemplate );
// TheSuperHackers @bugfix helmutbuhler 04/13/2025
// draw was originally not assigned here and potentially caused memory corruption
draw = TheThingFactory->newDrawable( tTemplate );

@Mauller Mauller Apr 20, 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.

Looks genuine, might need checking again against VC6 since it could cause a mismatch even though it fixes behaviour.

if( draw )
{
TheGameLogic->bindObjectAndDrawable(self, draw);
Expand Down