[Freeciv-Dev] Re: (PR#4769) Re: Re: (PR#4761) civserver get_invention crash

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Jason Short wrote:
> Jason Short wrote:
> 
>>This is caused by the F_UNDISBANDABLE flag.  When such a unit cannot be 
>>supported a unit of population is taken from the city instead.  Nobody 
>>checks to see if this causes the city to disband.  Random memory 
>>corruption can result.
>>
>>The attached patch should fix this particular instance.  However I 
>>suspect all callers of city_reduce_size should check the return value.
> 
> 
> Of course there are no UNDISBANDABLE units with upkeep in the default 
> ruleset, so it's a bug that we hit this code at all.  The logic in 
> city_distribute_surplus_shields is faulty and will cause population loss 
> any time you have to disband more than one unit from a city at a time.
> 
> Fix attached; this is in addition to the other (memory corruption) 
> patch.  You can see the current code is faulty by adding in the 'assert' 
> line from this patch all by itself: the assertion will trigger.

This "fix" is still broken: if there are any undisbandable units neither 
while loop will ever exit.

This one should do the trick...I hope.  I'm not sure about the update to 
pcity->shield_surplus; will this work under all government types?

Note if the city is destroyed the other patch is still needed to avoid 
memory corruption.  Also note if this happens the undisbandable unit 
will be disbanded.  Per and I discussed taking 4*upkeep worth of gold 
instead of disbanding the city, but decided against it.

jason

Index: server/cityturn.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityturn.c,v
retrieving revision 1.218
diff -u -r1.218 cityturn.c
--- server/cityturn.c   2003/08/04 15:42:47     1.218
+++ server/cityturn.c   2003/08/06 22:27:29
@@ -897,9 +897,10 @@
 {
   struct government *g = get_gov_pplayer(pplayer);
 
-  while (pcity->shield_surplus < 0) {
+  if (pcity->shield_surplus < 0) {
     unit_list_iterate(pcity->units_supported, punit) {
       if (utype_shield_cost(unit_type(punit), g) > 0
+         && pcity->shield_surplus < 0
           && !unit_flag(punit, F_UNDISBANDABLE)) {
        struct packet_unit_request packet;
 
@@ -908,18 +909,28 @@
                         pcity->name, unit_type(punit)->name);
         packet.unit_id = punit->id;
         handle_unit_disband(pplayer, &packet);
-       break;
+       /* pcity->shield_surplus is automatically updated. */
       }
     } unit_list_iterate_end;
+  }
+
+  if (pcity->shield_surplus < 0) {
     /* Special case: F_UNDISBANDABLE. This nasty unit won't go so easily. It'd
-       rather make the citizens pay in blood for their failure to upkeep it! */
+     * rather make the citizens pay in blood for their failure to upkeep it!
+     * If we make it here all normal units are already disbanded, so only
+     * undisbandable ones remain. */
     unit_list_iterate(pcity->units_supported, punit) {
-      if (utype_shield_cost(unit_type(punit), g) > 0) {
+      int upkeep = utype_shield_cost(unit_type(punit), g);
+
+      if (upkeep > 0 && pcity->shield_surplus < 0) {
+       assert(unit_flag(punit, F_UNDISBANDABLE));
        notify_player_ex(pplayer, pcity->x, pcity->y, E_UNIT_LOST,
                         _("Game: Citizens in %s perish for their failure to "
                         "upkeep %s!"), pcity->name, unit_type(punit)->name);
-        city_reduce_size(pcity, 1);
-       break;
+       city_reduce_size(pcity, 1);
+
+       /* No upkeep for the unit this turn. */
+       pcity->shield_surplus += upkeep;
       }
     }
     unit_list_iterate_end;