[Freeciv-Dev] (PR#10957) character checking on names (leaders and cities)

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=10957 >

Leaders and cities cannot be longer than MAX_LEN_NAME.  This is measured 
in bytes because they use a fixed-sized buffer.

Fixing this is ugly.  We have to malloc and free a name which may be of 
variable size.  We have to check the character length of names.  And 
worst of all we have to fit a variable-sized string into a network 
packet - which is impossible so I just estimated that it could be 5x as 
long as a normal string (hopefully this doesn't overflow the max packet 
length).

Is this worth it?  I can't say.  The current method works okay, but 
strings using non-ascii characters are limited to fewer characters than 
those with only ascii.  However this isn't a fatal error.

It is possible to use UCS entirely for our internal encoding.  This is 
what Raimar (and for a time, Vasco) wanted.  It would require tremendous 
changes.

jason

Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.414
diff -u -r1.414 packhand.c
--- client/packhand.c   4 Nov 2004 21:07:17 -0000       1.414
+++ client/packhand.c   9 Nov 2004 18:21:02 -0000
@@ -424,11 +424,12 @@
       update_descriptions = TRUE;
     }
     assert(pcity->id == packet->id);
+    free(pcity->name);
+    pcity->name = mystrdup(packet->name);
   }
   
   pcity->owner=packet->owner;
   pcity->tile = map_pos_to_tile(packet->x, packet->y);
-  sz_strlcpy(pcity->name, packet->name);
   
   pcity->size = packet->size;
   for (i = 0; i < 5; i++) {
@@ -1485,7 +1486,7 @@
   char msg[MAX_LEN_MSG];
   struct player *pplayer = &game.players[pinfo->playerno];
 
-  sz_strlcpy(pplayer->name, pinfo->name);
+  set_player_name(pplayer, pinfo->name);
 
   pplayer->nation=pinfo->nation;
   pplayer->is_male=pinfo->is_male;
Index: common/city.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/city.c,v
retrieving revision 1.252
diff -u -r1.252 city.c
--- common/city.c       19 Oct 2004 06:46:57 -0000      1.252
+++ common/city.c       9 Nov 2004 18:21:02 -0000
@@ -2396,7 +2396,7 @@
   pcity->id = 0;
   pcity->owner = pplayer->player_no;
   pcity->tile = ptile;
-  sz_strlcpy(pcity->name, name);
+  pcity->name = mystrdup(name);
   pcity->size = 1;
   specialist_type_iterate(sp) {
     pcity->specialists[sp] = 0;
@@ -2489,6 +2489,7 @@
 **************************************************************************/
 void remove_city_virtual(struct city *pcity)
 {
+  free(pcity->name);
   unit_list_unlink_all(&pcity->units_supported);
   free(pcity);
 }
Index: common/city.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/city.h,v
retrieving revision 1.165
diff -u -r1.165 city.h
--- common/city.h       20 Oct 2004 18:20:53 -0000      1.165
+++ common/city.h       9 Nov 2004 18:21:02 -0000
@@ -218,7 +218,7 @@
   int id;
   int owner;
   struct tile *tile;
-  char name[MAX_LEN_NAME];
+  char *name; /* Allocated & freed. */
 
   /* the people */
   int size;
Index: common/packets.def
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/packets.def,v
retrieving revision 1.58
diff -u -r1.58 packets.def
--- common/packets.def  4 Nov 2004 03:02:10 -0000       1.58
+++ common/packets.def  9 Nov 2004 18:21:02 -0000
@@ -390,7 +390,7 @@
   CITY id; key
   PLAYER owner;
   COORD x,y;
-  STRING name[MAX_LEN_NAME];
+  STRING name[5 * MAX_LEN_NAME]; /* Arbitrarily long buffer */
   UINT8 size;
 
   UINT8 ppl_happy[5], ppl_content[5], ppl_unhappy[5], ppl_angry[5];
@@ -435,7 +435,7 @@
   PLAYER owner;
   COORD x;
   COORD y;
-  STRING name[MAX_LEN_NAME];
+  STRING name[5 * MAX_LEN_NAME]; /* Arbitrarily large buffer. */
   UINT8 size;
   BOOL happy;
   BOOL unhappy;
@@ -1123,7 +1123,8 @@
   TECH_LIST init_techs[MAX_NUM_TECH_LIST];
 
   UINT8 leader_count;
-  STRING leader_name[MAX_NUM_LEADERS:leader_count][MAX_LEN_NAME];
+  /* Arbitrarily large buffer. */
+  STRING leader_name[MAX_NUM_LEADERS:leader_count][5 * MAX_LEN_NAME];
   BOOL leader_sex[MAX_NUM_LEADERS:leader_count];
 end
 
Index: common/player.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.c,v
retrieving revision 1.159
diff -u -r1.159 player.c
--- common/player.c     23 Oct 2004 20:02:52 -0000      1.159
+++ common/player.c     9 Nov 2004 18:21:02 -0000
@@ -88,7 +88,7 @@
 
   plr->player_no=plr-game.players;
 
-  sz_strlcpy(plr->name, ANON_PLAYER_NAME);
+  plr->name = mystrdup(ANON_USER_NAME);
   sz_strlcpy(plr->username, ANON_USER_NAME);
   plr->is_male = TRUE;
   plr->government=game.default_government;
@@ -151,6 +151,16 @@
   plr->debug = FALSE;
 }
 
+/****************************************************************************
+  Change the player's name.
+****************************************************************************/
+void set_player_name(struct player *pplayer, const char *name)
+{
+  assert(string_character_length(name) < MAX_LEN_NAME);
+  free(pplayer->name);
+  pplayer->name = mystrdup(name);
+}
+
 /***************************************************************
 ...
 ***************************************************************/
Index: common/player.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/player.h,v
retrieving revision 1.131
diff -u -r1.131 player.h
--- common/player.h     23 Oct 2004 20:02:52 -0000      1.131
+++ common/player.h     9 Nov 2004 18:21:03 -0000
@@ -178,7 +178,7 @@
 
 struct player {
   int player_no;
-  char name[MAX_LEN_NAME];
+  char *name; /* Alocated & freed. */
   char username[MAX_LEN_NAME];
   bool is_male;
   int government;
@@ -227,6 +227,7 @@
 };
 
 void player_init(struct player *plr);
+void set_player_name(struct player *pplayer, const char *name);
 struct player *find_player_by_name(const char *name);
 struct player *find_player_by_name_prefix(const char *name,
                                          enum m_pre_result *result);
Index: server/cityhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityhand.c,v
retrieving revision 1.137
diff -u -r1.137 cityhand.c
--- server/cityhand.c   29 Sep 2004 02:24:23 -0000      1.137
+++ server/cityhand.c   9 Nov 2004 18:21:03 -0000
@@ -383,7 +383,8 @@
     return;
   }
 
