<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39668 >

 Calling script_signal_emit() means that almost anything can happen.
Most definitely cities or units can disappear.
 I just glanced where it has been called, and fixed most obvious cases
where some now potentially invalid pointer was used immediately after
script_signal_emit().

 I left one case out of this patch. One in treaty handling needs more
inspection.


 - ML

diff -Nurd -X.diff_ignore freeciv/common/city.c freeciv/common/city.c
--- freeciv/common/city.c	2007-09-02 07:14:07.000000000 +0300
+++ freeciv/common/city.c	2007-09-04 17:48:27.000000000 +0300
@@ -2504,3 +2504,16 @@
   unit_list_free(pcity->units_supported);
   free(pcity);
 }
+
+/**************************************************************************
+  Check if unit with given id is still alive. Use this before using
+  old unit pointers when unit might have dead.
+**************************************************************************/
+bool city_exist(int id)
+{
+  if (find_city_by_id(id)) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
diff -Nurd -X.diff_ignore freeciv/common/city.h freeciv/common/city.h
--- freeciv/common/city.h	2007-09-02 07:14:07.000000000 +0300
+++ freeciv/common/city.h	2007-09-04 17:48:46.000000000 +0300
@@ -518,6 +518,8 @@
 			 int *pollu_prod, int *pollu_pop, int *pollu_mod);
 int city_pollution(const struct city *pcity, int shield_total);
 
+bool city_exist(int id);
+
 /*
  * Iterates over all improvements which are built in the given city.
  */
diff -Nurd -X.diff_ignore freeciv/common/unit.c freeciv/common/unit.c
--- freeciv/common/unit.c	2007-08-08 05:52:11.000000000 +0300
+++ freeciv/common/unit.c	2007-09-04 17:37:21.000000000 +0300
@@ -1604,3 +1604,16 @@
 
   return result;
 }
+
+/**************************************************************************
+  Check if unit with given id is still alive. Use this before using
+  old unit pointers when unit might have dead.
+**************************************************************************/
+bool unit_alive(int id)
+{
+  if (find_unit_by_id(id)) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
diff -Nurd -X.diff_ignore freeciv/common/unit.h freeciv/common/unit.h
--- freeciv/common/unit.h	2007-08-08 05:52:11.000000000 +0300
+++ freeciv/common/unit.h	2007-09-04 17:35:17.000000000 +0300
@@ -318,4 +318,6 @@
 enum unit_upgrade_result get_unit_upgrade_info(char *buf, size_t bufsz,
 					       const struct unit *punit);
 
+bool unit_alive(int id);
+
 #endif  /* FC__UNIT_H */
diff -Nurd -X.diff_ignore freeciv/server/cityturn.c freeciv/server/cityturn.c
--- freeciv/server/cityturn.c	2007-08-13 22:23:44.000000000 +0300
+++ freeciv/server/cityturn.c	2007-09-04 18:07:44.000000000 +0300
@@ -465,6 +465,7 @@
   bool have_square;
   int savings_pct = granary_savings(pcity), new_food;
   bool rapture_grow = city_rapture_grow(pcity); /* check before size increase! */
+  int saved_id = pcity->id;
 
   if (!city_can_grow_to(pcity, pcity->size + 1)) { /* need improvement */
     if (get_current_construction_bonus(pcity, EFT_SIZE_ADJ, RPT_CERTAIN) > 0
@@ -522,8 +523,10 @@
                    _("%s grows to size %d."), pcity->name, pcity->size);
   script_signal_emit("city_growth", 2,
 		      API_TYPE_CITY, pcity, API_TYPE_INT, pcity->size);
-
-  sanity_check_city(pcity);
+  if (city_exist(saved_id)) {
+    /* Script didn't destroy this city */
+    sanity_check_city(pcity);
+  }
   sync_cities();
 }
 
@@ -615,6 +618,7 @@
 {
   bool success = FALSE;
   int i;
+  int saved_id = pcity->id;
 
   if (worklist_is_empty(&pcity->worklist))
     /* Nothing in the worklist; bail now. */
@@ -624,6 +628,12 @@
   while (TRUE) {
     struct city_production target;
 
+    if (!city_exist(saved_id)) {
+      /* Some script has removed useless city that cannot build
+       * what it is told to! */
+      return FALSE;
+    }
+
     /* What's the next item in the worklist? */
     if (!worklist_peek_ith(&pcity->worklist, &target, i))
       /* Nothing more in the worklist.  Ah, well. */
@@ -661,15 +671,16 @@
 			    in the worklist, not its obsolete-closure
 			    new_target. */
 			 utype_name_translation(ptarget));
-	script_signal_emit("unit_cant_be_built", 3,
-			   API_TYPE_UNIT_TYPE, ptarget,
-			   API_TYPE_CITY, pcity,
-			   API_TYPE_STRING, "never");
+
 	/* Purge this worklist item. */
 	worklist_remove(&pcity->worklist, i-1);
 	/* Reset i to index to the now-next element. */
 	i--;
-	
+
+	script_signal_emit("unit_cant_be_built", 3,
+			   API_TYPE_UNIT_TYPE, ptarget,
+			   API_TYPE_CITY, pcity,
+			   API_TYPE_STRING, "never");
 	continue;
       } else {
 	/* Yep, we can go after new_target instead.  Joy! */
@@ -694,16 +705,16 @@
 			   "Purging..."),
 			 pcity->name,
 			 get_impr_name_ex(pcity, ptarget->index));
