[Freeciv-Dev] Re: (PR#15840) common/dataio.c:700 is dubious

["Guillaume Melquiond" <guillaume.melquiond@xxxxxxxxx>]

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

On 3/11/06, Jason Short wrote:
> The dataio code is pretty old and has not been closely inspected.  There
> was an error of this type in packets_gen.c that caused garbage data to
> be sent on (I forget exactly) reverse-endian or 64-bit systems.  It
> seems likely this code would also fail in those situations, and must be
> fixed.

While adding the missing fields and fixing the incorrect access, I
noticed a few other problems. As you can see in the patch, the macro
is necessary to achieve the exact same semantic as the normal
dio_get_uint8. But if you take a look at the other deserialization
functions of the file, there are no two functions that have the same
semantic.

For example, dio_get_uint8 will not modify the memory when there is no
data anymore. dio_get_bool8 will store a random boolean, but sometimes
it will store true and log an error message. dio_get_bool32 will store
true. dio_get_sint8 will store a random value (presumably outside of
the sint8 range). dio_get_sint16 will store 0. And so on. As I said,
each function has a slightly different semantic.

I will take a look at fixing this file. But I will first have to
understand where and how it is used (according to grep, there are
almost thousand users). At very first glance, my opinion would be that
each deserialization function should support NULL parameters, and they
should be allowed to locally trash memory when there is no data
anymore.

> As for why those other two fields aren't serialized, it is most likely
> because they were added to the diplstate field after the dataio code was
> written, and because they're not used anywhere in the client there was
> no reason to send them.  Of course there's no reason not to send them
> either, and in the long run it's better to have proper serialization.
>
> Anyway, a patch to fix this would be welcome.

Attached.

Index: common/dataio.c
===================================================================
--- common/dataio.c     (révision 11767)
+++ common/dataio.c     (copie de travail)
@@ -418,6 +418,10 @@
   }
 }
 
+#define dio_get_uint8_noint(din, dest) \
+  if (dest) { int _dgun_dest_ = *dest; dio_get_uint8(din, &_dgun_dest_); *dest 
= _dgun_dest_; } \
+  else dio_get_uint8(din, NULL)
+
 /**************************************************************************
 ...
 **************************************************************************/
@@ -697,18 +701,28 @@
   }
 }
 
+/**************************************************************************
+  De-serialize a player diplomatic state.
+**************************************************************************/
 void dio_get_diplstate(struct data_in *din, struct player_diplstate *pds)
 {
-  dio_get_uint8(din, (int *) &pds->type);
+  dio_get_uint8_noint(din, &pds->type);
+  dio_get_uint8_noint(din, &pds->max_state);
+  dio_get_uint16(din, &pds->first_contact_turn);
   dio_get_uint16(din, &pds->turns_left);
   dio_get_uint16(din, &pds->contact_turns_left);
   dio_get_uint8(din, &pds->has_reason_to_cancel);
 }
 
+/**************************************************************************
+  Serialize a player diplomatic state.
+**************************************************************************/
 void dio_put_diplstate(struct data_out *dout,
                       const struct player_diplstate *pds)
 {
   dio_put_uint8(dout, pds->type);
+  dio_put_uint8(dout, pds->max_state);
+  dio_put_uint16(dout, pds->first_contact_turn);
   dio_put_uint16(dout, pds->turns_left);
   dio_put_uint16(dout, pds->contact_turns_left);
   dio_put_uint8(dout, pds->has_reason_to_cancel);