-  sz_strlcpy(pcity->name, name);
+  free(pcity->name);
+  pcity->name = mystrdup(name);
   city_refresh(pcity);
   send_city_info(NULL, pcity);
 }
Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.278
diff -u -r1.278 citytools.c
--- server/citytools.c  31 Oct 2004 22:00:44 -0000      1.278
+++ server/citytools.c  9 Nov 2004 18:21:03 -0000
@@ -256,6 +256,15 @@
 bool is_allowed_city_name(struct player *pplayer, const char *city_name,
                          char *error_buf, size_t bufsz)
 {
+  if (string_character_length(city_name) >= MAX_LEN_NAME) {
+    /* Cities may not have more than MAX_LEN_NAME characters. */
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz,
+                 _("City name %s is too long."), city_name);
+    }
+    return FALSE;
+  }
+
   /* Mode 1: A city name has to be unique for each player. */
   if (game.allowed_city_names == 1 &&
       city_list_find_name(&pplayer->cities, city_name)) {
Index: server/ruleset.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/ruleset.c,v
retrieving revision 1.197
diff -u -r1.197 ruleset.c
--- server/ruleset.c    23 Oct 2004 18:54:40 -0000      1.197
+++ server/ruleset.c    9 Nov 2004 18:21:04 -0000
@@ -2208,13 +2208,12 @@
     } /* if (name) */
     remove_leading_trailing_spaces(cities[j]);
     city_names[j].name = mystrdup(cities[j]);
