From 54725b2279261d95b7155823ca513b12fe04e739 Mon Sep 17 00:00:00 2001
From: Louis Moureaux <m_louis30@yahoo.com>
Date: Tue, 10 Sep 2024 23:13:51 +0200
Subject: [PATCH] Remove dangling city pointers from the governor code

The governor code was using pointers to keep track of cities to be
removed or updated. These pointers would sometimes become invalid when
the city was lost or otherwise conquered, causing use-after-free errors.

This fixes the first version of #2351.
---
 client/governor.cpp | 49 ++++++++++++++++-----------------------------
 client/governor.h   |  7 ++++---
 2 files changed, 21 insertions(+), 35 deletions(-)

diff --git a/client/governor.cpp b/client/governor.cpp
index d27567f286..cefb1e5660 100644
--- a/client/governor.cpp
+++ b/client/governor.cpp
@@ -20,6 +20,7 @@
 #include "dataio.h"
 #include "featured_text.h"
 #include "nation.h"
+#include "player.h"
 #include "specialist.h"
 // client
 #include "attribute.h"
@@ -52,11 +53,6 @@
 
 static struct preset_list *preset_list = nullptr;
 
-static void city_remove(int city_id)
-{
-  attr_city_set(ATTR_CITY_CMA_PARAMETER, city_id, 0, nullptr);
-}
-
 struct cma_preset {
   char *descr;
   struct cm_parameter parameter;
@@ -119,19 +115,19 @@ governor *governor::i()
 // register new event and run it if hot
 void governor::add_city_changed(struct city *pcity)
 {
-  scity_changed.insert(pcity);
+  scity_changed.insert(pcity->id);
   run();
 };
 
 void governor::add_city_new(struct city *pcity)
 {
-  scity_changed.insert(pcity);
+  scity_changed.insert(pcity->id);
   run();
 };
 
 void governor::add_city_remove(struct city *pcity)
 {
-  scity_remove.insert(pcity);
+  scity_remove.insert(pcity->id);
   run();
 };
 
@@ -142,34 +138,23 @@ void governor::run()
     return;
   }
 
-  for (auto *pcity : qAsConst(scity_changed)) {
-    // dont check city if its not ours, asan says
-    // city was removed, but city still points to something
-    // uncomment and check whats happening when city is conquered
-    bool dontCont = false;
-    city_list_iterate(client.conn.playing->cities, wtf)
-    {
-      if (wtf == pcity) {
-        dontCont = true;
-      }
-    }
-    city_list_iterate_end;
+  // Remove deleted cities
+  for (auto id : scity_remove) {
+    attr_city_set(ATTR_CITY_CMAFE_PARAMETER, id, 0, nullptr);
+    attr_city_set(ATTR_CITY_CMA_PARAMETER, id, 0, nullptr);
+  }
+  scity_remove.clear();
 
-    if (!dontCont) {
-      continue;
-    }
-    if (pcity) {
-      gimb->handle_city(pcity);
+  // Handle changed cities
+  for (auto id : scity_changed) {
+    auto city = player_city_by_number(client.conn.playing, id);
+    // Might have been removed
+    if (city) {
+      gimb->handle_city(city);
     }
   }
   scity_changed.clear();
-  for (auto *pcity : qAsConst(scity_remove)) {
-    if (pcity) {
-      attr_city_set(ATTR_CITY_CMAFE_PARAMETER, pcity->id, 0, nullptr);
-      city_remove(pcity->id);
-    }
-  }
-  scity_remove.clear();
+
   update_turn_done_button_state();
 }
 
diff --git a/client/governor.h b/client/governor.h
index 83b1aa26df..252f4a2416 100644
--- a/client/governor.h
+++ b/client/governor.h
@@ -11,7 +11,8 @@
 #pragma once
 
 #include "attribute.h"
-#include <QSet>
+
+#include <set>
 
 class governor {
 public:
@@ -33,8 +34,8 @@ class governor {
   governor() { superhot = 1; };
   void run();
   static governor *m_instance;
-  QSet<struct city *> scity_changed;
-  QSet<struct city *> scity_remove;
+  std::set<int> scity_changed;
+  std::set<int> scity_remove;
   int superhot;
 };