-	script_signal_emit("building_cant_be_built", 3,
-			   API_TYPE_BUILDING_TYPE, ptarget,
-			   API_TYPE_CITY, pcity,
-			   API_TYPE_STRING, "never");
 
 	/* Purge this worklist item. */
 	worklist_remove(&pcity->worklist, i-1);
 	/* Reset i to index to the now-next element. */
 	i--;
-	
+
+	script_signal_emit("building_cant_be_built", 3,
+			   API_TYPE_BUILDING_TYPE, ptarget,
+			   API_TYPE_CITY, pcity,
+			   API_TYPE_STRING, "never");
 	continue;
       }
 
@@ -822,7 +833,14 @@
 	    }
 	    break;
 	  }
+
+         if (!city_exist(saved_id)) {
+            /* Some script has removed city */
+            return FALSE;
+          }
+
 	} requirement_vector_iterate_end;
+
 	if (!known) {
 	  /* This shouldn't happen...
 	     FIXME: make can_build_improvement() return a reason enum. */
@@ -1056,6 +1074,7 @@
   int mod;
   Impr_type_id id = pcity->production.value;
   struct impr_type *building = improvement_by_number(id);
+  int saved_id = pcity->id;
 
   if (!pcity->production.is_unit
       && improvement_has_flag(pcity->production.value, IF_GOLD)) {
@@ -1128,6 +1147,11 @@
 		       API_TYPE_BUILDING_TYPE, improvement_by_number(id),
 		       API_TYPE_CITY, pcity);
 
+    if (!city_exist(saved_id)) {
+      /* Script removed city */
+      return FALSE;
+    }
+
     /* Call this function since some buildings may change the
      * the vision range of a city */
     city_refresh_vision(pcity);
@@ -1192,19 +1216,24 @@
         _("%s is building %s, which is no longer available."),
         pcity->name,
         utype_name_translation(utype));
-    script_signal_emit("unit_cant_be_built", 3,
-		       API_TYPE_UNIT_TYPE, utype,
-		       API_TYPE_CITY, pcity,
-		       API_TYPE_STRING, "unavailable");
+
+    /* Log before signal emitting, so pointers are certainly valid */
     freelog(LOG_VERBOSE, "%s's %s tried to build %s, which is not available.",
             pplayer->name,
             pcity->name,
             utype_rule_name(utype));
+
+    script_signal_emit("unit_cant_be_built", 3,
+		       API_TYPE_UNIT_TYPE, utype,
+		       API_TYPE_CITY, pcity,
+		       API_TYPE_STRING, "unavailable");
+
     return TRUE;
   }
   if (pcity->shield_stock >= unit_build_shield_cost(utype)) {
     int pop_cost = unit_pop_value(utype);
     struct unit *punit;
+    int saved_city_id = pcity->id;
 
     /* Should we disband the city? -- Massimo */
     if (pcity->size == pop_cost
@@ -1254,15 +1283,17 @@
     script_signal_emit("unit_built",
 		       2, API_TYPE_UNIT, punit, API_TYPE_CITY, pcity);
 
-    /* Done building this unit; time to move on to the next. */
-    choose_build_target(pplayer, pcity);
+    if (city_exist(saved_city_id)) {
+      /* Done building this unit; time to move on to the next. */
+      choose_build_target(pplayer, pcity);
+    }
   }
   return TRUE;
 }
 
 /**************************************************************************
-return 0 if the city is removed
-return 1 otherwise
+return FALSE if the city is removed
+return TRUE otherwise
 **************************************************************************/
 static bool city_build_stuff(struct player *pplayer, struct city *pcity)
 {
@@ -1545,6 +1576,7 @@
   struct tile *ptile = pcity->tile;
   struct city *rcity=NULL;
   struct unit_type *utype = utype_by_number(pcity->production.value);
+  int saved_id = pcity->id;
 
   /* find closest city other than pcity */
   rcity = find_closest_owned_city(pplayer, ptile, FALSE, pcity);
@@ -1560,7 +1592,12 @@
 		       API_TYPE_UNIT_TYPE, utype,
 		       API_TYPE_CITY, pcity,
 		       API_TYPE_STRING, "pop_cost");
