[Freeciv-Dev] (PR#6627) clean up the goto route network protocol

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

This patch rewrites send_packet_goto_route and receive_packet_goto_route.

There are four ways in which this part of the code is currently inferior:

1.  The use of goto chunks.  This is intended to prevent DoS, no doubt, 
but it doesn't do much and takes a lot of code.

2.  Some values that only the server needs are initialized by the client 
and sent over the network, which is nonsensical.  This includes 
first_index and last_index values.  The server should just initialize 
these when it receives the packet.

3.  The unit's path (punit->pgr->pos) is actually allocated by the 
network code, then subverted for use by the core server code.  The 
result is that there is no clear sequence of allocs and frees.

4.  The server currently uses a circular buffer to store the goto route. 
  (The implementation of this buffer is problematic, but that's an issue 
for another patch.)  This circular buffer method requires that the 
buffer have an extra, unfilled position.  So you'd think the server 
would just create the buffer one size larger than needed - but no, it's 
the client that does this!  The result is very confusing.  (This is an 
offshoot of #2 and #3, but is so rediculous as to get its own mention.)

This patch fixes all of the above issues.

jason

? core.15382
Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.60
diff -u -r1.60 goto.c
--- client/goto.c       2003/09/21 09:06:43     1.60
+++ client/goto.c       2003/10/24 17:11:21
@@ -449,11 +449,7 @@
   p.unit_id = punit->id;
 
   /* we skip the start position */
-  /* FIXME: but for unknown reason the server discards the last position */
-  p.length = path->length - 1 + 1;
-  p.first_index = 0;
-  p.last_index = p.length - 1;
-  p.pos = fc_malloc(p.length * sizeof(*p.pos));
+  p.length = path->length - 1;
   for (i = 0; i < path->length - 1; i++) {
     p.pos[i].x = path->positions[i + 1].x;
     p.pos[i].y = path->positions[i + 1].y;
@@ -486,11 +482,7 @@
   p.unit_id = punit->id;
 
   /* we skip the start position */
-  /* FIXME: but for unknown reason the server discards the last position */
-  p.length = 2 * (path->length - 1) + 1;
-  p.first_index = 0;
-  p.last_index = p.length - 1;
-  p.pos = fc_malloc(p.length * sizeof(struct map_position));
+  p.length = 2 * (path->length - 1);
   j = 0;
   for (i = 1; i < path->length; i++) {
     p.pos[j].x = path->positions[i].x;
@@ -507,8 +499,6 @@
     j++;
   }
   send_packet_goto_route(&aconnection, &p, ROUTE_PATROL);
-  free(p.pos);
-  p.pos = NULL;
   pf_destroy_path(path);
 }
 
Index: common/capstr.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/capstr.c,v
retrieving revision 1.146
diff -u -r1.146 capstr.c
--- common/capstr.c     2003/10/02 17:54:57     1.146
+++ common/capstr.c     2003/10/24 17:11:21
@@ -81,7 +81,7 @@
                    "+diplomacy2 +citizens_style +root_tech auth " \
                    "+nat_ulimit +retake +goto_pack borders dip " \
                    "+packet_short_unit +unit_occupied endgame_rep " \
-                   "+terr_flags"
+                   "+terr_flags +goto_pack"
 
 /* "+1.14.0" is protocol for 1.14.0 release.
  *
@@ -158,6 +158,8 @@
  *
  * "terr_flags" means terrain flags (with the TER_NO_BARBS flag) have been
  * added.
+ *
+ * "goto_pack" means a revised goto packet protocol.
  */
 
 void init_our_capability(void)
Index: common/packets.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.c,v
retrieving revision 1.260
diff -u -r1.260 packets.c
--- common/packets.c    2003/10/07 18:55:08     1.260
+++ common/packets.c    2003/10/24 17:11:22
@@ -3103,84 +3103,36 @@
   RECEIVE_PACKET_END(packet);
 }
 
-enum packet_goto_route_type {
-  GR_FIRST_MORE, GR_FIRST_LAST, GR_MORE, GR_LAST
-};
-#define GOTO_CHUNK 20
 /**************************************************************************
-Chop the route up and send the pieces one by one.
+  Send a PACKET_GOTO_ROUTE or PACKET_PATROL_ROUTE.
 **************************************************************************/
 int send_packet_goto_route(struct connection *pc,
                           const struct packet_goto_route *packet,
                           enum goto_route_type type)
 {
-  int i;
-  int num_poses = packet->last_index > packet->first_index ?
-    packet->last_index - packet->first_index :
-    packet->length - packet->first_index + packet->last_index;
-  int num_chunks = (num_poses + GOTO_CHUNK - 1) / GOTO_CHUNK;
-  int this_chunk = 1;
-
-  i = packet->first_index;
-  assert(num_chunks > 0);
-  while (i != packet->last_index) {
-    unsigned char buffer[MAX_LEN_PACKET];
-    struct data_out dout;
-    int chunk_pos;
-
-    dio_output_init(&dout, buffer, sizeof(buffer));
-    dio_put_uint16(&dout, 0);
-
-    switch (type) {
-    case ROUTE_GOTO:
-      dio_put_uint8(&dout, PACKET_GOTO_ROUTE);
-      break;
-    case ROUTE_PATROL:
-      dio_put_uint8(&dout, PACKET_PATROL_ROUTE);
-      break;
-    default:
-      die("unknown type %d", type);
-    }
-
-    chunk_pos = 0;
-    if (this_chunk == 1) {
-      if (num_chunks == 1)
-       dio_put_uint8(&dout, GR_FIRST_LAST);
-      else
-       dio_put_uint8(&dout, GR_FIRST_MORE);
-    } else {
-      if (this_chunk == num_chunks)
-       dio_put_uint8(&dout, GR_LAST);
-      else
-       dio_put_uint8(&dout, GR_MORE);
-    }
-
-    while (i != packet->last_index && chunk_pos < GOTO_CHUNK) {
-      dio_put_uint8(&dout, packet->pos[i].x);
-      dio_put_uint8(&dout, packet->pos[i].y);
-      i++; i%=packet->length;
-      chunk_pos++;
-    }
-    /* if we finished fill the last chunk with NOPs */
-    assert(!is_normal_map_pos(MAX_UINT8, MAX_UINT8));
-    for (; chunk_pos < GOTO_CHUNK; chunk_pos++) {
-      dio_put_uint8(&dout, MAX_UINT8);
-      dio_put_uint8(&dout, MAX_UINT8);
-    }
-
-    dio_put_uint16(&dout, packet->unit_id);
-
-    {
-      size_t size = dio_output_used(&dout);
+  enum packet_type packet_type;
 
-      dio_output_rewind(&dout);
-      dio_put_uint16(&dout, size);
-      send_packet_data(pc, buffer, size);
-    }
-    this_chunk++;
+  if (type == ROUTE_GOTO) {
+    packet_type = PACKET_GOTO_ROUTE;
+  } else if (type == ROUTE_PATROL) {
+    packet_type = PACKET_PATROL_ROUTE;
+  } else {
+    assert(0);
   }
 
-  return 0;
+  {
+    SEND_PACKET_START(packet_type) {
+      int i;
+
+      dio_put_uint16(&dout, packet->unit_id);
+      dio_put_uint16(&dout, packet->length);
+
+      for (i = 0; i < packet->length; i++) {
+       dio_put_uint8(&dout, packet->pos[i].x);
+       dio_put_uint8(&dout, packet->pos[i].y);
+      }
+    } SEND_PACKET_END;
+  }
 }
 
 /**************************************************************************
@@ -3189,89 +3141,21 @@
 **************************************************************************/
 struct packet_goto_route *receive_packet_goto_route(struct connection *pc)
 {
-  struct data_in din;
-  int i, num_valid = 0;
-  enum packet_goto_route_type type;
-  struct map_position pos[GOTO_CHUNK];
-  struct map_position *pos2;
-  struct packet_goto_route *packet;
-  int length, unit_id;
-
-  dio_input_init(&din, pc->buffer->data, pc->buffer->ndata);
-  dio_get_uint16(&din, NULL);
-  dio_get_uint8(&din, NULL);
-
-  dio_get_uint8(&din, &i);
-  type = (enum packet_goto_route_type) i; /* safe conversion from integer */
-  for (i = 0; i < GOTO_CHUNK; i++) {
-    dio_get_uint8(&din, &pos[i].x);
-    dio_get_uint8(&din, &pos[i].y);
-    if (is_normal_map_pos(pos[i].x, pos[i].y)) {
-      num_valid++;
-    }
-  }
-  dio_get_uint16(&din, &unit_id);
+  RECEIVE_PACKET_START(packet_goto_route, packet) {
+    int i;
 
-  check_packet(&din, pc);
-  remove_packet_from_buffer(pc->buffer);
+    dio_get_uint16(&din, &packet->unit_id);
+    dio_get_uint16(&din, &packet->length);
 
-  /* sanity check */
-  if (!pc->route)
-    pc->route_length = 0;
-
-  switch (type) {
-  case GR_FIRST_MORE:
-    free(pc->route);
-    pc->route = fc_malloc(GOTO_CHUNK * sizeof(struct map_position));
-    pc->route_length = GOTO_CHUNK;
-    for (i = 0; i < GOTO_CHUNK; i++) {
-      pc->route[i].x = pos[i].x;
-      pc->route[i].y = pos[i].y;
+    if (packet->length > MAX_LEN_ROUTE) {
+      packet->length = MAX_LEN_ROUTE;
     }
-    return NULL;
-  case GR_LAST:
-    packet = fc_malloc(sizeof(struct packet_goto_route));
-    packet->unit_id = unit_id;
-    length = pc->route_length+num_valid+1;
-    if (!pc->route)
-      freelog(LOG_ERROR, "Got a GR_LAST packet with NULL without previous 
route");
-    packet->pos = fc_malloc(length * sizeof(struct map_position));
-    packet->length = length;
-    packet->first_index = 0;
-    packet->last_index = length-1;
-    for (i = 0; i < pc->route_length; i++)
-      packet->pos[i] = pc->route[i];
-    for (i = 0; i < num_valid; i++)
-      packet->pos[i+pc->route_length] = pos[i];
-    free(pc->route);
-    pc->route = NULL;
-    return packet;
-  case GR_FIRST_LAST:
-    packet = fc_malloc(sizeof(struct packet_goto_route));
-    packet->unit_id = unit_id;
-    packet->pos = fc_malloc((num_valid+1) * sizeof(struct map_position));
-    packet->length = num_valid + 1;
-    packet->first_index = 0;
-    packet->last_index = num_valid;
-    for (i = 0; i < num_valid; i++)
-      packet->pos[i] = pos[i];
-    return packet;
-  case GR_MORE:
-    pos2 = fc_malloc((GOTO_CHUNK+pc->route_length) * sizeof(struct 
map_position));
-    if (!pc->route)
-      freelog(LOG_ERROR, "Got a GR_MORE packet with NULL without previous 
route");
-    for (i = 0; i < pc->route_length; i++)
-      pos2[i] = pc->route[i];
-    for (i = 0; i < GOTO_CHUNK; i++)
-      pos2[i+pc->route_length] = pos[i];
-    free(pc->route);
-    pc->route = pos2;
-    pc->route_length += GOTO_CHUNK;
-    return NULL;
-  default:
-    freelog(LOG_ERROR, "invalid type in receive_packet_goto_route()");
-    return NULL;
-  }
+
+    for (i = 0; i < packet->length; i++) {
+      dio_get_uint8(&din, &packet->pos[i].x);
+      dio_get_uint8(&din, &packet->pos[i].y);
+    }
+  } RECEIVE_PACKET_END(packet);
 }
 
 /**************************************************************************
Index: common/packets.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.h,v
retrieving revision 1.156
diff -u -r1.156 packets.h
--- common/packets.h    2003/10/07 18:55:09     1.156
+++ common/packets.h    2003/10/24 17:11:22
@@ -26,6 +26,7 @@
 #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          1200      /* MAX_LEN_PACKET/3 - header */
 
 /* Note that MAX_LEN_USERNAME cannot be expanded, because it
    is used for the name in the first packet sent by the client,
@@ -957,11 +958,9 @@
 
 struct packet_goto_route
 {
-  int length;
-  int first_index;
-  int last_index;
-  struct map_position *pos;
   int unit_id;
+  int length;
+  struct map_position pos[MAX_LEN_ROUTE];
 };
 
 struct packet_attribute_chunk
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.275
diff -u -r1.275 unithand.c
--- server/unithand.c   2003/10/08 16:56:07     1.275
+++ server/unithand.c   2003/10/24 17:11:22
@@ -1539,21 +1539,10 @@
 {
   struct unit *punit = player_find_unit_by_id(pplayer, packet->unit_id);
 
-  if (!punit) {
+  if (!punit || packet->length < 1) {
     return FALSE;
   }
 
-  if (packet->first_index != 0) {
-    freelog(LOG_ERROR, "Bad route packet, first_index is %d!",
-           packet->first_index);
-    return FALSE;
-  }
-  if (packet->last_index != packet->length - 1) {
-    freelog(LOG_ERROR, "bad route, li: %d != l-1: %d",
-           packet->last_index, packet->length - 1);
-    return FALSE;
-  }
-
   return TRUE;
 }
 
@@ -1564,28 +1553,33 @@
 {
   struct goto_route *pgr = NULL;
   struct unit *punit = player_find_unit_by_id(pplayer, packet->unit_id);
+  int i;
 
   if (punit->pgr) {
     free(punit->pgr->pos);
     free(punit->pgr);
   }
 
+  /* pgr->pos is implemented as a circular buffer of positions, and this
+   * circular buffer requires an extra "empty" spot.  Hence we increase the
+   * length by one. */
   pgr = fc_malloc(sizeof(struct goto_route));
-
-  pgr->pos = packet->pos; /* here goes the malloced packet->pos */
+  pgr->length = packet->length + 1;
   pgr->first_index = 0;
-  pgr->length = packet->length;
-  pgr->last_index = packet->length-1; /* index magic... */
+  pgr->last_index = packet->length;
+  pgr->pos = fc_malloc(pgr->length * sizeof(*pgr->pos));
+
+  for (i = 0; i < packet->length; i++) {
+    pgr->pos[i] = packet->pos[i];
+  }
 
   punit->pgr = pgr;
 
 #ifdef DEBUG
-  {
-    int i = pgr->first_index;
-    freelog(LOG_DEBUG, "first:%d, last:%d, length:%d",
-          pgr->first_index, pgr->last_index, pgr->length);
-    for (; i < pgr->last_index;i++)
-      freelog(LOG_DEBUG, "%d,%d", pgr->pos[i].x, pgr->pos[i].y);
+  freelog(LOG_DEBUG, "first:%d, last:%d, length:%d",
+         pgr->first_index, pgr->last_index, pgr->length);
+  for (i = pgr->first_index; i < pgr->last_index; i++) {
+    freelog(LOG_DEBUG, "  %d,%d", pgr->pos[i].x, pgr->pos[i].y);
   }
 #endif
 
@@ -1621,8 +1615,6 @@
   struct unit *punit = player_find_unit_by_id(pplayer, packet->unit_id);
 
   if (!check_route(pplayer, packet)) {
-    free(packet->pos);
-    packet->pos = NULL;
     return;
   }