Skip to content

bugfix(object): Clean up stranded occupants and preserve teardown lookup #3308

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

Open
bobtista wants to merge 9 commits into
base: main
Choose a base branch
from
Open

bugfix(object): Clean up stranded occupants and preserve teardown lookup #3308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a4f6a80
bugfix(object): Fix crash when destroying an occupant of an already d…
bobtista Sep 17, 2026
c0efa7a
refactor: clear the object hash table after destroyAllObjectsImmediate
Caball009 Sep 18, 2026
a25c611
refactor: Improve performance
Caball009 Sep 18, 2026
59f318f
refactor(object): Extract tunnel containment cleanup
bobtista Sep 22, 2026
d3c76f9
refactor(generals): Keep tunnel cleanup private and clarify its limits
bobtista Sep 24, 2026
c7c29a6
refactor(gamelogic): Clear the Zero Hour object lookup table after de…
bobtista Oct 2, 2026
555259a
unify(object): Gate the Generals tunnel occupant cleanup and mirror i…
bobtista Oct 5, 2026
6fd4b0a
refactor(object): Make removeFromTunnelContain protected
bobtista Oct 5, 2026
8f37699
docs(object): Explain the stale tunnel pointer read
bobtista Oct 5, 2026
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 Generals/Code/GameEngine/Include/Common/TunnelTracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class TunnelTracker : public MemoryPoolObject,

Bool isValidContainerFor(const Object* obj, Bool checkCapacity) const;
void addToContainList( Object *obj ); ///< add 'obj' to contain list
void removeFromContain( Object *obj, Bool exposeStealthUnits = FALSE ); ///< remove 'obj' from contain list
Bool removeFromContain( Object *obj, Bool exposeStealthUnits = FALSE ); ///< remove 'obj' from contain list
Bool isInContainer( Object *obj ); ///< Is this thing inside?

void onTunnelCreated( const Object *newTunnel ); ///< A tunnel was made
Expand Down
4 changes: 4 additions & 0 deletions Generals/Code/GameEngine/Include/GameLogic/Object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,10 @@ class Object : public Thing, public Snapshot

virtual void reactToTransformChange(const Matrix3D* oldMtx, const Coord3D* oldPos, Real oldAngle) override;

#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
void removeFromTunnelContain();

@xezon xezon Oct 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: Maybe this should be protected like all the other ones above?

@bobtista bobtista Oct 5, 2026

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.

Done

#endif

private:

// yes, private. No, really. Private. Don't expose.
Expand Down
5 changes: 4 additions & 1 deletion Generals/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ void TunnelTracker::addToContainList( Object *obj )
}

// ------------------------------------------------------------------------
void TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
Bool TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
{

ContainedItemsList::iterator it = std::find(m_containList.begin(), m_containList.end(), obj);
Expand All @@ -212,8 +212,11 @@ void TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
DEBUG_ASSERTCRASH(m_heroUnitsContained > 0, ("TunnelTracker::removeFromContain - Removing hero but hero count is %d", m_heroUnitsContained));
--m_heroUnitsContained;
}

return true;
}

return false;
}

// ------------------------------------------------------------------------
Expand Down
36 changes: 35 additions & 1 deletion Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@
#include "Common/Team.h"
#include "Common/ThingFactory.h"
#include "Common/ThingTemplate.h"
#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
#include "Common/TunnelTracker.h"
#endif
#include "Common/Upgrade.h"
#include "Common/WellKnownKeys.h"
#include "Common/Xfer.h"
Expand Down Expand Up @@ -636,6 +639,23 @@ void Object::onRemovedFrom( Object *removedFrom )
m_containedByFrame = 0;
}

#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
//-------------------------------------------------------------------------------------------------
void Object::removeFromTunnelContain()
{
for (Int i = 0; i < ThePlayerList->getPlayerCount(); ++i)
{
TunnelTracker* tracker = ThePlayerList->getNthPlayer(i)->getTunnelSystem();
if (tracker && tracker->removeFromContain(this))
{
break;
}
}

onRemovedFrom(nullptr);
}
#endif