-    return FALSE;
+    if (!city_exist(saved_id)) {
+      /* Script decided to remove even the last city */
+      return TRUE;
+    } else {
+      return FALSE;
+    }
   }
 
   (void) create_unit(pplayer, ptile, utype,
diff -Nurd -X.diff_ignore freeciv/server/unittools.c freeciv/server/unittools.c
--- freeciv/server/unittools.c	2007-09-02 15:37:58.000000000 +0300
+++ freeciv/server/unittools.c	2007-09-04 17:41:42.000000000 +0300
@@ -2317,6 +2317,7 @@
   struct player *pplayer = unit_owner(punit);
   bool ok = TRUE;
   int hut_chance = myrand(12);
+  int saved_id = punit->id;
   
   if (game.info.hut_overflight == OVERFLIGHT_NOTHING && is_air_unit(punit)) {
     return ok;
@@ -2340,32 +2341,36 @@
 
   script_signal_emit("hut_enter", 1, API_TYPE_UNIT, punit);
 
-  switch (hut_chance) {
-  case 0:
-    hut_get_gold(punit, 25);
-    break;
-  case 1: case 2: case 3:
-    hut_get_gold(punit, 50);
-    break;
-  case 4:
-    hut_get_gold(punit, 100);
-    break;
-  case 5: case 6: case 7:
-    hut_get_tech(punit);
-    break;
-  case 8: case 9:
-    if (num_role_units(L_HUT) != 0) {
-      hut_get_mercenaries(punit);
-    } else {
-      hut_get_gold(punit, 25);
+  if (unit_alive(saved_id)) {
+    /* Script didn't cause unit death */
+
+    switch (hut_chance) {
+     case 0:
+       hut_get_gold(punit, 25);
+       break;
+     case 1: case 2: case 3:
+       hut_get_gold(punit, 50);
+       break;
+     case 4:
+       hut_get_gold(punit, 100);
+       break;
+     case 5: case 6: case 7:
+       hut_get_tech(punit);
+       break;
+     case 8: case 9:
+       if (num_role_units(L_HUT) != 0) {
+         hut_get_mercenaries(punit);
+       } else {
+         hut_get_gold(punit, 25);
+       }
+       break;
+     case 10:
+       ok = hut_get_barbarians(punit);
+       break;
+     case 11:
+       hut_get_city(punit);
+       break;
     }
-    break;
-  case 10:
-    ok = hut_get_barbarians(punit);
-    break;
-  case 11:
-    hut_get_city(punit);
-    break;
   }
 
   send_player_info(pplayer, pplayer);       /* eg, gold */
@@ -2724,6 +2729,7 @@
   struct unit *ptransporter = NULL;
   struct vision *old_vision = punit->server.vision;
   struct vision *new_vision;
+  int saved_id = punit->id;
     
   conn_list_do_buffer(pplayer->connections);
 
@@ -2890,10 +2896,19 @@
   } square_iterate_end;
 
   handle_unit_move_consequences(punit, psrctile, pdesttile);
+
+  /* FIXME: Should signal emit be after sentried units have been
+   *        waken up in case script causes unit death. */
   script_signal_emit("unit_moved", 3,
 		     API_TYPE_UNIT, punit,
 		     API_TYPE_TILE, psrctile,
 		     API_TYPE_TILE, pdesttile);
+
+  if (!unit_alive(saved_id)) {
+    /* Script caused unit to die */
+    return FALSE;
+  }
+
   wakeup_neighbor_sentries(punit);
   if (!unit_survive_autoattack(punit)) {
     conn_list_do_unbuffer(pplayer->connections);
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to