[Freeciv-Dev] (PR#13353) Insane Unit Ownership at Game Start

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

Seems this is yet another side effect of the genlist-pointer change. 
The aifill change is unrelated but makes it more likely to happen since
players are renumbered more often.

In 2.0, copying a player will duplicate the unit list so long as it's
empty.  But in 2.1, copying a player copies the pointer so the list is
effectively duplicated even if it's empty.

This patch fixes this in one of the two possible ways.  When removing a
player all elements are freed.  When renumbering players the player is
re-initted.  This means the list is freed only to be added again moments
later.  However this is potentially buggy if game_remove_player is
called somewhere and player_init is not then called afterwards.

The alternative (as explained in the FIXME comment that's removed) would
be to copy the player internally in game_renumber_players.  This would
avoid the issue of copied lists since the list of the removed player
would just be passed along to the "new" player.  However this is
potentially buggy since some elements may still need to be
re-initialized (like the username, etc. - the values that are currently
initialized in game_remove_player) and it's easy to make a mistake on
what has to be initialized (e.g., list internals) and what cannot be
(e.g., list pointers).

-jason

Index: common/game.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
retrieving revision 1.222
diff -u -r1.222 game.c
--- common/game.c       24 Jun 2005 04:34:03 -0000      1.222
+++ common/game.c       29 Jun 2005 03:11:23 -0000
@@ -429,9 +429,8 @@
 }
 
 /****************************************************************************
-  Reset a player's data to its initial state.  No further initialization
-  should be needed before reusing this player (no separate call to
-  player_init is needed).
+  Reset a player's data to its initial state.  After calling this you
+  must call player_init before the player can be used again.
 ****************************************************************************/
 void game_remove_player(struct player *pplayer)
 {
@@ -440,23 +439,31 @@
     pplayer->attribute_block.data = NULL;
   }
 
-  /* Unlink all the lists, but don't free them (they can be used later). */
+  assert(conn_list_size(pplayer->connections) == 0);
   conn_list_unlink_all(pplayer->connections);
+  conn_list_free(pplayer->connections);
+  pplayer->connections = NULL;
 
   unit_list_iterate(pplayer->units, punit) {
     game_remove_unit(punit);
   } unit_list_iterate_end;
   assert(unit_list_size(pplayer->units) == 0);
   unit_list_unlink_all(pplayer->units);
+  unit_list_free(pplayer->units);
+  pplayer->units = NULL;
 
   city_list_iterate(pplayer->cities, pcity) {
     game_remove_city(pcity);
   } city_list_iterate_end;
   assert(city_list_size(pplayer->cities) == 0);
   city_list_unlink_all(pplayer->cities);
+  city_list_free(pplayer->cities);
+  pplayer->cities = NULL;
+
+  free(pplayer->research);
+  pplayer->research = NULL;
 
   if (is_barbarian(pplayer)) game.info.nbarbarians--;
-  player_research_init(get_player_research(pplayer));
 }
 
 /***************************************************************
@@ -472,6 +479,8 @@
     conn_list_iterate(game.players[i].connections, pconn)
       pconn->player = &game.players[i];
     conn_list_iterate_end;
+    assert(unit_list_size(game.players[i].units) == 0);
+    assert(city_list_size(game.players[i].cities) == 0);
   }
 
   if(game.info.player_idx > plrno) {
@@ -481,23 +490,7 @@
 
   game.info.nplayers--;
 
-  /* a bit of cleanup to keep connections sane */
-  /* FIXME: this code is potentially quite buggy because it introduces
-   * a second place where we have to "initialize" players.  We should
-   * probably instead either use player_init on the removed player or
-   * copy the removed player directly from the middle of the array to
-   * the end.  However it's likely that neither will work without fixing
-   * some things elsewhere.
-   *
-   * Secondary FIXME: this code leaks memory because the conn_list is
-   * never freed.  See FIXME above... */
-  game.players[game.info.nplayers].connections = conn_list_new();
-  game.players[game.info.nplayers].is_connected = FALSE;
-  game.players[game.info.nplayers].was_created = FALSE;
-  game.players[game.info.nplayers].ai.control = FALSE;
-  sz_strlcpy(game.players[game.info.nplayers].name, ANON_PLAYER_NAME);
-  sz_strlcpy(game.players[game.info.nplayers].username, ANON_USER_NAME);
-  game.players[game.info.nplayers].team = NULL;
+  player_init(&game.players[game.info.nplayers]);
 }
 
 /**************************************************************************