//-------------------------------------------------------------------------------------------------
//-------------------------------------------------------------------------------------------------
Int Object::getTransportSlotCount() const
Expand Down Expand Up @@ -695,7 +715,21 @@ void Object::onDestroy()
// This is the old cleanUpContain safeguard. Say goodbye so they don't try to look us up.
if( m_containedBy && m_containedBy->getContain() )
{
m_containedBy->getContain()->removeFromContain( this );
#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC

@xezon xezon Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am skeptical about this change because it explicitly addresses Generals, indicating that this is no issue in Zero Hour, which then begs the question why and can we merge the Zero Hour code responsible for fixing this instead of having a separate fix for Generals that eventually will conflict with Zero Hour when merging?

@Caball009 Caball009 Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

TunnelContain::onCapture is not retail compatible in Generals, hence Generals specific issues.

@xezon xezon Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will all the code of this change disappear on merge?

@Caball009 Caball009 Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only if the merge coincides with abandoning retail compatibility.

@xezon xezon Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ok can you review this change?

@Caball009 Caball009 Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will do.

@Caball009 Caball009 Oct 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@xezon The Generals specific changes / additions need to be replicated to Zero Hour, right? (behind RTS_GENERALS && RETAIL_COMPATIBLE_CRC).

@xezon xezon Oct 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think he just applied that.

@bobtista bobtista Oct 5, 2026

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.

Yes, it is mirrored into Zero Hour behind the same macro.

// TheSuperHackers @bugfix bobtista / Caball009 17/09/2026 Remove stranded tunnel occupants during destruction.
// Surrendering can transfer a tunnel without updating its tunnel tracker. If that tunnel is destroyed,
// m_containedBy still points to the freed tunnel, and its ID is read here through that stale pointer.
// An unregistered ID means the tunnel is gone, so this object is removed from the tunnel trackers directly.
// This is a limited workaround that keeps retail compatibility. It cannot detect reuse of the freed memory.
if (!TheGameLogic->findObjectByID(m_containedBy->getID()))

@greptile-apps greptile-apps Bot Oct 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Stale container still dereferenced When a transferred tunnel has been freed but an occupant still points to it, this check reads the freed tunnel to get its ID. The condition above also reads it to get its contain module. If that memory is inaccessible, destruction can crash before the new cleanup runs. If the memory has been reused by a registered object with a contain module, the lookup can instead take the normal removal path and leave the occupant in its tunnel tracker.

@bobtista bobtista Oct 2, 2026

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.

Yes, it still reads through the stale pointer and cannot detect memory reused by another live object. Those limitations are in the description. The underlying dangling-pointer issue remains in #3316.

@greptile-apps greptile-apps Bot Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed. The PR description explicitly documents both limitations, and #3316 tracks the broader dangling-m_containedBy issue during gameplay. This change is intentionally a teardown-only safeguard, so I don’t have an additional actionable concern on this line.

{
removeFromTunnelContain();
}
else
#endif
{
m_containedBy->getContain()->removeFromContain(this);
}
}

//
Expand Down
16 changes: 8 additions & 8 deletions Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,14 +410,6 @@ void GameLogic::reset()
m_thingTemplateBuildableOverrides.clear();
m_controlBarOverrides.clear();

// set the hash to be rather large. We need to optimize this value later.
m_objHash.clear();
#if USING_STLPORT
m_objHash.resize(OBJ_HASH_SIZE);
#else
m_objHash.reserve(OBJ_HASH_SIZE);
#endif

m_pauseFrame = 0;
m_pauseSound = FALSE;
m_pauseMusic = FALSE;
Expand All @@ -432,6 +424,14 @@ void GameLogic::reset()
// destroy all objects
destroyAllObjectsImmediate();

// set the hash to be rather large. We need to optimize this value later.
m_objHash.clear();
#if USING_STLPORT
m_objHash.resize(OBJ_HASH_SIZE);
#else
m_objHash.reserve(OBJ_HASH_SIZE);
#endif

m_nextObjID = (ObjectID)1;

m_frameObjectsChangedTriggerAreas = 0;
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Include/Common/TunnelTracker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class TunnelTracker : public MemoryPoolObject,

Bool isValidContainerFor(const Object* obj, Bool checkCapacity) const;
void addToContainList( Object *obj ); ///< add 'obj' to contain list
void removeFromContain( Object *obj, Bool exposeStealthUnits = FALSE ); ///< remove 'obj' from contain list
Bool removeFromContain( Object *obj, Bool exposeStealthUnits = FALSE ); ///< remove 'obj' from contain list
Bool isInContainer( Object *obj ); ///< Is this thing inside?

