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

While I have not delved deeply enough to discern the underlying cause of
attributes with bad lengths (probably a code path with an uninitialized
variable), here is a patch to allow the game to proceed without crashing.

Basically, whenever you have data coming from an external source, you
always should verify the form!  Even on today's machines, there will always
be bit flips and corruption.

This checks the data at the savefile and network levels.

Of course, there's also the problem that each client can send up to
256KB of data!  That's not a good design....  It really doesn't scale, as
the game gets longer, it will get slower and slower!  (Another bug for
another day.)

Index: server/savegame.c
===================================================================
--- server/savegame.c   (revision 13067)
+++ server/savegame.c   (working copy)
@@ -2540,24 +2540,39 @@
 
   load_player_units(plr, plrno, file);
 
-  if (section_file_lookup(file, "player%d.attribute_v2_block_length", plrno)) {
-    int raw_length1, raw_length2, part_nr, parts;
+  /* Toss any existing attribute_block (should not exist) */
+  if (plr->attribute_block.data) {
+    free(plr->attribute_block.data);
+    plr->attribute_block.data = NULL;
+  }
+
+  /* This is a big heap of opaque data for the client, check everything! */
+  plr->attribute_block.length = secfile_lookup_int_default(
+      file, 0, "player%d.attribute_v2_block_length", plrno);
+
+  if (0 > plr->attribute_block.length) {
+    freelog(LOG_ERROR, "player%d.attribute_v2_block_length=%d too small",
+            plrno,
+            plr->attribute_block.length);
+    plr->attribute_block.length = 0;
+  } else if (MAX_ATTRIBUTE_BLOCK < plr->attribute_block.length) {
+    freelog(LOG_ERROR, "player%d.attribute_v2_block_length=%d too big (max 
%d)",
+            plrno,
+            plr->attribute_block.length,
+            MAX_ATTRIBUTE_BLOCK);
+    plr->attribute_block.length = 0;
+  } else if (0 < plr->attribute_block.length) {
+    int part_nr, parts;
+    size_t actual_length;
     size_t quoted_length;
     char *quoted;
 
-    raw_length1 =
-       secfile_lookup_int(file, "player%d.attribute_v2_block_length", plrno);
-    if (plr->attribute_block.data) {
-      free(plr->attribute_block.data);
-      plr->attribute_block.data = NULL;
-    }
-    plr->attribute_block.data = fc_malloc(raw_length1);
-    plr->attribute_block.length = raw_length1;
+    plr->attribute_block.data = fc_malloc(plr->attribute_block.length);
 
     quoted_length = secfile_lookup_int
        (file, "player%d.attribute_v2_block_length_quoted", plrno);
     quoted = fc_malloc(quoted_length + 1);
-    quoted[0] = 0;
+    quoted[0] = '\0';
 
     parts =
        secfile_lookup_int(file, "player%d.attribute_v2_block_parts", plrno);
@@ -2566,9 +2581,13 @@
       char *current = secfile_lookup_str(file,
                                         
"player%d.attribute_v2_block_data.part%d",
                                         plrno, part_nr);
-      if (!current)
+      if (!current) {
+        freelog(LOG_ERROR, "attribute_v2_block_parts=%d actual=%d",
+                parts,
+                part_nr);
        break;
-      freelog(LOG_DEBUG, "quoted_length=%lu quoted=%lu current=%lu",
+      }
+      freelog(LOG_DEBUG, "attribute_v2_block_length_quoted=%lu have=%lu 
part=%lu",
              (unsigned long) quoted_length,
              (unsigned long) strlen(quoted),
              (unsigned long) strlen(current));
@@ -2576,17 +2595,17 @@
       strcat(quoted, current);
     }
     if (quoted_length != strlen(quoted)) {
-      freelog(LOG_DEBUG, "quoted_length=%lu quoted=%lu",
+      freelog(LOG_FATAL, "attribute_v2_block_length_quoted=%lu actual=%lu",
              (unsigned long) quoted_length,
              (unsigned long) strlen(quoted));
       assert(0);
     }
 
-    raw_length2 =
+    actual_length =
        unquote_block(quoted,
                      plr->attribute_block.data,
                      plr->attribute_block.length);
-    assert(raw_length1 == raw_length2);
+    assert(actual_length == plr->attribute_block.length);
     free(quoted);
   }
 }