-    if (check_name(city_names[j].name)) {
+    if (string_character_length(city_names[j].name) >= MAX_LEN_NAME) {
       /* The ruleset contains a name that is too long.  This shouldn't
         happen - if it does, the author should get immediate feedback */
-      freelog(LOG_ERROR, "City name %s in ruleset for %s%s is too long "
-             "- shortening it.",
-              city_names[j].name, secfile_str1, secfile_str2);
-      assert(FALSE);
+      freelog(LOG_ERROR, "City name %s in ruleset for %s%s is too long (%d).",
+              city_names[j].name, secfile_str1, secfile_str2,
+             string_character_length(city_names[j].name));
       city_names[j].name[MAX_LEN_NAME - 1] = '\0';
     }
   }
@@ -2261,7 +2260,9 @@
     pl->leaders = fc_malloc(sizeof(*pl->leaders) * pl->leader_count);
     for(j = 0; j < dim; j++) {
       pl->leaders[j].name = mystrdup(leaders[j]);
-      if (check_name(leaders[j])) {
+      if (string_character_length(leaders[j]) >= MAX_LEN_NAME) {
+       freelog(LOG_ERROR, _("Leader name '%s' is too long."),
+               pl->leaders[j].name);
        pl->leaders[j].name[MAX_LEN_NAME - 1] = '\0';
       }
     }
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.206
diff -u -r1.206 srv_main.c
--- server/srv_main.c   8 Nov 2004 15:06:43 -0000       1.206
+++ server/srv_main.c   9 Nov 2004 18:21:05 -0000
@@ -1066,6 +1066,15 @@
     return FALSE;
   }
 
