Fix waiting attackers becoming stuck when target flag becomes unreachable.

When the defender kills the current attacker (at the flag)
`FindAttackerNearBuilding` is used to get the next one for the defender
to fight. This takes the closest attacker that is ready, i.e. waiting
around the flag and calls `AttackDefenderAtFlag` which usually causes
the attacker to start walking to the flag. However if there is no path
to the flag (anymore, e.g. after destroying a road) the attacker won't
do anything and the function returns `false`.

As this isn't checked the defender will keep waiting for it.

This can lead to use-after-free if the attacker is destroyed (defeated
in free fight, went back home, ...) as the defender will then have a
dangling pointer to it.

Fixes #1863

Author: Flamefire

libs/s25main/buildings/nobBaseMilitary.cpp

@@ -165,29 +165,28 @@ nofAttacker* nobBaseMilitary::FindAggressor(nofAggressiveDefender& defender)
 
 MapPoint nobBaseMilitary::FindAnAttackerPlace(unsigned short& ret_radius, const nofAttacker& soldier)
 {
-    const MapPoint flagPos = world->GetNeighbour(pos, Direction::SouthEast);
+    const MapPoint flagPos = GetFlagPos();
+    const MapPoint soldierPos = soldier.GetPos();
 
-    // Diesen Flaggenplatz nur nehmen, wenn es auch nich gerade eingenommen wird, sonst gibts Deserteure!
-    // Eigenommen werden können natürlich nur richtige Militärgebäude
-    bool capturing =
-      (BuildingProperties::IsMilitary(bldType_)) ? (static_cast<nobMilitary*>(this)->IsBeingCaptured()) : false;
+    // Only consider  the flag position for fighting if the building isn't currently being captured.
+    // Only "real" military buildings can be captured
+    const bool capturing =
+      BuildingProperties::IsMilitary(bldType_) && static_cast<nobMilitary*>(this)->IsBeingCaptured();
 
-    if(!capturing && world->IsValidPointForFighting(flagPos, soldier, false))
+    // Also check if we can reach this.
+    // If not, still consider the other points as the flag could become reachable by then.
+    if(!capturing && world->IsValidPointForFighting(flagPos, soldier, false)
+       && (soldierPos == flagPos || world->FindHumanPath(soldierPos, flagPos) != boost::none))
     {
         ret_radius = 0;
         return flagPos;
     }
 
-    const MapPoint soldierPos = soldier.GetPos();
-    // Get points AROUND the flag. Never AT the flag
-    const auto nodes = world->GetPointsInRadius(flagPos, 3, ReturnMapPointWithRadius{});
-
-    // Weg zu allen möglichen Punkten berechnen und den mit den kürzesten Weg nehmen
-    // Die bisher kürzeste gefundene Länge
+    // Check all points around the flag and take shortest
     unsigned min_length = std::numeric_limits<unsigned>::max();
     MapPoint minPt = MapPoint::Invalid();
     ret_radius = 100;
-    for(const auto& node : nodes)
+    for(const auto& node : world->GetPointsInRadius(flagPos, 3, ReturnMapPointWithRadius{}))
     {
         // We found a point with a better radius
         if(node.second > ret_radius)
@@ -196,18 +195,18 @@ MapPoint nobBaseMilitary::FindAnAttackerPlace(unsigned short& ret_radius, const
         if(!world->ValidWaitingAroundBuildingPoint(node.first, pos))
             continue;
 
-        // Derselbe Punkt? Dann können wir gleich abbrechen, finden ja sowieso keinen kürzeren Weg mehr
+        // If this is where the soldier currently is, it is already the shortest possible distance
         if(soldierPos == node.first)
         {
             ret_radius = node.second;
             return node.first;
         }
 
         unsigned length = 0;
-        // Gültiger Weg gefunden
+        // Is there a path at all?
         if(world->FindHumanPath(soldierPos, node.first, 100, false, &length))
         {
-            // Kürzer als bisher kürzester Weg? --> Dann nehmen wir diesen Punkt (vorerst)
+            // Take if shorter
             if(length < min_length)
             {
                 minPt = node.first;
@@ -252,30 +251,30 @@ bool nobBaseMilitary::CallDefender(nofAttacker& attacker)
 
 nofAttacker* nobBaseMilitary::FindAttackerNearBuilding()
 {
-    // Alle angreifenden Soldaten durchgehen
-    // Den Soldaten, der am nächsten dran steht, nehmen
     nofAttacker* best_attacker = nullptr;
-    unsigned best_radius = 0xFFFFFFFF;
-
-    for(auto* aggressor : aggressors)
+    do
     {
-        // Ist der Soldat überhaupt bereit zum Kämpfen (also wartet er um die Flagge herum oder rückt er nach)?
-        if(aggressor->IsAttackerReady())
+        // Call the closest attacker to fight the defender
+        best_attacker = nullptr;
+        unsigned best_radius = 0xFFFFFFFF;
+        for(auto* aggressor : aggressors)
         {
-            // Besser als bisher bester?
-            if(aggressor->GetRadius() < best_radius || !best_attacker)
+            // Only consider those waiting around the flag, otherwise the defender could go/stay inside
+            if(aggressor->IsAttackerReady())
             {
-                best_attacker = aggressor;
-                best_radius = best_attacker->GetRadius();
+                if(!best_attacker || aggressor->GetRadius() < best_radius)
+                {
+                    best_attacker = aggressor;
+                    best_radius = aggressor->GetRadius();
+                }
             }
         }
-    }
-
-    if(best_attacker)
-        best_attacker->AttackDefenderAtFlag();
-
-    // und ihn zurückgeben, wenns keine gibt, natürlich 0
-    return best_attacker;
+        // Return the attacker if found and he starts attacking, else try again
+        // The current one will be removed from aggressors or not be ready again
+        if(best_attacker && best_attacker->AttackDefenderAtFlag())
+            return best_attacker;
+    } while(best_attacker); // Stop if we didn't found any attacker at all
+    return nullptr;
 }
 
 void nobBaseMilitary::CheckArrestedAttackers()

libs/s25main/figures/nofAttacker.cpp

@@ -605,18 +605,23 @@ bool nofAttacker::AttackDefenderAtFlag()
 {
     // Walk to flag if possible
     const auto dir = world->FindHumanPath(pos, attacked_goal->GetFlagPos(), 3, true);
-    if(!dir)
-        return false;
-
-    const bool waiting_around_building = (state == SoldierState::AttackingWaitingAroundBuilding);
-    state = SoldierState::AttackingAttackingFlag;
+    if(dir)
+    {
+        const bool waiting_around_building = (state == SoldierState::AttackingWaitingAroundBuilding);
+        state = SoldierState::AttackingAttackingFlag;
 
-    if(waiting_around_building)
+        if(waiting_around_building)
+        {
+            StartWalking(*dir);
+            attacked_goal->SendSuccessor(pos, radius);
+        }
+        return true;
+    } else
     {
-        StartWalking(*dir);
-        attacked_goal->SendSuccessor(pos, radius);
+        state = nofActiveSoldier::SoldierState::AttackingWalkingToGoal;
+        MissAttackingWalk();
+        return false;
     }
-    return true;
 }
 
 void nofAttacker::AttackFlag()