void onTunnelCreated( const Object *newTunnel ); ///< A tunnel was made
Expand Down
4 changes: 4 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,10 @@ class Object : public Thing, public Snapshot

virtual void reactToTransformChange(const Matrix3D* oldMtx, const Coord3D* oldPos, Real oldAngle) override;

#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
void removeFromTunnelContain();
#endif

private:

// yes, private. No, really. Private. Don't expose.
Expand Down
5 changes: 4 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ void TunnelTracker::addToContainList( Object *obj )
}

// ------------------------------------------------------------------------
void TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
Bool TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
{

ContainedItemsList::iterator it = std::find(m_containList.begin(), m_containList.end(), obj);
Expand All @@ -213,8 +213,11 @@ void TunnelTracker::removeFromContain( Object *obj, Bool exposeStealthUnits )
DEBUG_ASSERTCRASH(m_heroUnitsContained > 0, ("TunnelTracker::removeFromContain - Removing hero but hero count is %d", m_heroUnitsContained));
--m_heroUnitsContained;
}

return true;
}

return false;
}

// ------------------------------------------------------------------------
Expand Down
36 changes: 35 additions & 1 deletion GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@
#include "Common/Team.h"
#include "Common/ThingFactory.h"
#include "Common/ThingTemplate.h"
#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
#include "Common/TunnelTracker.h"
#endif
#include "Common/Upgrade.h"
#include "Common/WellKnownKeys.h"
#include "Common/Xfer.h"
Expand Down Expand Up @@ -711,6 +714,23 @@ void Object::onRemovedFrom( Object *removedFrom )

}

#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
//-------------------------------------------------------------------------------------------------
void Object::removeFromTunnelContain()
{
for (Int i = 0; i < ThePlayerList->getPlayerCount(); ++i)
{
TunnelTracker* tracker = ThePlayerList->getNthPlayer(i)->getTunnelSystem();
if (tracker && tracker->removeFromContain(this))
{
break;
}
}

onRemovedFrom(nullptr);
}
#endif

//-------------------------------------------------------------------------------------------------
//-------------------------------------------------------------------------------------------------
Int Object::getTransportSlotCount() const
Expand Down Expand Up @@ -770,7 +790,21 @@ void Object::onDestroy()
// This is the old cleanUpContain safeguard. Say goodbye so they don't try to look us up.
if( m_containedBy && m_containedBy->getContain() )
{
m_containedBy->getContain()->removeFromContain( this );
#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
// TheSuperHackers @bugfix bobtista / Caball009 17/09/2026 Remove stranded tunnel occupants during destruction.
// Surrendering can transfer a tunnel without updating its tunnel tracker. If that tunnel is destroyed,
// m_containedBy still points to the freed tunnel, and its ID is read here through that stale pointer.
// An unregistered ID means the tunnel is gone, so this object is removed from the tunnel trackers directly.
// This is a limited workaround that keeps retail compatibility. It cannot detect reuse of the freed memory.
if (!TheGameLogic->findObjectByID(m_containedBy->getID()))
{
removeFromTunnelContain();
}
else
#endif
{
m_containedBy->getContain()->removeFromContain(this);
}
}

//
Expand Down
12 changes: 6 additions & 6 deletions GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,12 +420,6 @@ void GameLogic::reset()
m_thingTemplateBuildableOverrides.clear();
m_controlBarOverrides.clear();

// set the hash to be rather large. We need to optimize this value later.
// m_objHash.clear();
// m_objHash.resize(OBJ_HASH_SIZE);
m_objVector.clear();
m_objVector.resize(OBJ_HASH_SIZE, nullptr);

m_pauseFrame = 0;
m_pauseSound = FALSE;
m_pauseMusic = FALSE;
Expand All @@ -440,6 +434,12 @@ void GameLogic::reset()
// destroy all objects
destroyAllObjectsImmediate();

// set the hash to be rather large. We need to optimize this value later.
// m_objHash.clear();
// m_objHash.resize(OBJ_HASH_SIZE);
m_objVector.clear();
m_objVector.resize(OBJ_HASH_SIZE, nullptr);

m_nextObjID = (ObjectID)1;

m_frameObjectsChangedTriggerAreas = 0;
Expand Down
Loading