Skip to content

Commit da89598

Browse files
authored
MovableMan - fix usage of unordered_set::erase
`unordered_set::erase` invalidates the pointer that was erased. https://en.cppreference.com/w/cpp/container/unordered_set/erase.html > References and iterators to the erased elements are invalidated. Other iterators and references are not invalidated. I also tested with a fresh C++ code file with `-fsanitize-address` and, indeed, the pattern of code of using an iterator variable to erase and then accessing through it reliably explodes. --- What prompted this is that I have just had a completely random segfault crash originating from `SDL_Scancode ()` (????????) while playing the game. Here's the gdb backtrace (yes i play this game in debug build in gdb at this point lmao): ``` (gdb) bt #0 0x0000555556b39558 in typeinfo name for SDL_Scancode () cortex-command-community#1 0x0000555555ae47aa in RTE::MovableMan::Update (this=0x555557453f50) at ../Source/Managers/MovableMan.cpp:1618 cortex-command-community#2 0x000055555567597c in RunGameLoop () at ../SourceMain.cpp:354 cortex-command-community#3 0x000055555567658a in main (argc=1 argv=0x7fffffffdc88) at ../Source/Main.cpp:468 ``` The last sensible location in that backtrace is right after the erase. Nothing seems to be broken with this applied. --- There are comments and code around these spots that this commit touches that are apparently to work around the same issue I experienced. They're probably redundant/unneeded any more lol
1 parent 07bb70b commit da89598

File tree

1 file changed

+9
-9
lines changed

1 file changed

+9
-9
lines changed

Source/Managers/MovableMan.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1484,8 +1484,6 @@ void MovableMan::Update() {
14841484
if (!(*aIt)->IsSetToDelete())
14851485
m_Actors.push_back(*aIt);
14861486
else {
1487-
m_ValidActors.erase(*aIt);
1488-
14891487
// Also remove actor from the roster
14901488
if ((*aIt)->GetTeam() >= 0) {
14911489
// m_ActorRoster[(*aIt)->GetTeam()].remove(*aIt);
@@ -1494,6 +1492,8 @@ void MovableMan::Update() {
14941492

14951493
(*aIt)->DestroyScriptState();
14961494
delete (*aIt);
1495+
1496+
m_ValidActors.erase(*aIt);
14971497
}
14981498
}
14991499
m_AddedActors.clear();
@@ -1504,9 +1504,9 @@ void MovableMan::Update() {
15041504
if (!(*iIt)->IsSetToDelete()) {
15051505
m_Items.push_back(*iIt);
15061506
} else {
1507-
m_ValidItems.erase(*iIt);
15081507
(*iIt)->DestroyScriptState();
15091508
delete (*iIt);
1509+
m_ValidItems.erase(*iIt);
15101510
}
15111511
}
15121512
m_AddedItems.clear();
@@ -1517,9 +1517,9 @@ void MovableMan::Update() {
15171517
if (!(*parIt)->IsSetToDelete()) {
15181518
m_Particles.push_back(*parIt);
15191519
} else {
1520-
m_ValidParticles.erase(*parIt);
15211520
(*parIt)->DestroyScriptState();
15221521
delete (*parIt);
1522+
m_ValidParticles.erase(*parIt);
15231523
}
15241524
}
15251525
m_AddedParticles.clear();
@@ -1571,8 +1571,8 @@ void MovableMan::Update() {
15711571
if ((*iIt)->GetRestThreshold() < 0) {
15721572
(*iIt)->SetRestThreshold(500);
15731573
}
1574-
m_ValidItems.erase(*iIt);
15751574
m_Particles.push_back(*iIt);
1575+
m_ValidItems.erase(*iIt);
15761576
iIt++;
15771577
}
15781578
m_Items.erase(imidIt, m_Items.end());
@@ -1600,9 +1600,9 @@ void MovableMan::Update() {
16001600
RemoveActorFromTeamRoster(*aIt);
16011601

16021602
// Delete
1603-
m_ValidActors.erase(*aIt);
16041603
(*aIt)->DestroyScriptState();
16051604
delete (*aIt);
1605+
m_ValidActors.erase(*aIt);
16061606
aIt++;
16071607
}
16081608
// Try to set the existing iterator to a safer value, erase can crash in debug mode otherwise?
@@ -1614,9 +1614,9 @@ void MovableMan::Update() {
16141614
imidIt = iIt;
16151615

16161616
while (iIt != m_Items.end()) {
1617-
m_ValidItems.erase(*iIt);
16181617
(*iIt)->DestroyScriptState();
16191618
delete (*iIt);
1619+
m_ValidItems.erase(*iIt);
16201620
iIt++;
16211621
}
16221622
m_Items.erase(imidIt, m_Items.end());
@@ -1626,9 +1626,9 @@ void MovableMan::Update() {
16261626
midIt = parIt;
16271627

16281628
while (parIt != m_Particles.end()) {
1629-
m_ValidParticles.erase(*parIt);
16301629
(*parIt)->DestroyScriptState();
16311630
delete (*parIt);
1631+
m_ValidParticles.erase(*parIt);
16321632
parIt++;
16331633
}
16341634
m_Particles.erase(midIt, m_Particles.end());
@@ -1658,9 +1658,9 @@ void MovableMan::Update() {
16581658
if ((*parIt)->GetDrawPriority() >= terrMat->GetPriority()) {
16591659
(*parIt)->DrawToTerrain(g_SceneMan.GetTerrain());
16601660
}
1661-
m_ValidParticles.erase(*parIt);
16621661
(*parIt)->DestroyScriptState();
16631662
delete (*parIt);
1663+
m_ValidParticles.erase(*parIt);
16641664
parIt++;
16651665
}
16661666
m_Particles.erase(midIt, m_Particles.end());

0 commit comments

Comments
 (0)