Skip to content

[GEN][ZH] Fix list element erasure with iterator on unrelated container in ResourceGatheringManager::findBestSupplyCenter, MilesAudioManager::killLowestPrioritySoundImmediately #631

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

Merged
xezon merged 2 commits into from
Apr 9, 2025
Merged

[GEN][ZH] Fix list element erasure with iterator on unrelated container in ResourceGatheringManager::findBestSupplyCenter, MilesAudioManager::killLowestPrioritySoundImmediately #631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
073f1f8
[ZH] Fix incorrect containers being used in iterator loops
Mauller Apr 8, 2025
3a808cb
[GEN] Fix incorrect containers being used in iterator loops
Mauller Apr 8, 2025
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: 2 additions & 2 deletions Generals/Code/GameEngine/Source/Common/RTS/ResourceGatheringManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ Object *ResourceGatheringManager::findBestSupplyCenter( Object *queryObject )
if( dock )
{
static const NameKeyType key_centerUpdate = NAMEKEY("SupplyCenterDockUpdate");
SupplyWarehouseDockUpdate *centerModule = (SupplyWarehouseDockUpdate*)dock->findUpdateModule( key_centerUpdate );
SupplyCenterDockUpdate *centerModule = (SupplyCenterDockUpdate*)dock->findUpdateModule( key_centerUpdate );
//If remotely okay, let User win.
if( centerModule && computeRelativeCost( queryObject, dock, NULL ) != FLT_MAX )
return dock;
Expand All @@ -234,7 +234,7 @@ Object *ResourceGatheringManager::findBestSupplyCenter( Object *queryObject )

if( currentCenter == NULL )
{
iterator = m_supplyWarehouses.erase( iterator );
iterator = m_supplyCenters.erase( iterator );
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion Generals/Code/GameEngine/Source/Common/System/SubsystemInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ void SubsystemInterfaceList::addSubsystem(SubsystemInterface* sys)
void SubsystemInterfaceList::removeSubsystem(SubsystemInterface* sys)
{
#ifdef DUMP_PERF_STATS
for (SubsystemList::iterator it = m_allSubsystems.begin(); it != m_subsystems.end(); ++it)
for (SubsystemList::iterator it = m_allSubsystems.begin(); it != m_allSubsystems.end(); ++it)
{
if ( (*it) == sys) {
m_allSubsystems.erase(it);
Expand Down
4 changes: 2 additions & 2 deletions Generals/Code/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2085,9 +2085,9 @@ Bool MilesAudioManager::killLowestPrioritySoundImmediately( AudioEventRTS *event

if( playing->m_audioEventRTS && playing->m_audioEventRTS == lowestPriorityEvent )
{
//Release this 3D sound channel immediately because we are going to play another sound in it's place.
//Release this sound channel immediately because we are going to play another sound in it's place.
releasePlayingAudio( playing );
m_playing3DSounds.erase( it );
m_playingSounds.erase( it );
return TRUE;
}
}
Expand Down
4 changes: 2 additions & 2 deletions GeneralsMD/Code/GameEngine/Source/Common/RTS/ResourceGatheringManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ Object *ResourceGatheringManager::findBestSupplyCenter( Object *queryObject )
if( dock )
{
static const NameKeyType key_centerUpdate = NAMEKEY("SupplyCenterDockUpdate");
SupplyWarehouseDockUpdate *centerModule = (SupplyWarehouseDockUpdate*)dock->findUpdateModule( key_centerUpdate );
SupplyCenterDockUpdate *centerModule = (SupplyCenterDockUpdate*)dock->findUpdateModule( key_centerUpdate );

@xezon xezon Apr 8, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This certainly was no runtime bug, because this pointer is just tested for null.

//If remotely okay, let User win.
if( centerModule && computeRelativeCost( queryObject, dock, NULL ) != FLT_MAX )
return dock;
Expand All @@ -234,7 +234,7 @@ Object *ResourceGatheringManager::findBestSupplyCenter( Object *queryObject )

if( currentCenter == NULL )
{
iterator = m_supplyWarehouses.erase( iterator );
iterator = m_supplyCenters.erase( iterator );
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion GeneralsMD/Code/GameEngine/Source/Common/System/SubsystemInterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ void SubsystemInterfaceList::addSubsystem(SubsystemInterface* sys)
void SubsystemInterfaceList::removeSubsystem(SubsystemInterface* sys)
{
#ifdef DUMP_PERF_STATS
for (SubsystemList::iterator it = m_allSubsystems.begin(); it != m_subsystems.end(); ++it)
for (SubsystemList::iterator it = m_allSubsystems.begin(); it != m_allSubsystems.end(); ++it)

@xezon xezon Apr 8, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a debug bug fix. No concern for retail game.

{
if ( (*it) == sys) {
m_allSubsystems.erase(it);
Expand Down
4 changes: 2 additions & 2 deletions GeneralsMD/Code/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2087,9 +2087,9 @@ Bool MilesAudioManager::killLowestPrioritySoundImmediately( AudioEventRTS *event

if( playing->m_audioEventRTS && playing->m_audioEventRTS == lowestPriorityEvent )
{
//Release this 3D sound channel immediately because we are going to play another sound in it's place.
//Release this sound channel immediately because we are going to play another sound in it's place.
releasePlayingAudio( playing );
m_playing3DSounds.erase( it );
m_playingSounds.erase( it );

@xezon xezon Apr 8, 2025

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 99% sure that this just worked with no bugs with stlport's implementation.

  iterator erase(iterator __position) {
    _List_node_base* __next_node = __position._M_node->_M_next; // accesses next node
    _List_node_base* __prev_node = __position._M_node->_M_prev; // accesses prev node
    _Node* __n = (_Node*) __position._M_node; // accesses current node
    __prev_node->_M_next = __next_node; // writes prev node
    __next_node->_M_prev = __prev_node; // writes next node
    _Destroy(&__n->_M_data); // calls destructor with the same type (PlayingAudio*)
    this->_M_node.deallocate(__n, 1); // calls deallocate on default allocator for std::list
    return iterator((_Node*)__next_node); // return local
    }

Same with m_supplyCenters.erase( iterator )

So these were no runtime issues in the retail game.

@Mauller Mauller Apr 8, 2025

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.

That is funny that it essentially looks like it does not care what container it is called on, just that erase is called with an iterator.

return TRUE;
}
}
Expand Down