[GEN][ZH] Fix heap-use-after-free in W3DRadar::drawHeroIcon() (#1035)

Author: xezon

Core/GameEngine/CMakeLists.txt

@@ -112,6 +112,7 @@ set(GAMEENGINE_SRC
 #    Include/Common/StateMachine.h
 #    Include/Common/StatsCollector.h
 #    Include/Common/STLTypedefs.h
+    Include/Common/STLUtils.h
 #    Include/Common/StreamingArchiveFile.h
 #    Include/Common/SubsystemInterface.h
 #    Include/Common/SystemInfo.h

Core/GameEngine/Include/Common/STLUtils.h

@@ -0,0 +1,61 @@
+/*
+**	Command & Conquer Generals Zero Hour(tm)
+**	Copyright 2025 TheSuperHackers
+**
+**	This program is free software: you can redistribute it and/or modify
+**	it under the terms of the GNU General Public License as published by
+**	the Free Software Foundation, either version 3 of the License, or
+**	(at your option) any later version.
+**
+**	This program is distributed in the hope that it will be useful,
+**	but WITHOUT ANY WARRANTY; without even the implied warranty of
+**	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+**	GNU General Public License for more details.
+**
+**	You should have received a copy of the GNU General Public License
+**	along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#pragma once
+
+#include <Utility/CppMacros.h>
+#include <utility>
+
+namespace stl
+{
+
+// Finds first matching element in vector-like container and erases it.
+template <typename Container>
+bool find_and_erase(Container& container, const typename Container::value_type& value)
+{
+	typename Container::const_iterator it = container.begin();
+	for (; it != container.end(); ++it)
+	{
+		if (*it == value)
+		{
+			container.erase(it);
+			return true;
+		}
+	}
+	return false;
+}
+
+// Finds first matching element in vector-like container and removes it by swapping it with the last element.
+// This is generally faster than erasing from a vector, but will change the element sorting.
+template <typename Container>
+bool find_and_erase_unordered(Container& container, const typename Container::value_type& value)
+{
+	typename Container::iterator it = container.begin();
+	for (; it != container.end(); ++it)
+	{
+		if (*it == value)
+		{
+			*it = CPP_11(std::move)(container.back());
+			container.pop_back();
+			return true;
+		}
+	}
+	return false;
+}
+
+} // namespace stl

Generals/Code/GameEngine/Include/Common/GameEngine.h

@@ -85,6 +85,8 @@ class GameEngine : public SubsystemInterface
 
 protected:
 
+	virtual void resetSubsystems( void );
+
 	virtual FileSystem *createFileSystem( void );								///< Factory for FileSystem classes
 	virtual LocalFileSystem *createLocalFileSystem( void ) = 0;	///< Factory for LocalFileSystem classes
 	virtual ArchiveFileSystem *createArchiveFileSystem( void ) = 0;	///< Factory for ArchiveFileSystem classes

Generals/Code/GameEngine/Include/Common/Radar.h

@@ -190,9 +190,8 @@ class Radar : public Snapshot,
  	Bool tryEvent( RadarEventType event, const Coord3D *pos );	///< try to make a "stealth" event
 
 	// adding and removing objects from the radar
-	void addObject( Object *obj );													///< add object to radar
-	void removeObject( Object *obj );												///< remove object from radar
-	void examineObject( Object *obj );											///< re-examine object and resort if needed
+	virtual bool addObject( Object *obj );									///< add object to radar
+	virtual bool removeObject( Object *obj );								///< remove object from radar
 
 	// radar options
 	void hide( Bool hide ) { m_radarHidden = hide; }				///< hide/unhide the radar

Generals/Code/GameEngine/Include/Common/STLTypedefs.h

@@ -58,6 +58,7 @@ class STLSpecialAlloc;
 #include "Common/UnicodeString.h"
 #include "Common/GameCommon.h"
 #include "Common/GameMemory.h"
+#include "Common/STLUtils.h"
 
 //-----------------------------------------------------------------------------
 

Generals/Code/GameEngine/Source/Common/GameEngine.cpp

@@ -509,7 +509,8 @@ void GameEngine::init()
 
 	initDisabledMasks();
 
-	TheSubsystemList->resetAll();
+	resetSubsystems();
+
 	HideControlBar();
 }  // end init
 
@@ -528,7 +529,7 @@ void GameEngine::reset( void )
 	if (TheGameLogic->isInMultiplayerGame())
 		deleteNetwork = true;
 
-	TheSubsystemList->resetAll();
+	resetSubsystems();
 
 	if (deleteNetwork)
 	{
@@ -545,6 +546,16 @@ void GameEngine::reset( void )
 	}
 }
 
+/// -----------------------------------------------------------------------------------------------
+void GameEngine::resetSubsystems( void )
+{
+	// TheSuperHackers @fix xezon 09/06/2025 Reset GameLogic first to purge all world objects early.
+	// This avoids potentially catastrophic issues when objects and subsystems have cross dependencies.
+	TheGameLogic->reset();
+
+	TheSubsystemList->resetAll();
+}
+
 /// -----------------------------------------------------------------------------------------------
 DECLARE_PERF_TIMER(GameEngine_update)
 

Generals/Code/GameEngine/Source/Common/System/Radar.cpp

@@ -394,13 +394,13 @@ void Radar::newMap( TerrainLogic *terrain )
 /** Add an object to the radar list.  The object will be sorted in the list to be grouped
 	* using it's radar priority */
 //-------------------------------------------------------------------------------------------------
-void Radar::addObject( Object *obj )
+bool Radar::addObject( Object *obj )
 {
 
 	// get the radar priority for this object
 	RadarPriorityType newPriority = obj->getRadarPriority();
 	if( isPriorityVisible( newPriority ) == FALSE )
-		return;
+		return false;
 
 	// if this object is on the radar, remove it in favor of the new add
 	RadarObject **list;
@@ -544,6 +544,7 @@ void Radar::addObject( Object *obj )
 
 	}  // end else
 
+	return true;
 }  // end addObject
 
 //-------------------------------------------------------------------------------------------------
@@ -590,24 +591,24 @@ Bool Radar::deleteFromList( Object *obj, RadarObject **list )
 //-------------------------------------------------------------------------------------------------
 /** Remove an object from the radar, the object may reside in any list */
 //-------------------------------------------------------------------------------------------------
-void Radar::removeObject( Object *obj )
+bool Radar::removeObject( Object *obj )
 {
 
 	// sanity
 	if( obj->friend_getRadarData() == NULL )
-		return;
+		return false;
 
 	if( deleteFromList( obj, &m_localObjectList ) == TRUE )
-		return;
+		return true;
 	else if( deleteFromList( obj, &m_objectList ) == TRUE )
-		return;
+		return true;
 	else
 	{
 
 		// sanity
 		DEBUG_ASSERTCRASH( 0, ("Radar: Tried to remove object '%s' which was not found\n",
 											 obj->getTemplate()->getName().str()) );
-
+		return false;
 	}  // end else
 
 }  // end removeObject

Generals/Code/GameEngineDevice/Include/W3DDevice/Common/W3DRadar.h

@@ -59,6 +59,9 @@ class W3DRadar : public Radar
 	virtual void update( void );																	///< subsystem update
 	virtual void reset( void );																		///< subsystem reset
 
+	virtual bool addObject( Object *obj );									///< add object to radar
+	virtual bool removeObject( Object *obj );								///< remove object from radar
+
 	virtual void newMap( TerrainLogic *terrain );				///< reset radar for new map
 
 	void draw( Int pixelX, Int pixelY, Int width, Int height );		///< draw the radar
@@ -80,7 +83,7 @@ class W3DRadar : public Radar
 	void drawViewBox( Int pixelX, Int pixelY, Int width, Int height );  ///< draw view box
 	void buildTerrainTexture( TerrainLogic *terrain );	 ///< create the terrain texture of the radar
 	void drawIcons( Int pixelX, Int pixelY, Int width, Int height );	///< draw all of the radar icons
-	void renderObjectList( const RadarObject *listHead, TextureClass *texture, Bool calcHero = FALSE );			 ///< render an object list to the texture
+	void renderObjectList( const RadarObject *listHead, TextureClass *texture );			 ///< render an object list to the texture
 	void interpolateColorForHeight( RGBColor *color, 
 																	Real height, 
 																	Real hiZ, 
@@ -118,7 +121,7 @@ class W3DRadar : public Radar
 	Real m_viewZoom;															///< camera zoom used for the view box we have	
 	ICoord2D m_viewBox[ 4 ];											///< radar cell points for the 4 corners of view box
 
-	std::list<const Coord3D *> m_cachedHeroPosList;					//< cache of hero positions for drawing icons in radar overlay
+	std::vector<const Object *> m_cachedHeroObjectList; //< cache of hero objects for drawing icons in radar overlay
 };
 
 

Generals/Code/GameEngineDevice/Source/W3DDevice/Common/System/W3DRadar.cpp

@@ -608,18 +608,18 @@ void W3DRadar::drawEvents( Int pixelX, Int pixelY, Int width, Int height )
 void W3DRadar::drawIcons( Int pixelX, Int pixelY, Int width, Int height )
 {
 	// draw the hero icons
-	std::list<const Coord3D *>::const_iterator iter = m_cachedHeroPosList.begin();
-	while (iter != m_cachedHeroPosList.end())
+	std::vector<const Object *>::const_iterator iter = m_cachedHeroObjectList.begin();
+	while (iter != m_cachedHeroObjectList.end())
 	{
-		drawHeroIcon( pixelX, pixelY, width, height, (*iter) );
+		drawHeroIcon( pixelX, pixelY, width, height, (*iter)->getPosition() );
 		++iter;
 	}
 }
 
 //-------------------------------------------------------------------------------------------------
 /** Render an object list into the texture passed in */
 //-------------------------------------------------------------------------------------------------
-void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *texture, Bool calcHero )
+void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *texture )
 {
 
 	// sanity
@@ -637,12 +637,6 @@ void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *text
 	if (player)
 		playerIndex=player->getPlayerIndex();
 
-	if( calcHero )
-	{
-		// clear all entries from the cached hero object list
-		m_cachedHeroPosList.clear();
-	}
-
 	for( const RadarObject *rObj = listHead; rObj; rObj = rObj->friend_getNext() )
 	{
 
@@ -652,11 +646,6 @@ void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *text
 		// get object
 		const Object *obj = rObj->friend_getObject();
 
-		// cache hero object positions for drawing in icon layer
-		if( calcHero && obj->isHero() )
-		{
-			m_cachedHeroPosList.push_back(obj->getPosition());
-		}
     Bool skip = FALSE;
 
 		// check for shrouded status
@@ -1001,6 +990,34 @@ void W3DRadar::update( void )
 
 }  // end update
 
+//-------------------------------------------------------------------------------------------------
+bool W3DRadar::addObject( Object *obj )
+{
+	if (Radar::addObject(obj))
+	{
+		if (obj->isHero())
+		{
+			m_cachedHeroObjectList.push_back(obj);
+		}
+		return true;
+	}
+	return false;
+}
+
+//-------------------------------------------------------------------------------------------------
+bool W3DRadar::removeObject( Object *obj )
+{
+	if (Radar::removeObject(obj))
+	{
+		if (obj->isHero())
+		{
+			stl::find_and_erase_unordered(m_cachedHeroObjectList, obj);
+		}
+		return true;
+	}
+	return false;
+}
+
 //-------------------------------------------------------------------------------------------------
 /** Reset the radar for the new map data being given to it */
 //-------------------------------------------------------------------------------------------------
@@ -1416,7 +1433,7 @@ void W3DRadar::draw( Int pixelX, Int pixelY, Int width, Int height )
 
 		// rebuild the object overlay
 		renderObjectList( getObjectList(), m_overlayTexture );
-		renderObjectList( getLocalObjectList(), m_overlayTexture, TRUE );
+		renderObjectList( getLocalObjectList(), m_overlayTexture );
 
 	}  // end if
 
