[Freeciv-Dev] (PR#13213) removing a player twice causes server problems

["Jason Dorje Short" <jdorje@xxxxxxxxx>]

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

If you:

  /create a
  /remove a
  /create a
  /remove a

the last line will cause some server problems:

==21000== Invalid read of size 4
==21000==    at 0x80C0928: game_remove_player (game.c:444)
==21000==    by 0x8085414: server_remove_player (plrhand.c:1703)
==21000==    by 0x80550BF: remove_player (stdinhand.c:961)
==21000==    by 0x805B61B: handle_stdin_input (stdinhand.c:3414)
==21000==    by 0x805520E: read_init_script (stdinhand.c:994)
==21000==    by 0x80531E7: srv_main (srv_main.c:1763)
==21000==    by 0x804AB38: main (civserver.c:242)
==21000==  Address 0x1BB40A38 is 0 bytes inside a block of size 4 free'd
==21000==    at 0x1B904B04: free (vg_replace_malloc.c:152)
==21000==    by 0x80C0978: game_remove_player (speclist.h:121)
==21000==    by 0x8085414: server_remove_player (plrhand.c:1703)
==21000==    by 0x80550BF: remove_player (stdinhand.c:961)
==21000==    by 0x805B61B: handle_stdin_input (stdinhand.c:3414)
==21000==    by 0x805520E: read_init_script (stdinhand.c:994)
==21000==    by 0x80531E7: srv_main (srv_main.c:1763)
==21000==    by 0x804AB38: main (civserver.c:242)

It seems the unit list (and other elements) are accessed after being
freed.  This seems to happen because game_remove_player (the opposite of
player_init) is called when a player is removed; however player_init is
not called when a player is created.  This patch fixes it.

-jason

create a
remove a
create a
remove a
create a
remove a

? foo
? vgcore.pid21194
Index: common/game.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/game.c,v
retrieving revision 1.217
diff -u -r1.217 game.c
--- common/game.c       4 Jun 2005 16:31:59 -0000       1.217
+++ common/game.c       4 Jun 2005 22:00:28 -0000
@@ -428,9 +428,11 @@
   game.info.turn++;
 }
 
-/***************************************************************
-...
-***************************************************************/
+/****************************************************************************
+  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).
+****************************************************************************/
 void game_remove_player(struct player *pplayer)
 {
   if (pplayer->attribute_block.data) {
@@ -438,21 +440,21 @@
     pplayer->attribute_block.data = NULL;
   }
 
+  /* Unlink all the lists, but don't free them (they can be used later). */
   conn_list_unlink_all(pplayer->connections);
-  conn_list_free(pplayer->connections);
 
-  unit_list_iterate(pplayer->units, punit) 
+  unit_list_iterate(pplayer->units, punit) {
     game_remove_unit(punit);
-  unit_list_iterate_end;
-  unit_list_free(pplayer->units);
+  } unit_list_iterate_end;
+  unit_list_unlink_all(pplayer->units);
 
-  city_list_iterate(pplayer->cities, pcity) 
+  city_list_iterate(pplayer->cities, pcity) {
     game_remove_city(pcity);
-  city_list_iterate_end;
-  city_list_free(pplayer->cities);
+  } city_list_iterate_end;
+  city_list_unlink_all(pplayer->cities);
 
   if (is_barbarian(pplayer)) game.info.nbarbarians--;
-  free(pplayer->research);
+  player_research_init(pplayer->research);
 }
 
 /***************************************************************
Index: common/player.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.c,v
retrieving revision 1.184
diff -u -r1.184 player.c
--- common/player.c     1 Jun 2005 01:02:34 -0000       1.184
+++ common/player.c     4 Jun 2005 22:00:28 -0000
@@ -85,7 +85,7 @@
 /***************************************************************
  Fill the structure with some sane values
 ***************************************************************/
-static void player_research_init(struct player_research* research)
+void player_research_init(struct player_research* research)
 {
   memset(research, 0, sizeof(struct player_research));
   research->tech_goal = A_UNSET;
Index: common/player.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.h,v
retrieving revision 1.155
diff -u -r1.155 player.h
--- common/player.h     1 Jun 2005 01:02:34 -0000       1.155
+++ common/player.h     4 Jun 2005 22:00:28 -0000
@@ -246,6 +246,7 @@
   bv_debug debug;
 };
 
+void player_research_init(struct player_research* research);
 void player_init(struct player *plr);
 struct player *find_player_by_name(const char *name);
 struct player *find_player_by_name_prefix(const char *name,