Crash when marooned generals die

[morganchristiansson]

12 enemy generals got trapped on an island when the capture of my guardhouse cut the road connecting the island to the port they landed on.

I was quite happy with this cause they'd all die, but the game crashes when they die.

Thread 1 "s25client" received signal SIGTRAP, Trace/breakpoint trap.
0x00005555557328fe in nofSoldier::Destroy() ()
(gdb) bt
#0  0x00005555557328fe in nofSoldier::Destroy() ()
#1  0x0000555555732991 in nofActiveSoldier::Destroy() ()
#2  0x00005555557374c8 in nofAttacker::Destroy() ()
#3  0x000055555594fffb in EventManager::DestroyCurrentObjects() ()
#4  0x000055555594ff6b in EventManager::ExecuteNextGF() ()
#5  0x00005555559589f2 in Game::RunGF() ()
#6  0x000055555585a1f6 in GameClient::NextGF(bool) ()
#7  0x00005555558596f5 in GameClient::ExecuteGameFrame() ()
#8  0x0000555555852fa2 in GameClient::Run() ()
#9  0x00005555555f98c7 in GameManager::Run() ()
#10 0x00005555555d5c2a in (anonymous namespace)::RunProgram(boost::program_options::variables_map&) ()
#11 0x00005555555d6421 in main ()

savefile pre-crash: crash.sav.gz

edit: Playing Erics campaign mission 5.

[morganchristiansson]

AI Root Cause Analysis

The crash occurs because noFigure::Die() does not call Abrogate() before removing the figure from the world and adding it to the kill list. For soldiers, this means pointers like homeBld, attacked_goal, huntingDefender, enemy, and ship_obj_id remain set when Destroy() is later called from the kill list, causing the RTTR_Assert failures (which raise SIGTRAP).

The specific scenario you hit:

1. Enemy sea attackers (nofAttacker) landed on an island via ship (ship_obj_id set)
2. The road back to the harbor/ship was cut when the guardhouse was captured
3. The attackers try to return home via HandleState_SeaAttack_ReturnToShip()
4. No path back to the ship → it calls StartWandering() then Wander() before cleaning up:
   • homeBld->SoldierLost(this) is called but homeBld is NOT set to nullptr
   • CancelAtShip() is called after Wander() (which may call Die() immediately)
5. Wander() → Die() → adds to kill list → Destroy() → assert fails on !GetHomeBld()

Additionally, noFigure::Die() lacks any cleanup call, making any path that leads to soldiers dying from wandering (without prior Abrogate()) crash.

Proposed Fixes

1. noFigure::Die() — Call Abrogate() before removing the figure, ensuring goal_ and homeBld (via AbrogateWorkplace()) are cleaned up as a safety net for all paths.
2. nofAttacker::HandleState_SeaAttack_ReturnToShip() — Reorder cleanup to happen before StartWandering()/Wander():
   • In the !homeBld branch: call CancelAtShip() and InformTargetsAboutCancelling() before StartWandering()
   • In the "no path" branch: notify home and set homeBld = nullptr, call CancelAtShip() and InformTargetsAboutCancelling() before StartWandering()
   • In the "ship not found" branch: call InformTargetsAboutCancelling() before StartWandering()
3. nofAttacker::HomeDestroyed() (else branch) — Call InformTargetsAboutCancelling() and CancelAtShip() to clear attacked_goal, huntingDefender, enemy, and ship_obj_id when the home is destroyed while the soldier is not in a
   directly-handled state.

[morganchristiansson]

I pushed back on the AIs heavy handed multi layered fix and got this smaller fix:

diff --git a/libs/s25main/figures/nofAttacker.cpp b/libs/s25main/figures/nofAttacker.cpp
index 987b13bc1..32108d089 100644
--- a/libs/s25main/figures/nofAttacker.cpp
+++ b/libs/s25main/figures/nofAttacker.cpp
@@ -886,11 +886,10 @@ void nofAttacker::HandleState_SeaAttack_ReturnToShip()
     if(!homeBld)
     {
         // Home destroyed -> start wandering
+        CancelAtShip();
         state = SoldierState::FigureWork;
         StartWandering();
         Wander();
-
-        CancelAtShip();
     } else if(pos == shipPos) // Arrived at ship
     {
         for(noBase& figure : world->GetFigures(pos))
@@ -916,13 +915,12 @@ void nofAttacker::HandleState_SeaAttack_ReturnToShip()
         else
         {
             // No path -> wander around
-            StartWandering();
+            // Notify home and ship first, then wander
+            AbrogateWorkplace();
+            CancelAtShip();
             state = SoldierState::FigureWork;
+            StartWandering();
             Wander();
-
-            // Notify home and ship
-            homeBld->SoldierLost(this);
-            CancelAtShip();
         }
     }
 }

Which fixes the crash.

However with my guardhouse captured I can no longer connect the island by road and the game soft-locked.

[Flamefire]

Can you attach a screenshot of the situation? It sounds like a similar situation we had before: The only way to cross a 1-node river was by using a road and hence destroying that road disconnects the land masses. Is that the case?

The fix looks correct. homeBld->SoldierLost(this); without resetting homeBld looks like an oversight, AbrogateWorkplace does that.
So I'd accept that as a PR from you if you want or add one myself. Maybe combine the 2 comments like "No path -> Notify home and ship, and wander around".

Best to add a test case for that first, e.g. based on that for the above case: FlagBecomesUnreachableForWaitingAttacker

However with my guardhouse captured I can no longer connect the island by road and the game soft-locked.

Unfortunate but (pretty much) by design.

[morganchristiansson]

Is that the case?

Yes

Screenshot from attached crash.sav.gz:

[Flamefire]

Ok, thanks. I assume there was a road on the ice floes between the center island and the one at the bottom?

[morganchristiansson]

Yes there were roads connecting up/left with middle and then bottom which enabled me to destroy the harbour where the ships are. Then got pummelled by waves of generals from floating fortresses.

Took a while before I realized I need road to allow soldiers/hunters/lumberjacks to cross. Not sure I like the mechanic.

[Flamefire]

I've checked your save game and do not see any crash or assertion failure.
Checking the code I see how that could happen but it doesn't show in the provided savegame.

homeBld->SoldierLost(this) should be replaced by AbrogateWorkplace but I don't see any invalid state in the savegame.
Did you attach the wrong save?
Do you still have the replay?

Note that the AI analysis is also wrong in CancelAtShip() is called after Wander() (which may call Die() immediately). This cannot happen, unless the soldier would happen to be on a node where there is literally no way. But even then the rest of the code would still execute before nofSoldier::Destroy is called, hence the reordering doesn't change anything.