[Freeciv-Dev] (PR#13350) pubserver 2.0 crash in send_player_info_c

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

> [jdorje - Mon Jun 27 17:01:36 2005]:
> 
> This could relate to PR#13217, which recently changed this function.

Unlikely, since 

(gdb) select 4
(gdb) p src
$1 = (struct player *) 0x81f5d3c
(gdb) p pplayer
$2 = (struct player *) 0x81f5d3c
(gdb) p pconn
$3 = (struct connection *) 0x817df90
(gdb) p pconn->player
$4 = (struct player *) 0x81f5d3c
(gdb) p pconn->player->is_observer
$5 = false

meaning the newly-added clause for !pconn->pplayer doesn't apply.

There are two assertions in package_player_info, and the backtrace
doesn't seem to indicate which has failed:

  assert(server_state != RUN_GAME_STATE
         || ((tech_exists(plr->research.researching)
              && plr->research.researching != A_NONE)
             || is_future_tech(plr->research.researching)
             || plr->research.researching == A_UNSET));
  assert((tech_exists(plr->ai.tech_goal) && plr->ai.tech_goal != A_NONE)
         || plr->ai.tech_goal == A_UNSET);

However:

(gdb) p server_state
$8 = RUN_GAME_STATE
(gdb) p pconn->player->research.researching
$9 = 5
(gdb) p pconn->player->ai.tech_goal
$10 = 0

so the tech goal is 0, which is A_NONE, which is apparently disallowed.
 The backtrace *does* make it clear why this happens:

#3  0x4007015d in __assert_fail () from /lib/libc.so.6
#4  0x080717d5 in send_player_info_c (src=0x81f5d3c, dest=0x81f847c)
    at plrhand.c:1459
#5  0x08070c55 in handle_player_tech_goal (pplayer=0x81f5d3c, tech=0)
    at plrhand.c:861
#6  0x0806ab0b in server_handle_packet (type=PACKET_PLAYER_TECH_GOAL,
    packet=0x8311300, pplayer=0x81f5d3c, pconn=0x817df90) at hand_gen.c:130
#7  0x0804ed7e in handle_packet_input (pconn=0x817df90, packet=0x8311300,
    type=45) at srv_main.c:1014
#8  0x080868d2 in sniff_packets () at sernet.c:627

...it's because the client asked for it!  And we apparently allow that
even though it causes a crash (well, assertion) later.

So, I think this patch fixes the bug.  It's for 2.0 but should be
committed to the dev branch also.  However according to the comment in
the code the behavior probably still isn't perfect.  This should be
fixed in the dev branch (Mateusz, maybe you can do this as part of your
server tech cleanup).

-jason

Index: server/plrhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/plrhand.c,v
retrieving revision 1.330.2.28
diff -u -r1.330.2.28 plrhand.c
--- server/plrhand.c    28 Jun 2005 10:21:22 -0000      1.330.2.28
+++ server/plrhand.c    29 Jun 2005 02:05:57 -0000
@@ -853,6 +853,14 @@
     return;
   }
 
+  if (tech == A_NONE) {
+    /* A_NONE "exists" but is not allowed as a tech goal.  A_UNSET should
+     * be used instead.  However the above code may prevent the client from
+     * ever setting the goal to A_UNSET, meaning once a goal is set it
+     * can't be removed. */
+    return;
+  }
+
   choose_tech_goal(pplayer, tech);
   send_player_info(pplayer, pplayer);