@@ -3378,41 +3397,77 @@
     secfile_insert_int(file, i, "player%d.total_ncities", plrno);
   }
 
-#define PART_SIZE (2*1024)
+  /* This is a big heap of opaque data from the client.  Although the binary
+   * format is not user editable, keep the lines short enough for debugging,
+   * and hope that data compression will keep the file a reasonable size.
+   * Note that the "quoted" format is a multiple of 3.
+   */
+#define PART_SIZE (3*256)
   if (plr->attribute_block.data) {
     char *quoted = quote_block(plr->attribute_block.data,
                               plr->attribute_block.length);
+    char *quoted_at = strchr(quoted, ':');
+    size_t bytes_left = strlen(quoted);
+    size_t bytes_at_colon = 1 + (quoted_at - quoted);
+    size_t bytes_adjust = bytes_at_colon % 3;
+    int current_part_nr;
+    int parts;
     char part[PART_SIZE + 1];
-    int current_part_nr, parts;
-    size_t bytes_left;
 
     secfile_insert_int(file, plr->attribute_block.length,
                       "player%d.attribute_v2_block_length", plrno);
-    secfile_insert_int(file, strlen(quoted),
+    secfile_insert_int(file, bytes_left,
                       "player%d.attribute_v2_block_length_quoted", plrno);
 
-    parts = (strlen(quoted) - 1) / PART_SIZE + 1;
-    bytes_left = strlen(quoted);
+    /* Try to wring some compression efficiencies out of the "quoted" format.
+     * The first line has a variable length decimal, mis-aligning triples.
+     */
+    if ((bytes_left - bytes_adjust) > PART_SIZE) {
+      /* first line can be longer */
+      parts = 1 + (bytes_left - bytes_adjust - 1) / PART_SIZE;
+    } else {
+      parts = 1;
+    }
 
     secfile_insert_int(file, parts,
                       "player%d.attribute_v2_block_parts", plrno);
 
-    for (current_part_nr = 0; current_part_nr < parts; current_part_nr++) {
+    if (parts > 1) {
+      size_t size_of_current_part = PART_SIZE + bytes_adjust;
+
+      /* first line can be longer */
+      memcpy(part, quoted, size_of_current_part);
+      part[size_of_current_part] = '\0';
+      secfile_insert_str(file, part,
+                        "player%d.attribute_v2_block_data.part%d",
+                        plrno,
+                        0);
+      bytes_left -= size_of_current_part;
+      quoted_at = &quoted[size_of_current_part];
+      current_part_nr = 1;
+    } else {
+      quoted_at = quoted;
+      current_part_nr = 0;
+    }
+
+    for (; current_part_nr < parts; current_part_nr++) {
       size_t size_of_current_part = MIN(bytes_left, PART_SIZE);
 
       assert(bytes_left);
 
-      memcpy(part, quoted + PART_SIZE * current_part_nr,
-            size_of_current_part);
-      part[size_of_current_part] = 0;
+      memcpy(part, quoted_at, size_of_current_part);
+      part[size_of_current_part] = '\0';
       secfile_insert_str(file, part,
-                        "player%d.attribute_v2_block_data.part%d", plrno,
+                        "player%d.attribute_v2_block_data.part%d",
+                        plrno,
                         current_part_nr);
       bytes_left -= size_of_current_part;
+      quoted_at = &quoted_at[size_of_current_part];
     }
     assert(bytes_left == 0);
     free(quoted);
   }
+#undef PART_SIZE
 }
 
 
Index: common/packets.h
===================================================================
--- common/packets.h    (revision 13067)
+++ common/packets.h    (working copy)
@@ -32,10 +32,17 @@
 
 #define MAX_LEN_USERNAME        10        /* see below */
 #define MAX_LEN_MSG             1536
-#define MAX_ATTRIBUTE_BLOCK     (256*1024)     /* largest attribute block */
-#define ATTRIBUTE_CHUNK_SIZE    (1024*2)  /* attribute chunk size to use */
 #define MAX_LEN_ROUTE          2000      /* MAX_LEN_PACKET/2 - header */
 
+/* The size of opaque (void *) data sent in the network packet.  To avoid
+ * fragmentation issues, this SHOULD NOT be larger than the standard
+ * ethernet or PPP 1500 byte frame size (with room for headers).
+ *
+ * Do not spend much time optimizing, you have no idea of the actual dynamic
+ * path characteristics between systems, such as VPNs and tunnels.
+ */
+#define ATTRIBUTE_CHUNK_SIZE    (1400)
+
 enum report_type {
   REPORT_WONDERS_OF_THE_WORLD,
   REPORT_TOP_5_CITIES,
Index: common/player.h
===================================================================
--- common/player.h     (revision 13067)
+++ common/player.h     (working copy)
@@ -145,6 +145,13 @@
 };
 
 BV_DEFINE(bv_debug, PLAYER_DEBUG_LAST);
+
+struct attribute_block_s {
+  void *data;
+  int length;
+#define MAX_ATTRIBUTE_BLOCK     (256*1024)     /* largest attribute block */
+};
+
 struct player {
   int player_no;
   char name[MAX_LEN_NAME];
@@ -188,14 +195,9 @@
   int small_wonders[B_LAST];              /* contains city id's */
          /* wonders[] may also be (-1), or the id of a city
            which no longer exists, if the wonder has been destroyed */
-  struct {
-    int length;
-    void *data;
-  } attribute_block;
-  struct {
-    int length;
-    void *data;
-  } attribute_block_buffer;
+
+  struct attribute_block_s attribute_block;
+  struct attribute_block_s attribute_block_buffer;
   bv_debug debug;
 };
 
Index: common/game.c
===================================================================
--- common/game.c       (revision 13067)
+++ common/game.c       (working copy)
@@ -491,14 +491,14 @@
   if (pplayer->attribute_block.data) {
     free(pplayer->attribute_block.data);
     pplayer->attribute_block.data = NULL;
-    pplayer->attribute_block.length = 0;
   }
+  pplayer->attribute_block.length = 0;
 
   if (pplayer->attribute_block_buffer.data) {
     free(pplayer->attribute_block_buffer.data);
     pplayer->attribute_block_buffer.data = NULL;
-    pplayer->attribute_block_buffer.length = 0;
   }
+  pplayer->attribute_block_buffer.length = 0;
 
 
 #if 0
Index: common/packets.c
===================================================================
--- common/packets.c    (revision 13067)
+++ common/packets.c    (working copy)
@@ -51,7 +51,7 @@
 #define JUMBO_BORDER           (64*1024-COMPRESSION_BORDER-1)
 #endif
 
-#define BASIC_PACKET_LOG_LEVEL LOG_DEBUG
+#define BASIC_PACKET_LOG_LEVEL LOG_VERBOSE
 #define COMPRESS_LOG_LEVEL     LOG_DEBUG
 #define COMPRESS2_LOG_LEVEL    LOG_DEBUG
 
@@ -94,7 +94,7 @@
     pc->client.last_request_id_used =
        get_next_request_id(pc->client.last_request_id_used);
     result = pc->client.last_request_id_used;
-    freelog(LOG_DEBUG, "sending request %d", result);
+    freelog(BASIC_PACKET_LOG_LEVEL, "sending request %d", result);
   }
 
   if (pc->outgoing_packet_notify) {
@@ -558,8 +558,10 @@
                                           packet_player_attribute_chunk
                                           *chunk)
 {
-  freelog(LOG_DEBUG, "received attribute chunk %d/%d %d", chunk->offset,
-         chunk->total_length, chunk->chunk_length);
+  freelog(BASIC_PACKET_LOG_LEVEL, "received attribute chunk %u/%u %u",
+         (unsigned int) chunk->offset,
+         (unsigned int) chunk->total_length,
+         (unsigned int) chunk->chunk_length);
 
   if (chunk->total_length < 0
       || chunk->chunk_length < 0
@@ -683,6 +685,9 @@
   }
 }
 
