Rewrite cm.{h,cpp} for more memory safety

Use std::unique_ptr to prevent memory leaks or double free (there was a double
free in result_came_from_server). Also don't trust that the server sends CMA
results before PACKET_PROCESSING_FINISHED.

Closes #803.

ai/default/aihand.cpp

@@ -461,7 +461,7 @@ static void dai_manage_taxes(struct ai_type *ait, struct player *pplayer)
     // Check if we celebrate - the city state must be restored at the end!
     city_list_iterate(pplayer->cities, pcity)
     {
-      struct cm_result *cmr = cm_result_new(pcity);
+      auto cmr = cm_result_new(pcity);
       struct ai_city *city_data = def_ai_city_data(pcity, ait);
 
       cm_query_result(pcity, &cmp, cmr, false); // burn some CPU
@@ -476,7 +476,6 @@ static void dai_manage_taxes(struct ai_type *ait, struct player *pplayer)
       } else {
         city_data->celebrate = false;
       }
-      cm_result_destroy(cmr);
     }
     city_list_iterate_end;
 
@@ -646,7 +645,7 @@ static void dai_manage_taxes(struct ai_type *ait, struct player *pplayer)
 
     city_list_iterate(pplayer->cities, pcity)
     {
-      struct cm_result *cmr = cm_result_new(pcity);
+      auto cmr = cm_result_new(pcity);
 
       if (def_ai_city_data(pcity, ait)->celebrate) {
         log_base(LOGLEVEL_TAX, "setting %s to celebrate",
@@ -662,7 +661,6 @@ static void dai_manage_taxes(struct ai_type *ait, struct player *pplayer)
           CITY_LOG(LOG_ERROR, pcity, "has NO valid state!");
         }
       }
-      cm_result_destroy(cmr);
     }
     city_list_iterate_end;
   } else if (celebrate == AI_CELEBRATION_NO) {

client/governor.cpp

@@ -89,9 +89,8 @@ class cma_yoloswag {
 private:
   struct city *check_city(int city_id, struct cm_parameter *parameter);
   bool apply_result_on_server(struct city *pcity,
-                              const struct cm_result *result);
-  struct cm_result *cma_state_result;
-  const struct cm_result *cma_result_got;
+                              std::unique_ptr<cm_result> &&result);
+  std::unique_ptr<cm_result> cma_result_got;
   int last_request;
   struct city *xcity;
 };
@@ -220,7 +219,6 @@ void cma_got_result(int citynr) { gimb->result_came_from_server(citynr); }
 // yolo constructor
 cma_yoloswag::cma_yoloswag()
 {
-  cma_state_result = nullptr;
   last_request = -9999;
   cma_result_got = nullptr;
   xcity = nullptr;
@@ -246,9 +244,10 @@ void cma_yoloswag::result_came_from_server(int last_request_id)
   }
 
   // Return.
-  cm_result_from_main_map(cma_state_result, pcity);
+  auto state_result = cm_result_new(pcity);
+  cm_result_from_main_map(state_result, pcity);
 