@@ -1475,7 +1492,7 @@ void W3DRadar::refreshTerrain( TerrainLogic *terrain )
 
 /*
  *
-	void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *texture, Bool calcHero )
+	void W3DRadar::renderObjectList( const RadarObject *listHead, TextureClass *texture )
 {
 
 	// sanity
@@ -1495,12 +1512,6 @@ void W3DRadar::refreshTerrain( TerrainLogic *terrain )
 
 	UnsignedByte minAlpha = 8;
 
-	if( calcHero )
-	{
-		// clear all entries from the cached hero object list
-		m_cachedHeroPosList.clear();
-	}
-
 	for( const RadarObject *rObj = listHead; rObj; rObj = rObj->friend_getNext() )
 	{
     UnsignedByte h = (UnsignedByte)(rObj->isTemporarilyHidden());
@@ -1513,12 +1524,6 @@ void W3DRadar::refreshTerrain( TerrainLogic *terrain )
 		const Object *obj = rObj->friend_getObject();
 		UnsignedByte r = 1;   // all decoys
 
-		// cache hero object positions for drawing in icon layer
-		if( calcHero && obj->isHero() )
-		{
-			m_cachedHeroPosList.push_back(obj->getPosition());
-		}
-
 		// get the color we're going to draw in
 		UnsignedInt c = 0xfe000000;// this is a decoy
     c |= (UnsignedInt)( obj->testStatus( OBJECT_STATUS_STEALTHED ) );//so is this