Skip to content

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

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.

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
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion Core/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
# c stands for core, i stands for Interface
# i stands for Interface
add_library(corei_gameengine_include INTERFACE)
add_library(corei_libraries_include INTERFACE)
add_library(corei_libraries_source_wwvegas INTERFACE)
add_library(corei_libraries_source_wwvegas_wwdebug INTERFACE)
add_library(corei_libraries_source_wwvegas_wwlib INTERFACE)
add_library(corei_always INTERFACE)

target_include_directories(corei_gameengine_include INTERFACE "GameEngine/Include")
target_include_directories(corei_libraries_include INTERFACE "Libraries/Include")
target_include_directories(corei_libraries_source_wwvegas INTERFACE "Libraries/Source/WWVegas")
target_include_directories(corei_libraries_source_wwvegas_wwdebug INTERFACE "Libraries/Source/WWVegas/WWDebug")
Expand Down
1 change: 1 addition & 0 deletions Core/GameEngine/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,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
Expand Down
61 changes: 61 additions & 0 deletions Core/GameEngine/Include/Common/STLUtils.h
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions GeneralsMD/Code/GameEngine/Include/Common/GameEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 2 additions & 3 deletions GeneralsMD/Code/GameEngine/Include/Common/Radar.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/GameEngine/Include/Common/STLTypedefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ class STLSpecialAlloc;
#include "Common/UnicodeString.h"
#include "Common/GameCommon.h"
#include "Common/GameMemory.h"
#include "Common/STLUtils.h"

//-----------------------------------------------------------------------------

Expand Down
15 changes: 13 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/Common/GameEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,8 @@ void GameEngine::init( int argc, char *argv[] )
initDisabledMasks();
initDamageTypeFlags();

TheSubsystemList->resetAll();
resetSubsystems();

HideControlBar();
} // end init

Expand All @@ -703,7 +704,7 @@ void GameEngine::reset( void )
if (TheGameLogic->isInMultiplayerGame())
deleteNetwork = true;

TheSubsystemList->resetAll();
resetSubsystems();

if (deleteNetwork)
{
Expand All @@ -720,6 +721,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();
Copy link
Author

Choose a reason for hiding this comment

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

With this change, this resetAll crashed on map unload in

>	generalszh.exe!TunnelContain::iterateContained(void(*)(Object *, void *) func, void * userData, bool reverse) Line 191	C++
 	generalszh.exe!Object::isHero() Line 2044	C++
 	generalszh.exe!W3DRadar::addObject(Object * obj) Line 1003	C++
 	generalszh.exe!Player::becomingLocalPlayer(bool yes) Line 1138	C++
 	[Inline Frame] generalszh.exe!PlayerList::setLocalPlayer(Player *) Line 324	C++
 	generalszh.exe!PlayerList::init() Line 246	C++
 	generalszh.exe!PlayerList::reset() Line 127	C++
 	generalszh.exe!SubsystemInterfaceList::resetAll() Line 188	C++
 	[Inline Frame] generalszh.exe!GameLogic::isInMultiplayerGame() Line 418	C++
 	generalszh.exe!GameEngine::reset() Line 703	C++
 	generalszh.exe!GameLogic::clearGameData(bool showScoreScreen) Line 281	C++
 	generalszh.exe!GameLogic::logicMessageDispatcher(GameMessage * msg, void * userData) Line 464	C++
 	generalszh.exe!GameLogic::processCommandList(CommandList * list) Line 2597	C++
 	generalszh.exe!GameLogic::update() Line 3767	C++
 	[Inline Frame] generalszh.exe!SubsystemInterface::UPDATE() Line 134	C++
 	generalszh.exe!GameEngine::update() Line 766	C++
 	generalszh.exe!Win32GameEngine::update() Line 93	C++
 	generalszh.exe!GameEngine::execute() Line 823	C++
 	generalszh.exe!GameMain(int argc, char * * argv) Line 47	C++
 	generalszh.exe!WinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, char * lpCmdLine, int nCmdShow) Line 988	C++
 	[Inline Frame] generalszh.exe!invoke_main() Line 102	C++
 	generalszh.exe!__scrt_common_main_seh() Line 288	C++
 	kernel32.dll!75e3fcc9()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!76f582ae()	Unknown
 	ntdll.dll!76f5827e()	Unknown