-  success = (*cma_state_result == *cma_result_got);
+  success = (cma_result_got && *state_result == *cma_result_got);
   if (!success) {
 #if SHOW_APPLY_RESULT_ON_SERVER_ERRORS
     qCritical("apply_result_on_server(city %d=\"%s\") no match!", pcity->id,
@@ -257,37 +256,34 @@ void cma_yoloswag::result_came_from_server(int last_request_id)
     log_test("apply_result_on_server(city %d=\"%s\") have:", pcity->id,
              city_name_get(pcity));
     cm_print_city(pcity);
-    cm_print_result(cma_state_result);
+    cm_print_result(state_result);
 
     log_test("apply_result_on_server(city %d=\"%s\") want:", pcity->id,
              city_name_get(pcity));
     cm_print_result(cma_result_got);
 #endif // SHOW_APPLY_RESULT_ON_SERVER_ERRORS
   }
-  cm_result_destroy(cma_state_result);
-  cm_result_destroy(const_cast<cm_result *>(cma_result_got));
+  cma_result_got = nullptr;
   log_apply_result("apply_result_on_server() return %d.", (int) success);
-  cma_state_result = nullptr;
   last_request = -9999;
   xcity = nullptr;
 }
 
 cma_yoloswag::~cma_yoloswag() = default;
 
 /**
- Change the actual city setting to the given result. Returns TRUE iff
- the actual data matches the calculated one.
-****************************************************************************/
-bool cma_yoloswag::apply_result_on_server(struct city *pcity,
-                                          const struct cm_result *result)
+ * Change the actual city setting to the given result. Returns TRUE iff
+ * the actual data matches the calculated one.
+ */
+bool cma_yoloswag::apply_result_on_server(
+    struct city *pcity, std::unique_ptr<cm_result> &&result)
 {
   int first_request_id = 0, last_request_id = 0, i;
   int city_radius_sq = city_map_radius_sq_get(pcity);
-  struct cm_result *current_state;
   struct tile *pcenter = city_tile(pcity);
 
   fc_assert_ret_val(result->found_a_valid, false);
-  current_state = cm_result_new(pcity);
+  auto current_state = cm_result_new(pcity);
   cm_result_from_main_map(current_state, pcity);
 
   if (*current_state == *result && !ALWAYS_APPLY_AT_SERVER) {
@@ -402,8 +398,7 @@ bool cma_yoloswag::apply_result_on_server(struct city *pcity,
 
   connection_do_unbuffer(&client.conn);
 
-  cma_result_got = result;
-  cma_state_result = current_state;
+  cma_result_got = std::move(result);
   last_request = last_request_id;
   xcity = pcity;
   return true;
@@ -550,8 +545,7 @@ struct city *cma_yoloswag::check_city(int city_id,
  */
 void cma_yoloswag::handle_city(struct city *pcity)
 {
-  auto result = std::unique_ptr<cm_result, typeof(&cm_result_destroy)>(
-      cm_result_new(pcity), &cm_result_destroy);
+  auto result = cm_result_new(pcity);
   bool handled;
   int i, city_id = pcity->id;
 
@@ -573,7 +567,7 @@ void cma_yoloswag::handle_city(struct city *pcity)
       break;
     }
 
-    cm_query_result(pcity, &parameter, result.get(), false);
+    cm_query_result(pcity, &parameter, result, false);
     if (!result->found_a_valid) {
       log_handle_city2("  no valid found result");
 
@@ -586,7 +580,7 @@ void cma_yoloswag::handle_city(struct city *pcity)
       handled = true;
       break;
     } else {
-      if (!apply_result_on_server(pcity, result.get())) {
+      if (!apply_result_on_server(pcity, std::move(result))) {
         log_handle_city2("  doesn't cleanly apply");
         if (pcity == check_city(city_id, NULL) && i == 0) {
           create_event(city_tile(pcity), E_CITY_CMA_RELEASE, ftc_client,
@@ -600,6 +594,7 @@ void cma_yoloswag::handle_city(struct city *pcity)
         handled = true;
         break;
       }
+      result = nullptr;
     }
   }
 

common/aicore/cm.cpp

@@ -263,7 +263,7 @@ static void real_print_partial_solution(QtMsgType level, const char *file,
 #define print_partial_solution(loglevel, soln, state)
 #endif // FREECIV_DEBUG
 
-static void cm_result_copy(struct cm_result *result,
+static void cm_result_copy(std::unique_ptr<cm_result> &result,
                            const struct city *pcity, bool *workers_map);
 
 static double estimate_fitness(const struct cm_state *state,
@@ -318,17 +318,15 @@ void cm_free()
 /**
    Create a new cm_result.
  */
-struct cm_result *cm_result_new(struct city *pcity)
+std::unique_ptr<cm_result> cm_result_new(struct city *pcity)
 {
-  struct cm_result *result;
-
   // initialise all values
-  result = new cm_result[1]();
+  auto result = std::make_unique<cm_result>();
   result->city_radius_sq =
       pcity ? city_map_radius_sq_get(pcity) : CITY_MAP_MAX_RADIUS_SQ;
   int tiles = city_map_tiles(result->city_radius_sq);
   fc_assert_ret_val(tiles > 0, result);
-  result->worker_positions = new bool[tiles]();
+  result->worker_positions.resize(tiles, false);
 
   /* test if the city pointer is valid; the cm_result struct can be
    * returned as it uses the maximal possible value for the size of
@@ -338,19 +336,6 @@ struct cm_result *cm_result_new(struct city *pcity)
   return result;
 }
 
-/**
-   Destroy a cm_result.
- */
-void cm_result_destroy(struct cm_result *result)
-{
-  if (result != NULL) {
-    if (result->worker_positions != NULL) {
-      FCPP_FREE(result->worker_positions);
-    }
-    FCPP_FREE(result);
-  }
-}
-
 /****************************************************************************
   Functions of tile-types.
 ****************************************************************************/
@@ -780,7 +765,7 @@ evaluate_solution(struct cm_state *state,
  */
 static void convert_solution_to_result(struct cm_state *state,
                                        const struct partial_solution *soln,
-                                       struct cm_result *result)
+                                       std::unique_ptr<cm_result> &result)
 {
   struct cm_fitness fitness;
 
@@ -2050,7 +2035,8 @@ static void cm_state_free(struct cm_state *state)
  */
 static void cm_find_best_solution(struct cm_state *state,
                                   const struct cm_parameter *const parameter,
-                                  struct cm_result *result, bool negative_ok)
+                                  std::unique_ptr<cm_result> &result,
+                                  bool negative_ok)
 {
   int loop_count = 0;
   int max_count;
@@ -2100,7 +2086,7 @@ static void cm_find_best_solution(struct cm_state *state,
    solution.
  */
 void cm_query_result(struct city *pcity, const struct cm_parameter *param,
-                     struct cm_result *result, bool negative_ok)
+                     std::unique_ptr<cm_result> &result, bool negative_ok)
 {
   struct cm_state *state = cm_state_init(pcity, negative_ok);
 
@@ -2192,7 +2178,7 @@ void cm_init_emergency_parameter(struct cm_parameter *dest)
 /**
    Count the total number of workers in the result.
  */
-int cm_result_workers(const struct cm_result *result)
+int cm_result_workers(const std::unique_ptr<cm_result> &result)
 {
   int count = 0;
 
@@ -2214,7 +2200,7 @@ int cm_result_workers(const struct cm_result *result)
 /**
    Count the total number of specialists in the result.
  */
-int cm_result_specialists(const struct cm_result *result)
+int cm_result_specialists(const std::unique_ptr<cm_result> &result)
 {
   int count = 0;
 
@@ -2227,7 +2213,7 @@ int cm_result_specialists(const struct cm_result *result)
 /**
    Count the total number of citizens in the result.
  */
-int cm_result_citizens(const struct cm_result *result)
+int cm_result_citizens(const std::unique_ptr<cm_result> &result)
 {
   return cm_result_workers(result) + cm_result_specialists(result);
 }
@@ -2236,7 +2222,7 @@ int cm_result_citizens(const struct cm_result *result)
    Copy the city's current setup into the cm result structure. Wrapper for
    cm_result_main().
  */
-void cm_result_from_main_map(struct cm_result *result,
+void cm_result_from_main_map(std::unique_ptr<cm_result> &result,
                              const struct city *pcity)
 {
   cm_result_copy(result, pcity, NULL);
@@ -2247,15 +2233,15 @@ void cm_result_from_main_map(struct cm_result *result,
    is a bool array with the size city_map_tiles_from_city(pcity). It is TRUE
    for tiles worked by the city.
  */
-static void cm_result_copy(struct cm_result *result,
+static void cm_result_copy(std::unique_ptr<cm_result> &result,
                            const struct city *pcity, bool *workers_map)
 {
   struct tile *pcenter = city_tile(pcity);
 
   // clear worker positions
-  memset(result->worker_positions, 0,
-         sizeof(*result->worker_positions)
-             * city_map_tiles(result->city_radius_sq));
+  for (auto position : result->worker_positions) {
+    position = false;
+  }
 
   city_tile_iterate_index(result->city_radius_sq, pcenter, ptile, ctindex)
   {
@@ -2452,11 +2438,11 @@ void cm_print_city(const struct city *pcity)
 /**
    Print debugging information about a full CM result.
  */
-void cm_print_result(const struct cm_result *result)
+void cm_print_result(const std::unique_ptr<cm_result> &result)
 {
   int *city_map_data = new int[city_map_tiles(result->city_radius_sq)]();
 
-  log_test("cm_print_result(result=%p)", (void *) result);
+  log_test("cm_print_result(result=%p)", (void *) result.get());
   log_test("  found_a_valid=%d disorder=%d happy=%d", result->found_a_valid,
            result->disorder, result->happy);
 

common/aicore/cm.h

@@ -32,16 +32,17 @@ struct cm_result {
   int surplus[O_LAST];
 
   int city_radius_sq;
-  bool *worker_positions;
+  std::vector<bool> worker_positions;
   citizens specialists[SP_MAX];
+
+  ~cm_result() = default;
 };
 
 void cm_init();
 void cm_init_citymap();
 void cm_free();
 
-struct cm_result *cm_result_new(struct city *pcity);
-void cm_result_destroy(struct cm_result *result);
+std::unique_ptr<cm_result> cm_result_new(struct city *pcity);
 
 /*
  * Will try to meet the requirements and fill out the result. Caller
@@ -50,7 +51,7 @@ void cm_result_destroy(struct cm_result *result);
  */
 void cm_query_result(struct city *pcity,
                      const struct cm_parameter *const parameter,
-                     struct cm_result *result, bool negative_ok);
+                     std::unique_ptr<cm_result> &result, bool negative_ok);
 
 /***************** utility methods *************************************/
 bool operator==(const struct cm_parameter &p1,
@@ -61,11 +62,11 @@ void cm_init_parameter(struct cm_parameter *dest);
 void cm_init_emergency_parameter(struct cm_parameter *dest);
 
 void cm_print_city(const struct city *pcity);
-void cm_print_result(const struct cm_result *result);
+void cm_print_result(const std::unique_ptr<cm_result> &result);
 
-int cm_result_citizens(const struct cm_result *result);
-int cm_result_specialists(const struct cm_result *result);
-int cm_result_workers(const struct cm_result *result);
+int cm_result_citizens(const std::unique_ptr<cm_result> &result);
+int cm_result_specialists(const std::unique_ptr<cm_result> &result);
+int cm_result_workers(const std::unique_ptr<cm_result> &result);
 
-void cm_result_from_main_map(struct cm_result *result,
+void cm_result_from_main_map(std::unique_ptr<cm_result> &result,
                              const struct city *pcity);

server/cityturn.cpp

@@ -282,7 +282,8 @@ void remove_obsolete_buildings(struct player *pplayer)
   Rearrange workers according to a cm_result struct.  The caller must make
   sure that the result is valid.
 **************************************************************************/
-void apply_cmresult_to_city(struct city *pcity, const struct cm_result *cmr)
+void apply_cmresult_to_city(struct city *pcity,
+                            const std::unique_ptr<cm_result> &cmr)
 {
   struct tile *pcenter = city_tile(pcity);
 
@@ -367,7 +368,6 @@ static void set_default_city_manager(struct cm_parameter *cmp,
 void auto_arrange_workers(struct city *pcity)
 {
   struct cm_parameter cmp;
-  struct cm_result *cmr;
 
   /* See comment in freeze_workers(): we can't rearrange while
    * workers are frozen (i.e. multiple updates need to be done). */
@@ -403,7 +403,7 @@ void auto_arrange_workers(struct city *pcity)
 
   /* This must be after city_refresh() so that the result gets created for
    * the right city radius */
-  cmr = cm_result_new(pcity);
+  auto cmr = cm_result_new(pcity);
   cm_query_result(pcity, &cmp, cmr, false);
 
   if (!cmr->found_a_valid) {
@@ -461,7 +461,6 @@ void auto_arrange_workers(struct city *pcity)
   }
   sanity_check_city(pcity);
 
-  cm_result_destroy(cmr);
   TIMING_LOG(AIT_CITIZEN_ARRANGE, TIMER_STOP);
 }
 

server/cityturn.h

@@ -25,7 +25,8 @@ void city_refresh_queue_add(struct city *pcity);
 void city_refresh_queue_processing();
 
 void auto_arrange_workers(struct city *pcity); // will arrange the workers
-void apply_cmresult_to_city(struct city *pcity, const struct cm_result *cmr);
+void apply_cmresult_to_city(struct city *pcity,
+                            const std::unique_ptr<cm_result> &cmr);
 
 bool city_change_size(struct city *pcity, citizens new_size,
                       struct player *nationality, const char *reason);