+  if (string_character_length(name) >= MAX_LEN_NAME) {
+    /* Leaders may not have more than MAX_LEN_NAME characters. */
+    if (error_buf) {
+      my_snprintf(error_buf, bufsz,
+                 _("Leader name %s is too long."), name);
+    }
+    return FALSE;
+  }
+
   /* Any name already taken is not allowed. */
   players_iterate(other_player) {
     if (other_player->nation == nation) {
@@ -1155,7 +1164,7 @@
   lsend_packet_nation_select_ok(&pplayer->connections);
 
   pplayer->nation = nation_no;
-  sz_strlcpy(pplayer->name, name);
+  set_player_name(pplayer, name);
   pplayer->is_male = is_male;
   pplayer->city_style = city_style;
 
@@ -1388,8 +1397,8 @@
 
     old_nplayers = game.nplayers;
     pplayer = get_player(old_nplayers);
-     
-    sz_strlcpy(pplayer->name, player_name);
+
+    set_player_name(pplayer, player_name);
     sz_strlcpy(pplayer->username, ANON_USER_NAME);
 
     freelog(LOG_NORMAL, _("%s has been added as an AI-controlled player."),
Index: server/stdinhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/stdinhand.c,v
retrieving revision 1.368
diff -u -r1.368 stdinhand.c
--- server/stdinhand.c  8 Nov 2004 14:46:14 -0000       1.368
+++ server/stdinhand.c  9 Nov 2004 18:21:05 -0000
@@ -866,7 +866,7 @@
 
   pplayer = &game.players[game.nplayers];
   server_player_init(pplayer, FALSE);
-  sz_strlcpy(pplayer->name, arg);
+  set_player_name(pplayer, arg);
   sz_strlcpy(pplayer->username, ANON_USER_NAME);
   pplayer->was_created = TRUE; /* must use /remove explicitly to remove */
 
@@ -897,33 +897,34 @@
 {
   enum m_pre_result match_result;
   struct player *pplayer;
-  char name[MAX_LEN_NAME];
 
   pplayer=find_player_by_name_prefix(arg, &match_result);
   
   if(!pplayer) {
     cmd_reply_no_such_player(CMD_REMOVE, caller, arg, match_result);
     return FALSE;
-  }
+  } else {
+    char name[strlen(pplayer->name) + 1];
 
-  if (!(game.is_new_game && (server_state==PRE_GAME_STATE ||
-                            server_state==SELECT_RACES_STATE))) {
-    cmd_reply(CMD_REMOVE, caller, C_FAIL,
-             _("Players cannot be removed once the game has started."));
-    return FALSE;
-  }
-  if (check) {
-    return TRUE;
-  }
+    if (!(game.is_new_game && (server_state==PRE_GAME_STATE ||
+                              server_state==SELECT_RACES_STATE))) {
+      cmd_reply(CMD_REMOVE, caller, C_FAIL,
+               _("Players cannot be removed once the game has started."));
+      return FALSE;
+    }
+    if (check) {
+      return TRUE;
+    }
 
-  sz_strlcpy(name, pplayer->name);
-  team_remove_player(pplayer);
-  server_remove_player(pplayer);
-  if (!caller || caller->used) {     /* may have removed self */
-    cmd_reply(CMD_REMOVE, caller, C_OK,
-             _("Removed player %s from the game."), name);
+    sz_strlcpy(name, pplayer->name);
+    team_remove_player(pplayer);
+    server_remove_player(pplayer);
+    if (!caller || caller->used) {     /* may have removed self */
+      cmd_reply(CMD_REMOVE, caller, C_OK,
+               _("Removed player %s from the game."), name);
+    }
+    return TRUE;
   }
-  return TRUE;
 }
 
 /**************************************************************************
@@ -2624,7 +2625,7 @@
       pplayer = &game.players[game.nplayers];
       server_player_init(pplayer, 
                          (server_state == RUN_GAME_STATE) || 
!game.is_new_game);
-      sz_strlcpy(pplayer->name, OBSERVER_NAME);
+      set_player_name(pplayer, OBSERVER_NAME);
       sz_strlcpy(pplayer->username, ANON_USER_NAME);
 
       pplayer->is_connected = FALSE;
@@ -2690,7 +2691,7 @@
   /* if the connection is already attached to a player,
    * unattach and cleanup old player (rename, remove, etc) */
   if (pconn->player) {
-    char name[MAX_LEN_NAME];
+    char name[strlen(pplayer->name) + 1];
 
     /* if a pconn->player is removed, we'll lose pplayer */
     sz_strlcpy(name, pplayer->name);
@@ -2847,7 +2848,7 @@
   /* if the connection is already attached to a player,
    * unattach and cleanup old player (rename, remove, etc) */
   if (pconn->player) {
-    char name[MAX_LEN_NAME];
+    char name[strlen(pplayer->name) + 1];
 
     /* if a pconn->player is removed, we'll lose pplayer */
     sz_strlcpy(name, pplayer->name);
Index: utility/shared.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/shared.c,v
retrieving revision 1.119
diff -u -r1.119 shared.c
--- utility/shared.c    18 Oct 2004 23:49:27 -0000      1.119
+++ utility/shared.c    9 Nov 2004 18:21:05 -0000
@@ -43,6 +43,7 @@
 #endif
 
 #include "astring.h"
+#include "fciconv.h"
 #include "fcintl.h"
 #include "log.h"
 #include "mem.h"
@@ -569,6 +570,29 @@
   return num_lines;
 }
 
+static size_t wstrlen(unsigned int *string)
+{
+  size_t i;
+
+  for (i = 0; string[i] != 0; i++) {
+    /* Nothing */
+  }
+
+  return i;
+}
+
+size_t string_character_length(const char *string)
+{
+  unsigned int unicode[strlen(string) + 1];
+
+  convert_string(string, get_internal_encoding(), "UCS-4",
+                (char *)unicode, sizeof(unicode));
+
+  assert(unicode[0] != 0x0000FEFF && unicode[0] != 0xFFFE0000);
+
+  return wstrlen(unicode);
+}
+
 /***************************************************************************
   Returns pointer to '\0' at end of string 'str', and decrements
   *nleft by the length of 'str'.  This is intended to be useful to
Index: utility/shared.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/utility/shared.h,v
retrieving revision 1.137
diff -u -r1.137 shared.h
--- utility/shared.h    21 Oct 2004 20:27:29 -0000      1.137
+++ utility/shared.h    9 Nov 2004 18:21:05 -0000
@@ -206,6 +206,7 @@
 char *skip_leading_spaces(char *s);
 void remove_leading_trailing_spaces(char *s);
 int wordwrap_string(char *s, int len);
+size_t string_character_length(const char *string);
 
 bool check_strlen(const char *str, size_t len, const char *errmsg);
 size_t loud_strlcpy(char *buffer, const char *str, size_t len,