Exception thrown: read access violation.
**owningPlayer** was 0x150.

The problem is that the Object's TunnelContain module has a dependency on the player, but the player was already purged from the PlayerList. And so this would crash.

Resetting GameLogic first avoids this (and potentially similar issues in the future). Alternatively we could add more NULL checks in TunnelContain, but I think that is not necessary if Subsystem just have a proper reset sequence.

}

/// -----------------------------------------------------------------------------------------------
DECLARE_PERF_TIMER(GameEngine_update)

Expand Down
15 changes: 8 additions & 7 deletions GeneralsMD/Code/GameEngine/Source/Common/System/Radar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -397,13 +397,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;
Expand Down Expand Up @@ -547,6 +547,7 @@ void Radar::addObject( Object *obj )

} // end else

return true;
} // end addObject

//-------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -593,24 +594,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

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

I changed the container type here because list of coordinate pointers is not particularly great.

Copy link

@Caball009 Caball009 Jun 9, 2025

Choose a reason for hiding this comment

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

Wouldn't it be better to change to std::vector<Coord2D>? As far as I can tell, the z coordinate is irrelevant for the radar and the hero coordinates are updated each frame, so using a pointer is not an advantage when the size of 2D coordinates is 2 * sizeof(float).

Changes:
void W3DRadar::drawHeroIcon( Int pixelX, Int pixelY, Int width, Int height, const Coord3D *pos )
->
void W3DRadar::drawHeroIcon( Int pixelX, Int pixelY, Int width, Int height, Coord2D pos )

m_cachedHeroPosList.push_back(obj->getPosition());
->
m_cachedHeroPosList.push_back(Coord2D(obj->getPosition()->x, obj->getPosition()->y));

Copy link
Author

Choose a reason for hiding this comment

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

The position changes when the Hero moves. Putting a fixed position into the vector would paint the radar icon at a static position.

Choose a reason for hiding this comment

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

I don't think it would. I just tried it and the position is not fixed on the radar. It wouldn't be if the position is updated each frame; i.e. if W3DRadar::renderObjectList is called for each frame.

Copy link
Author

Choose a reason for hiding this comment

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

I suspect you test with calcHero code? The calcHero rebuilds the vector every now and then. This change does not.

Choose a reason for hiding this comment

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

Right, sorry.

};


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -606,18 +606,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
Expand All @@ -635,12 +635,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() )
{

Expand All @@ -650,11 +644,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
Expand Down Expand Up @@ -995,6 +984,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 */
//-------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -1404,7 +1421,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

Expand Down Expand Up @@ -1463,7 +1480,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
Expand All @@ -1483,12 +1500,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());
Expand All @@ -1501,12 +1512,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ add_library(z_wwdownload STATIC)
set_target_properties(z_wwdownload PROPERTIES OUTPUT_NAME wwdownload)

target_link_libraries(z_wwdownload PRIVATE
corei_gameengine_include
corei_wwdownload
zi_always
zi_gameengine_include
Expand Down
1 change: 1 addition & 0 deletions GeneralsMD/Code/Tools/ParticleEditor/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ target_include_directories(z_particleeditor PRIVATE
target_link_libraries(z_particleeditor PRIVATE
core_debug
core_profile
corei_gameengine_include
corei_libraries_source_wwvegas
corei_libraries_source_wwvegas_wwlib
d3d8lib
Expand Down
2 changes: 0 additions & 2 deletions GeneralsMD/Code/Tools/WorldBuilder/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,6 @@ target_link_libraries(z_worldbuilder PRIVATE
core_browserdispatch
z_gameengine
z_gameenginedevice
zi_gameengine_include
zi_gameenginedevice_include
zi_always
)

Expand Down
Loading