From ee9b9c46f2a2157ca3bd79a9697e0ed2afe536d7 Mon Sep 17 00:00:00 2001 From: "seer-by-sentry[bot]" <157164994+seer-by-sentry[bot]@users.noreply.github.com> Date: Mon, 2 Mar 2026 01:32:15 +0000 Subject: [PATCH] Fix: Prevent crashes during drawable iteration when drawables are destroyed --- .../Source/GameClient/GameClient.cpp | 25 +++++++++++++++---- .../Source/GameClient/GameClient.cpp | 25 +++++++++++++++---- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/Generals/Code/GameEngine/Source/GameClient/GameClient.cpp b/Generals/Code/GameEngine/Source/GameClient/GameClient.cpp index dd1288e23a0..023cc4c862c 100644 --- a/Generals/Code/GameEngine/Source/GameClient/GameClient.cpp +++ b/Generals/Code/GameEngine/Source/GameClient/GameClient.cpp @@ -757,17 +757,32 @@ void GameClient::updateHeadless() */ void GameClient::iterateDrawablesInRegion( Region3D *region, GameClientFuncPtr userFunc, void *userData ) { - Drawable *draw, *nextDrawable; - - for( draw = m_drawableList; draw; draw=nextDrawable ) + // TheSuperHackers @bugfix Collect matching drawable IDs upfront before invoking any callbacks. + // The userFunc (e.g. drawablePostDraw) can indirectly call getShroudedStatus(), which may + // trigger ghost-object snapshot operations that destroy drawables and remove them from + // m_drawableList. Pre-fetching nextDrawable before the call is not sufficient because the + // pre-fetched pointer itself can become dangling if that drawable is also destroyed during + // the callback. By collecting IDs first and then validating each via findDrawableByID(), + // we ensure we never dereference a freed drawable pointer. + static std::vector s_drawableIDs; + s_drawableIDs.clear(); + + for( Drawable *draw = m_drawableList; draw; draw = draw->getNextDrawable() ) { - nextDrawable = draw->getNextDrawable(); - Coord3D pos = *draw->getPosition(); if( region == nullptr || (pos.x >= region->lo.x && pos.x <= region->hi.x && pos.y >= region->lo.y && pos.y <= region->hi.y && pos.z >= region->lo.z && pos.z <= region->hi.z) ) + { + s_drawableIDs.push_back( draw->getID() ); + } + } + + for( Int i = 0; i < (Int)s_drawableIDs.size(); ++i ) + { + Drawable *draw = findDrawableByID( s_drawableIDs[i] ); + if( draw != nullptr ) { (*userFunc)( draw, userData ); } diff --git a/GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp b/GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp index d37a4295687..00c8c275f39 100644 --- a/GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp +++ b/GeneralsMD/Code/GameEngine/Source/GameClient/GameClient.cpp @@ -825,17 +825,32 @@ void GameClient::updateHeadless() */ void GameClient::iterateDrawablesInRegion( Region3D *region, GameClientFuncPtr userFunc, void *userData ) { - Drawable *draw, *nextDrawable; - - for( draw = m_drawableList; draw; draw=nextDrawable ) + // TheSuperHackers @bugfix Collect matching drawable IDs upfront before invoking any callbacks. + // The userFunc (e.g. drawablePostDraw) can indirectly call getShroudedStatus(), which may + // trigger ghost-object snapshot operations that destroy drawables and remove them from + // m_drawableList. Pre-fetching nextDrawable before the call is not sufficient because the + // pre-fetched pointer itself can become dangling if that drawable is also destroyed during + // the callback. By collecting IDs first and then validating each via findDrawableByID(), + // we ensure we never dereference a freed drawable pointer. + static std::vector s_drawableIDs; + s_drawableIDs.clear(); + + for( Drawable *draw = m_drawableList; draw; draw = draw->getNextDrawable() ) { - nextDrawable = draw->getNextDrawable(); - Coord3D pos = *draw->getPosition(); if( region == nullptr || (pos.x >= region->lo.x && pos.x <= region->hi.x && pos.y >= region->lo.y && pos.y <= region->hi.y && pos.z >= region->lo.z && pos.z <= region->hi.z) ) + { + s_drawableIDs.push_back( draw->getID() ); + } + } + + for( Int i = 0; i < (Int)s_drawableIDs.size(); ++i ) + { + Drawable *draw = findDrawableByID( s_drawableIDs[i] ); + if( draw != nullptr ) { (*userFunc)( draw, userData ); }