[Freeciv-Dev] (PR#13223) pubserver crash in handle_player_research

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

See game 431737.

The crash here is an infinite recursion in handle_player_research.  It
is called repeatedly and recursively with Republic and Writing on
players 0 and 3.

I don't know how exactly this happens.  But it's fairly obvious that
this code is quite buggy.

  choose_tech(pplayer, tech);
  send_player_info(pplayer, pplayer);

  /* Notify Team members.  Note that the the player may not necessarily
   * be researching the tech that has been set: if there were enough
   * research points then research will finish immediately and the
   * player will start researching the "next" tech (probably A_NONE). */
  players_iterate(aplayer) {
    if (pplayer != aplayer
        && aplayer->research.researching
             != pplayer->research.researching
        && pplayer->diplstates[aplayer->player_no].type == DS_TEAM
        && aplayer->is_alive) {
      handle_player_research(aplayer, pplayer->research.researching);
    }
  } players_iterate_end;

But since the first choose_tech call may change
pplayer->research.researching, or may do nothing at all if "tech" is
invalid, this is quite problematic.

Scenario 1.  Both players are researching writing.  Player 0 changes the
research to horseback writing.  Because of low techpenalty or carryover
(from having just finished the previous tech and having enough bulbs to
complete H.R. immediately) horseback writing is acquired immediately,
inside choose_tech, and pplayer->research.researching is set to A_NONE.
 Now the second handle_player_research call is called with A_NONE.  In
this second call choose_tech does nothing (you can't choose to research
A_NONE).  Now in the second level of recusion the tech is Writing again.
 So player 0 and player 3 are now both researching writing, BUT player 0
also has horseback writing (and fewer bulbs).

Scenario 2.  Follow through scenario 1.  Now the next turn player 3 has
enough bulbs to finish writing.  He chooses to research republic next
and handle_player_research is called with Republic.  This sets player
3's tech to republic inside choose_tech.  The recursive call now passes
republic as the tech to player 0.  But since player 0 hasn't finished
writing yet (not enough bulbs) he can't research republic.  Now we're in
an infinite recursion.

I don't know if this is how the original crash happened.  But I think
the point is clear.  This patch fixes it and is much cleaner.

Note that in 2.0.99 this isn't a problem because of the shared research
struct (however the shared research struct has a whole host of its own
bugs).

-jason

Index: server/plrhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
retrieving revision 1.330.2.21
diff -u -r1.330.2.21 plrhand.c
--- server/plrhand.c    27 May 2005 08:32:10 -0000      1.330.2.21
+++ server/plrhand.c    7 Jun 2005 07:53:12 -0000
@@ -817,19 +817,13 @@
 **************************************************************************/
 void handle_player_research(struct player *pplayer, int tech)
 {
-  choose_tech(pplayer, tech);
-  send_player_info(pplayer, pplayer);
-
-  /* Notify Team members.  Note that the the player may not necessarily
-   * be researching the tech that has been set: if there were enough
-   * research points then research will finish immediately and the
-   * player will start researching the "next" tech (probably A_NONE). */
+  /* choose_tech and send update for all players on the team. */
   players_iterate(aplayer) {
-    if (pplayer != aplayer
-       && aplayer->research.researching != pplayer->research.researching
-       && pplayer->diplstates[aplayer->player_no].type == DS_TEAM
-       && aplayer->is_alive) {
-      handle_player_research(aplayer, pplayer->research.researching);
+    if (pplayer == aplayer
+       || (pplayer->diplstates[aplayer->player_no].type == DS_TEAM
+           && aplayer->is_alive)) {
+      choose_tech(aplayer, tech);
+      send_player_info(aplayer, aplayer);
     }
   } players_iterate_end;
 }