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

On 08/09/07, Marko Lindqvist  wrote:
>
>  move_unit()

 ...

> Now some
> callers are ready to segfault when ever moving unit dies due to
> autoattack or by entering barbarian hut (or some other reason?).

 Minimally tested patch for S2_1.


 - ML

diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2007-09-02 15:37:58.000000000 +0300
+++ freeciv/server/citytools.c	2007-09-09 15:56:12.000000000 +0300
@@ -578,8 +578,14 @@
    * Only relevant if we are transferring to another player. */
   if (pplayer != pvictim) {
     unit_list_iterate_safe((ptile)->units, vunit)  {
-      /* Don't transfer units already owned by new city-owner --wegge */ 
+      /* Don't transfer units already owned by new city-owner --wegge */
       if (unit_owner(vunit) == pvictim) {
+        /* vunit may die during transfer_unit().
+         * unit_list_unlink() is still safe using vunit pointer, as
+         * pointer is not used for dereferencing, only as value.
+         * Not sure if it would be safe to unlink first and transfer only
+         * after that. Not sure if it is correct to unlink at all in
+         * some cases, depending which list 'units' points to. */
 	transfer_unit(vunit, pcity, verbose);
 	unit_list_unlink(units, vunit);
       } else if (!pplayers_allied(pplayer, unit_owner(vunit))) {
@@ -604,7 +610,7 @@
       transfer_unit(vunit, pcity, verbose);
     } else {
       /* The unit is lost.  Call notify_player (in all other cases it is
-       * called autmatically). */
+       * called automatically). */
       freelog(LOG_VERBOSE, "Lost %s's %s at (%d,%d) when %s was lost.",
 	      unit_owner(vunit)->name,
 	      unit_rule_name(vunit),
diff -Nurd -X.diff_ignore freeciv/server/maphand.c freeciv/server/maphand.c
--- freeciv/server/maphand.c	2007-08-31 21:24:51.000000000 +0300
+++ freeciv/server/maphand.c	2007-09-09 15:16:38.000000000 +0300
@@ -1360,6 +1360,8 @@
 static void bounce_units_on_terrain_change(struct tile *ptile)
 {
   unit_list_iterate_safe(ptile->units, punit) {
+    bool unit_alive = TRUE;
+
     if (punit->tile == ptile
 	&& punit->transported_by == -1
 	&& !can_unit_exist_at_tile(punit, ptile)) {
@@ -1376,14 +1378,16 @@
 			   punit->tile, E_UNIT_RELOCATED,
 			   _("Moved your %s due to changing terrain."),
 			   unit_name_translation(punit));
-	  (void) move_unit(punit, ptile2, 0);
-	  if (punit->activity == ACTIVITY_SENTRY) {
-	    handle_unit_activity_request(punit, ACTIVITY_IDLE);
-	  }
+	  unit_alive = move_unit(punit, ptile2, 0);
+          if (unit_alive) {
+            if (punit->activity == ACTIVITY_SENTRY) {
+              handle_unit_activity_request(punit, ACTIVITY_IDLE);
+            }
+          }
 	  break;
 	}
       } adjc_iterate_end;
-      if (punit->tile == ptile) {
+      if (unit_alive && punit->tile == ptile) {
 	/* if we get here we could not move punit */
 	freelog(LOG_VERBOSE,
 		"Disbanded %s's %s due to changing land to sea at (%d,%d).",
diff -Nurd -X.diff_ignore freeciv/server/unithand.c freeciv/server/unithand.c
--- freeciv/server/unithand.c	2007-08-01 19:18:41.000000000 +0300
+++ freeciv/server/unithand.c	2007-09-09 15:27:46.000000000 +0300
@@ -273,12 +273,15 @@
   Transfer a unit from one homecity to another. If unit is not presently
   in its new homecity, it will be moved there. This new homecity must
   be valid for this unit.
+
+  Note that unit may die in the process.
 **************************************************************************/
 void real_unit_change_homecity(struct unit *punit, struct city *new_pcity)
 {
   struct city *old_pcity = find_city_by_id(punit->homecity);
   struct player *old_owner = unit_owner(punit);
   struct player *new_owner = city_owner(new_pcity);
+  bool unit_alive = TRUE;
 
   if (old_owner != new_owner) {
     vision_clear_sight(punit->server.vision);
@@ -296,16 +299,19 @@
 
   if (!same_pos(punit->tile, new_pcity->tile)) {
     assert(can_unit_exist_at_tile(punit, new_pcity->tile));
-    move_unit(punit, new_pcity->tile, 0); /* teleport to location */
-  }
-  
-  unit_list_prepend(new_pcity->units_supported, punit);
-  if (old_pcity) {
-    unit_list_unlink(old_pcity->units_supported, punit);
+    /* Teleport to location */
+    unit_alive = move_unit(punit, new_pcity->tile, 0);
   }
 
-  punit->homecity = new_pcity->id;
-  send_unit_info(unit_owner(punit), punit);
+  if (unit_alive) {
+    unit_list_prepend(new_pcity->units_supported, punit);
+    if (old_pcity) {
+      unit_list_unlink(old_pcity->units_supported, punit);
+    }
+
+    punit->homecity = new_pcity->id;
+    send_unit_info(unit_owner(punit), punit);
+  }
 
   city_refresh(new_pcity);
   send_city_info(new_owner, new_pcity);
@@ -315,7 +321,8 @@
     city_refresh(old_pcity);
     send_city_info(old_owner, old_pcity);
   }
-  assert(unit_owner(punit) == city_owner(new_pcity));
+
+  assert(!unit_alive || unit_owner(punit) == city_owner(new_pcity));
 }
 
 /**************************************************************************
@@ -1121,8 +1128,10 @@
       assert(is_enemy_city_tile(pdesttile, pplayer) != NULL);
 
       if (unit_has_type_flag(punit, F_NUCLEAR)) {
-        move_unit(punit, pcity->tile, 0);
-        handle_unit_attack_request(punit, punit); /* Boom! */
+        if (move_unit(punit, pcity->tile, 0)) {
+          /* Survived dangers of moving */
+          handle_unit_attack_request(punit, punit); /* Boom! */
+        }
         return TRUE;
       }
 
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to