From 69cfc9bcede389046ec6390f63f95004841ac493 Mon Sep 17 00:00:00 2001 From: Caball009 <82909616+Caball009@users.noreply.github.com> Date: Mon, 25 May 2026 17:08:12 +0200 Subject: [PATCH 1/5] bugfix(contain): Add workaround for use-after-free bug that could crash the game. --- .../GameEngine/Include/GameLogic/Object.h | 2 +- .../Source/GameLogic/Object/Object.cpp | 51 ++++++++++++++++--- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h b/GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h index bfdf333393f..29d391b81a3 100644 --- a/GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h +++ b/GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h @@ -761,7 +761,7 @@ class Object : public Thing, public Snapshot Object* m_containedBy; /**< an object can only be contained by at most one other object, this is that object (if present) */ - ObjectID m_xferContainedByID; ///< xfer uses IDs to store pointers and looks them up after + ObjectID m_containedByID; ///< ID of the object we're contained by; only to be used when m_containedBy cannot be used UnsignedInt m_containedByFrame; ///< frame we were contained by m_containedBy Real m_constructionPercent; ///< for objects being built ... this is the amount completed (0.0 to 100.0) diff --git a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp index a8029b4ec8d..7ecd6baaed5 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp @@ -182,7 +182,7 @@ Object::Object( const ThingTemplate *tt, const ObjectStatusMaskType &objectStatu m_physics(nullptr), m_geometryInfo(tt->getTemplateGeometryInfo()), m_containedBy(nullptr), - m_xferContainedByID(INVALID_ID), + m_containedByID(INVALID_ID), m_containedByFrame(0), m_behaviors(nullptr), m_body(nullptr), @@ -691,6 +691,17 @@ void Object::onContainedBy( Object *containedBy ) m_containedBy = containedBy; m_containedByFrame = TheGameLogic->getFrame(); +#if RETAIL_COMPATIBLE_CRC + if (containedBy && !containedBy->isDestroyed()) + { + m_containedByID = containedBy->getID(); + } + else + { + m_containedByID = INVALID_ID; + } +#endif + handlePartitionCellMaintenance(); // which should unlook me now that I am contained } @@ -704,6 +715,10 @@ void Object::onRemovedFrom( Object *removedFrom ) m_containedBy = nullptr; m_containedByFrame = 0; +#if RETAIL_COMPATIBLE_CRC + m_containedByID = INVALID_ID; +#endif + handlePartitionCellMaintenance(); // get a clean look, now that I am outdoors, again } @@ -756,9 +771,29 @@ 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() ) + if (m_containedBy) { - m_containedBy->getContain()->removeFromContain( this ); + if (ContainModuleInterface* contained = m_containedBy->getContain()) + { +#if RETAIL_COMPATIBLE_CRC + if (m_containedByID == INVALID_ID) + { + // TheSuperHackers @bugfix Caball009 25/05/2026 Due to a potential use-after-free bug that cannot be fixed + // with retail compatibility, the 'contained by' pointer of this object may point to an already destroyed object. + // Avoid removing this object from the contain list, because it could crash the game, + // as the begin / end iterator for STLPort and MSVC std::list implementations depends on dynamically allocated memory. + DEBUG_CRASH(("container object must be valid; this looks like use-after-free")); + } + else + { + DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, + ("contained by pointer is out of sync with contained by ID")); + contained->removeFromContain(this); + } +#else + contained->removeFromContain(this); +#endif + } } // @@ -4249,13 +4284,13 @@ void Object::xfer( Xfer *xfer ) if( xfer->getXferMode() == XFER_SAVE ) { if( m_containedBy != nullptr ) - m_xferContainedByID = m_containedBy->getID(); + m_containedByID = m_containedBy->getID(); else - m_xferContainedByID = INVALID_ID; + m_containedByID = INVALID_ID; } - xfer->xferObjectID( &m_xferContainedByID ); + xfer->xferObjectID( &m_containedByID ); } // contained by frame @@ -4480,8 +4515,8 @@ void Object::xfer( Xfer *xfer ) //------------------------------------------------------------------------------------------------- void Object::loadPostProcess() { - if( m_xferContainedByID != INVALID_ID ) - m_containedBy = TheGameLogic->findObjectByID(m_xferContainedByID); + if( m_containedByID != INVALID_ID ) + m_containedBy = TheGameLogic->findObjectByID(m_containedByID); else m_containedBy = nullptr; From 1af8f31291af2b7a997db06fe92ad6bb3b1dd7e1 Mon Sep 17 00:00:00 2001 From: Caball009 <82909616+Caball009@users.noreply.github.com> Date: Tue, 26 May 2026 11:53:50 +0200 Subject: [PATCH 2/5] Addressed feedback. --- .../Source/GameLogic/Object/Object.cpp | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp index 7ecd6baaed5..5d386241de8 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp @@ -700,6 +700,9 @@ void Object::onContainedBy( Object *containedBy ) { m_containedByID = INVALID_ID; } +#else + DEBUG_ASSERTCRASH(containedBy == nullptr || !containedBy->isDestroyed(), + ("Object::onContainedBy - Adding into a destroyed container")); #endif handlePartitionCellMaintenance(); // which should unlook me now that I am contained @@ -773,27 +776,28 @@ void Object::onDestroy() // This is the old cleanUpContain safeguard. Say goodbye so they don't try to look us up. if (m_containedBy) { - if (ContainModuleInterface* contained = m_containedBy->getContain()) - { #if RETAIL_COMPATIBLE_CRC - if (m_containedByID == INVALID_ID) - { - // TheSuperHackers @bugfix Caball009 25/05/2026 Due to a potential use-after-free bug that cannot be fixed - // with retail compatibility, the 'contained by' pointer of this object may point to an already destroyed object. - // Avoid removing this object from the contain list, because it could crash the game, - // as the begin / end iterator for STLPort and MSVC std::list implementations depends on dynamically allocated memory. - DEBUG_CRASH(("container object must be valid; this looks like use-after-free")); - } - else - { - DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, - ("contained by pointer is out of sync with contained by ID")); - contained->removeFromContain(this); - } + DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, + ("contained by pointer is out of sync with contained by ID")); + + if (m_containedByID == INVALID_ID) + { + // TheSuperHackers @bugfix Caball009 25/05/2026 Due to a potential use-after-free bug that cannot be fixed + // with retail compatibility, the 'contained by' pointer of this object may point to an already destroyed object. + // Avoid removing this object from the contain list, because it could crash the game, + // as the begin / end iterator for STLPort and MSVC std::list implementations depends on dynamically allocated memory. + DEBUG_CRASH(("container object must be valid; this looks like use-after-free")); + } + else if (ContainModuleInterface* contained = m_containedBy->getContain()) + { + contained->removeFromContain(this); + } #else + if (ContainModuleInterface* contained = m_containedBy->getContain()) + { contained->removeFromContain(this); -#endif } +#endif } // From 3ae7e44c3bcdaf8954ee1b0a00a840f7574c9180 Mon Sep 17 00:00:00 2001 From: Caball009 <82909616+Caball009@users.noreply.github.com> Date: Tue, 26 May 2026 12:28:49 +0200 Subject: [PATCH 3/5] Fixed xfer logic. --- GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp index 5d386241de8..4eb5bd75db2 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp @@ -4287,13 +4287,15 @@ void Object::xfer( Xfer *xfer ) // our responsibility. if( xfer->getXferMode() == XFER_SAVE ) { + // TheSuperHackers @tweak Contained by ID is already set with retail compatibility; don't overwrite it. +#if !RETAIL_COMPATIBLE_CRC if( m_containedBy != nullptr ) m_containedByID = m_containedBy->getID(); else m_containedByID = INVALID_ID; +#endif } - xfer->xferObjectID( &m_containedByID ); } From 6b4853997c43117c731f76591180393228fe1dfc Mon Sep 17 00:00:00 2001 From: Caball009 <82909616+Caball009@users.noreply.github.com> Date: Tue, 26 May 2026 14:04:54 +0200 Subject: [PATCH 4/5] Moved debug assertion. --- .../GameEngine/Source/GameLogic/Object/Object.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp index 4eb5bd75db2..ff357c4f14d 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp @@ -777,9 +777,6 @@ void Object::onDestroy() if (m_containedBy) { #if RETAIL_COMPATIBLE_CRC - DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, - ("contained by pointer is out of sync with contained by ID")); - if (m_containedByID == INVALID_ID) { // TheSuperHackers @bugfix Caball009 25/05/2026 Due to a potential use-after-free bug that cannot be fixed @@ -788,9 +785,15 @@ void Object::onDestroy() // as the begin / end iterator for STLPort and MSVC std::list implementations depends on dynamically allocated memory. DEBUG_CRASH(("container object must be valid; this looks like use-after-free")); } - else if (ContainModuleInterface* contained = m_containedBy->getContain()) + else { - contained->removeFromContain(this); + DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, + ("contained by pointer is out of sync with contained by ID")); + + if (ContainModuleInterface* contained = m_containedBy->getContain()) + { + contained->removeFromContain(this); + } } #else if (ContainModuleInterface* contained = m_containedBy->getContain()) From d49db75dba174640d54ae9906f8cfface4f95b60 Mon Sep 17 00:00:00 2001 From: Caball009 <82909616+Caball009@users.noreply.github.com> Date: Tue, 26 May 2026 23:41:28 +0200 Subject: [PATCH 5/5] Addressed feedback. --- .../Source/GameLogic/Object/Object.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp index ff357c4f14d..377a82f8832 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp @@ -692,6 +692,8 @@ void Object::onContainedBy( Object *containedBy ) m_containedByFrame = TheGameLogic->getFrame(); #if RETAIL_COMPATIBLE_CRC + // TheSuperHackers @info Set INVALID_ID if the container object was destroyed + // to indicate that the pointer will become a dangling pointer in the next frame. if (containedBy && !containedBy->isDestroyed()) { m_containedByID = containedBy->getID(); @@ -790,15 +792,15 @@ void Object::onDestroy() DEBUG_ASSERTCRASH(TheGameLogic->findObjectByID(m_containedByID) == m_containedBy, ("contained by pointer is out of sync with contained by ID")); - if (ContainModuleInterface* contained = m_containedBy->getContain()) + if (ContainModuleInterface* contain = m_containedBy->getContain()) { - contained->removeFromContain(this); + contain->removeFromContain(this); } } #else - if (ContainModuleInterface* contained = m_containedBy->getContain()) + if (ContainModuleInterface* contain = m_containedBy->getContain()) { - contained->removeFromContain(this); + contain->removeFromContain(this); } #endif } @@ -4288,16 +4290,16 @@ void Object::xfer( Xfer *xfer ) // No, the contain module is just going to friend_ reach in and set this for us. // Containers more complicated than Open (like Tunnel) can't do that. Our variable, // our responsibility. +#if !RETAIL_COMPATIBLE_CRC + // TheSuperHackers @tweak Contained by ID is already set with retail compatibility; don't overwrite it. if( xfer->getXferMode() == XFER_SAVE ) { - // TheSuperHackers @tweak Contained by ID is already set with retail compatibility; don't overwrite it. -#if !RETAIL_COMPATIBLE_CRC if( m_containedBy != nullptr ) m_containedByID = m_containedBy->getID(); else m_containedByID = INVALID_ID; -#endif } +#endif xfer->xferObjectID( &m_containedByID ); }