[Freeciv-Dev] "bad string in packet" messages (PR#272)

[David Pfitzner <dwp@xxxxxxxxxxxxxx>]

Some similar cases were fixed before 1.10.0, but I found that 
there are still more cases where you can get "bad string in 
packet" messages, from using uninitialised strings (ie, the 
string data is not actually used by the server, but is 
included in the packet anyway, causing problems).

In particular I saw cases with setting the Research and 
Goal techs, but looking at the code other cases with 
packet_player_request are also potential problems.

Looking into this, I don't understand why the worklist 
stuff was included into packet_player_request, along with
tax/sci/lux rates, government choice, and tech/goal.
Because we only ever use part of the packet at a time, but
we send the whole lot every time (with unused parts 
containing uninitialised "junk" data).  Using separate 
packets would have avoided the above problem as well as 
using less bandwidth. 

(Actually, this isn't really the fault of the worklist
implementation, since even beforehand the non-rates
stuff could have just been sent as individual values.
The worklist stuff just perpetuated the problem, and
made it significantly worse by happening to contain a
string as part of the data.)

Well, I guess we don't want to change the protocol so 
soon after introducing capstr "+1.10" :-/
But I didn't like trying to find all the problem instances, 
and adding many duplicate lines initialising req.worklist.name 
in non-worklist code.  So the attached patch is a slightly 
hack-ish work-around.

-- David

diff -u -r --exclude-from exclude fc1/common/packets.c 
freeciv-cvs/common/packets.c
--- fc1/common/packets.c        Sat Feb 19 23:32:47 2000
+++ freeciv-cvs/common/packets.c        Fri Feb 25 21:07:19 2000
@@ -1171,6 +1171,17 @@
                               enum packet_type req_type)
 {
   unsigned char buffer[MAX_LEN_PACKET], *cptr;
+
+  /* The following is a hack to avoid changing lots of individual
+     pieces of code which actually only use a part of this packet
+     and ignore the worklist stuff.  IMO the separate uses should
+     use separate packets, to avoid this problem (and would also
+     reduce amount sent).  --dwp
+  */
+  if (req_type != PACKET_PLAYER_WORKLIST) {
+    packet->worklist.name[0] = '\0';
+  }
+  
   cptr=put_uint8(buffer+2, req_type);
   cptr=put_uint8(cptr, packet->tax);
   cptr=put_uint8(cptr, packet->luxury);