+/**************************************************************************
+  Test and log for sending player attribute_block
+**************************************************************************/
 void pre_send_packet_player_attribute_chunk(struct connection *pc,
                                            struct packet_player_attribute_chunk
                                            *packet)
@@ -695,16 +700,23 @@
   assert(packet->chunk_length <= packet->total_length);
   assert(packet->offset >= 0 && packet->offset < packet->total_length);
 
-  freelog(LOG_DEBUG, "sending attribute chunk %d/%d %d", packet->offset,
-         packet->total_length, packet->chunk_length);
+  freelog(BASIC_PACKET_LOG_LEVEL, "sending attribute chunk %d/%d %d",
+         packet->offset, packet->total_length, packet->chunk_length);
 
 }
 
+/**************************************************************************
+  ...
+**************************************************************************/
 void post_receive_packet_game_state(struct connection *pc,
                                    struct packet_game_state *packet)
 {
   conn_clear_packet_cache(pc);
 }
+
+/**************************************************************************
+  ...
+**************************************************************************/
 void post_send_packet_game_state(struct connection *pc,
                                 const struct packet_game_state *packet)
 {
Index: client/packhand.c
===================================================================
--- client/packhand.c   (revision 13067)
+++ client/packhand.c   (working copy)
@@ -2296,7 +2296,7 @@
     impr_type_iterate(id) {
       b = improvement_by_number(id);
       freelog(LOG_DEBUG, "Impr: %s...",
-             b->name);
+             advance_rule_name(id));
       if (tech_exists(b->obsolete_by)) {
        freelog(LOG_DEBUG, "  obsolete_by %2d/%s",
                b->obsolete_by,
@@ -2585,7 +2585,7 @@
   }
 
   pl->num_groups = p->ngroups;
-  pl->groups = fc_malloc(sizeof(*(pl->groups)) * pl->num_groups);
+  pl->groups = fc_calloc(sizeof(*(pl->groups)), pl->num_groups);
   for (i = 0; i < p->ngroups; i++) {
     pl->groups[i] = nation_group_by_number(p->groups[i]);
     if (!pl->groups[i]) {
Index: client/attribute.c
===================================================================
--- client/attribute.c  (revision 13067)
+++ client/attribute.c  (working copy)
@@ -37,6 +37,13 @@
   int key, id, x, y;
 };
 
+enum attribute_serial {
+  A_SERIAL_FAIL,
+  A_SERIAL_OK,
+  A_SERIAL_OLD,
+};
+
+
 /****************************************************************************
  Hash function for attribute_hash.
 *****************************************************************************/
@@ -91,8 +98,8 @@
  sizeof(int) at serialization time is different from sizeof(int) at
  deserialization time.
 *****************************************************************************/
-static void serialize_hash(struct hash_table *hash, void **pdata,
-                          int *pdata_length)
+static enum attribute_serial serialize_hash( struct hash_table *hash,
+                                       void **pdata, int *pdata_length )
 {
   /*
    * Layout of version 2:
@@ -181,15 +188,22 @@
   *pdata = result;
   *pdata_length = total_length;
   free(value_lengths);
-  freelog(LOG_DEBUG, "serialized %d entries in %d bytes", entries,
-         total_length);
+  freelog(ATTRIBUTE_LOG_LEVEL,
+          "attribute.c serialize_hash()"
+          " serialized %u entries in %u bytes",
+          (unsigned int) entries,
+          (unsigned int) total_length);
+  return A_SERIAL_OK;
 }
 
 /****************************************************************************
-...
+  This data was serialized (above), sent as an opaque data packet to the 
+  server, stored in a savegame, retrieved from the savegame, sent as an 
+  opaque data packet back to the client, and now is ready to be restored.
+  Check everything!
 *****************************************************************************/
-static bool unserialize_hash(struct hash_table *hash, void *data,
-                            size_t data_length)
+static enum attribute_serial unserialize_hash( struct hash_table *hash,
+                                       void *data, size_t data_length )
 {
   int entries, i, dummy;
   struct data_in din;
@@ -200,18 +214,36 @@
 
   dio_get_uint32(&din, &dummy);
   if (dummy != 0) {
-    return FALSE;
+    freelog(LOG_VERBOSE,
+            "attribute.c unserialize_hash() preamble,"
+            " uint32 %u != 0",
+            (unsigned int) dummy);
+    return A_SERIAL_OLD;
   }
   dio_get_uint8(&din, &dummy);
   if (dummy != 2) {
-    return FALSE;
+    freelog(LOG_VERBOSE,
+            "attribute.c unserialize_hash() preamble,"
+            " uint8 %u != 2 version",
+            (unsigned int) dummy);
+    return A_SERIAL_OLD;
   }
   dio_get_uint32(&din, &entries);
   dio_get_uint32(&din, &dummy);
-  assert(data_length == dummy);
+  if (dummy != data_length) {
+    freelog(LOG_VERBOSE,
+            "attribute.c unserialize_hash() preamble,"
+            " uint32 %u != %u data_length",
+            (unsigned int) dummy,
+            (unsigned int) data_length);
+    return A_SERIAL_FAIL;
+  }
 
-  freelog(LOG_DEBUG, "try to unserialized %d entries from %d bytes",
-         entries, (unsigned int) data_length);
+  freelog(ATTRIBUTE_LOG_LEVEL,
+          "attribute.c unserialize_hash()"
+          " uint32 %u entries, %u data_length",
+          (unsigned int) entries,
+          (unsigned int) data_length);
 
   for (i = 0; i < entries; i++) {
     struct attr_key *pkey = fc_malloc(sizeof(*pkey));
@@ -220,14 +252,50 @@
     struct data_out dout;
 
     dio_get_uint32(&din, &value_length);
-    
-    pvalue = fc_malloc(value_length + 4);
-    
+    if (din.too_short) {
+      freelog(LOG_VERBOSE,
+              "attribute.c unserialize_hash()"
+              " uint32 value_length dio_input_too_short");
+      free(pkey);
+      return A_SERIAL_FAIL;
+    }
+    if (value_length > dio_input_remaining(&din)) {
+      freelog(LOG_VERBOSE,
+              "attribute.c unserialize_hash()"
+              " uint32 %u value_length > %u input_remaining",
+              (unsigned int) value_length,
+              (unsigned int) dio_input_remaining(&din));
+      free(pkey);
+      return A_SERIAL_FAIL;
+    }
+    if (value_length < 16 /* including itself */) {
+      freelog(LOG_VERBOSE,
+              "attribute.c unserialize_hash()"
+              " uint32 %u value_length < 16",
+              (unsigned int) value_length);
+      free(pkey);
+      return A_SERIAL_FAIL;
+    }
+    freelog(ATTRIBUTE_LOG_LEVEL,
+            "attribute.c unserialize_hash()"
+            " uint32 %u value_length",
+            (unsigned int) value_length);
+
+    /* next 12 bytes */
     dio_get_uint32(&din, &pkey->key);
     dio_get_uint32(&din, &pkey->id);
     dio_get_sint16(&din, &pkey->x);
     dio_get_sint16(&din, &pkey->y);
 
+    if (din.too_short) {
+      freelog(LOG_VERBOSE,
+              "attribute.c unserialize_hash()"
+              " uint32 key dio_input_too_short");
+      free(pkey);
+      return A_SERIAL_FAIL;
+    }
+    pvalue = fc_malloc(value_length + 4);
+
     dio_output_init(&dout, pvalue, 4);
     dio_put_uint32(&dout, value_length);
     dio_get_memory(&din, ADD_TO_POINTER(pvalue, 4), value_length);
@@ -238,15 +306,13 @@
        * to delete all attributes.  Another symptom of the bug is the
        * value_length (above) is set to a random value, which can also
        * cause a bug. */
-      freelog(LOG_ERROR, _("There has been a CMA error.  "
-                          "Your citizen governor settings may be broken."));
       free(pvalue);
       free(pkey);
       hash_delete_all_entries(hash);
-      return FALSE;
+      return A_SERIAL_FAIL;
     }
   }
-  return TRUE;
+  return A_SERIAL_OK;
 }
 
 /****************************************************************************
@@ -290,10 +356,19 @@
 
   assert(attribute_hash != NULL);
 
-  if (!unserialize_hash(attribute_hash, pplayer->attribute_block.data,
-                       pplayer->attribute_block.length)) {
-    freelog(LOG_ERROR, _("Old attributes detected and removed."));
-  }
+  switch (unserialize_hash(attribute_hash,
+                           pplayer->attribute_block.data,
+                           pplayer->attribute_block.length)) {
+  case A_SERIAL_FAIL:
+    freelog(LOG_ERROR, _("There has been a CMA error.  "
+                         "Your citizen governor settings may be broken."));
+    break;
+  case A_SERIAL_OLD:
+    freelog(LOG_NORMAL, _("Old attributes detected and removed."));
+    break;
+  default:
+    break;
+  };
 }
 
 /****************************************************************************
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to