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

This is old and crufty code!  Used malloc() instead of calloc(), so
uninitialized variables have bad data.

Server sent 0 for bool max (bad), and other fields (probably OK).

The client never set the default values (leaving them uninitialized),
although they were sent over the network.

And, client assert() for negative id, but never tested for id out of range.

And, client assert() for bad type.

And, client never tested for missing categories.

And, client never set various string pointers to NULL, so free() potentially
could have problems....

Fixed all the above, adding LOG_ERROR instead of dying.

Never trust network data!

Also, use the same variable names in client, network, and server.

Compiles and runs for both GTK2 and XAW.

gui-win32 doesn't use proper symbols, so somebody else should fix it.

Index: server/settings.h
===================================================================
--- server/settings.h   (revision 14152)
+++ server/settings.h   (working copy)
@@ -71,9 +71,9 @@
    * etc, and should end with a "."
    */
   const char *extra_help;
-  enum sset_type type;
-  enum sset_category category;
-  enum sset_level level;
+  enum sset_type stype;
+  enum sset_category scategory;
+  enum sset_level slevel;
 
   /* 
    * About the *_validate functions: If the function is non-NULL, it
Index: server/stdinhand.c
===================================================================
--- server/stdinhand.c  (revision 14152)
+++ server/stdinhand.c  (working copy)
@@ -1114,7 +1114,7 @@
     for (i=0;settings[i].name;i++) {
       struct settings_s *op = &settings[i];
 
-      switch (op->type) {
+      switch (op->stype) {
       case SSET_INT:
        fprintf(script_file, "set %s %i\n", op->name, *op->int_value);
        break;
@@ -1551,7 +1551,7 @@
                                  ? _("changeable") : _("fixed")));
   
   if (may_view_option(caller, id)) {
-    switch (op->type) {
+    switch (op->stype) {
     case SSET_BOOL:
       cmd_reply(help_cmd, caller, C_COMMENT,
                _("Value: %d, Minimum: 0, Default: %d, Maximum: 1"),
@@ -1678,19 +1678,21 @@
     sz_strlcpy(packet.short_help, setting->short_help);
     sz_strlcpy(packet.extra_help, setting->extra_help);
 
-    packet.category = setting->category;
-    packet.type = setting->type;
-    packet.class = setting->sclass;
+    packet.stype = setting->stype;
+    packet.scategory = setting->scategory;
+    packet.sclass = setting->sclass;
     packet.is_visible = (sset_is_to_client(setting_id)
                         || pconn->access_level == ALLOW_HACK);
 
     if (packet.is_visible) {
-      switch (setting->type) {
+      switch (setting->stype) {
       case SSET_STRING:
        strcpy(packet.strval, setting->string_value);
        strcpy(packet.default_strval, setting->string_default_value);
        break;
       case SSET_BOOL:
+       packet.min = FALSE;
+       packet.max = TRUE;
        packet.val = *(setting->bool_value);
        packet.default_val = setting->bool_default_value;
        break;
@@ -1968,9 +1970,9 @@
       int len = 0;
 
  
-   if ((level == SSET_ALL || op->level == level || cmd >= 0 
+   if ((level == SSET_ALL || op->slevel == level || cmd >= 0 
      || level == SSET_CHANGED))  {
-   switch (op->type) {
+   switch (op->stype) {
         case SSET_BOOL: 
          if (*op->bool_value != op->bool_default_value) {
            is_changed = TRUE;
@@ -2485,7 +2487,7 @@
   do_update = FALSE;
   buffer[0] = '\0';
 
-  switch (op->type) {
+  switch (op->stype) {
   case SSET_BOOL:
     if (sscanf(arg, "%d", &val) != 1) {
       cmd_reply(CMD_SET, caller, C_SYNTAX, _("Value must be an integer."));
Index: common/packets.def
===================================================================
--- common/packets.def  (revision 14152)
+++ common/packets.def  (working copy)
@@ -1327,8 +1327,8 @@
   STRING short_help[MAX_LEN_PACKET];
   STRING extra_help[MAX_LEN_PACKET];
 
-  SSET_TYPE type;
-  SSET_CLASS class;
+  SSET_TYPE stype;
+  SSET_CLASS sclass;
   BOOL is_visible;
 
   SINT32 val;                                  /* value for bool or int */
@@ -1339,7 +1339,7 @@
   STRING strval[MAX_LEN_PACKET];               /* space for string */
   STRING default_strval[MAX_LEN_PACKET];       /* space for string */
 
-  UINT8 category;                              /* which category this is in */
+  UINT8 scategory;
 end
 
 /************** Effects hash packets **********************/
Index: manual/civmanual.c
===================================================================
--- manual/civmanual.c  (revision 14152)
+++ manual/civmanual.c  (working copy)
@@ -141,9 +141,9 @@
           fprintf(doc, "<pre>%s</pre>\n\n", abuf.str);
         }
         fprintf(doc, "<p class=\"misc\">");
-        fprintf(doc, _("Level: %s.<br>"), _(sset_level_names[op->level]));
+        fprintf(doc, _("Level: %s.<br>"), _(sset_level_names[op->slevel]));
         fprintf(doc, _("Category: %s.<br>"),
-                _(sset_category_names[op->category]));
+                _(sset_category_names[op->scategory]));
         if (op->to_client == SSET_SERVER_ONLY) {
           fprintf(doc, _("Can only be used in server console. "));
         }
@@ -153,7 +153,7 @@
           fprintf(doc, _("Can <b>not</b> be changed during a game. "));
         }
         fprintf(doc, "</p>\n\n");
-        switch (op->type) {
+        switch (op->stype) {
         case SSET_BOOL:
           fprintf(doc, _("<p class=\"bounds\">Minimum: 0, Default: %d, "
                          "Maximum: 1</p>\n\n"),
Index: client/gui-gtk-2.0/repodlgs.c
===================================================================
--- client/gui-gtk-2.0/repodlgs.c       (revision 14152)
+++ client/gui-gtk-2.0/repodlgs.c       (working copy)
@@ -1441,12 +1441,8 @@
   bool *used;
   int i;
 
-  used = fc_malloc(num_options_categories * sizeof(bool));
+  used = fc_calloc(num_options_categories, sizeof(bool));
 
-  for(i = 0; i < num_options_categories; i++){
-    used[i] = FALSE;
-  }
-
   tips = gtk_tooltips_new();
   settable_options_dialog_shell = gtk_dialog_new_with_buttons(_("Game 
Settings"),
       NULL, 0,
@@ -1465,7 +1461,7 @@
   gtk_box_pack_start(GTK_BOX(GTK_DIALOG(win)->vbox), book, FALSE, FALSE, 2);
 
   /* create a number of notebook pages for each category */
-  vbox = fc_malloc(num_options_categories * sizeof(GtkWidget *));
+  vbox = fc_calloc(num_options_categories, sizeof(GtkWidget *));
 
   for (i = 0; i < num_options_categories; i++) {
     vbox[i] = gtk_vbox_new(FALSE, 2);
@@ -1480,9 +1476,9 @@
 
     /* create a box for the new option and insert it in the correct page */
     hbox = gtk_hbox_new(FALSE, 0);
-    gtk_box_pack_start(GTK_BOX(vbox[settable_options[i].category]), 
+    gtk_box_pack_start(GTK_BOX(vbox[settable_options[i].scategory]), 
                        hbox, FALSE, FALSE, 0);
-    used[settable_options[i].category] = TRUE;
+    used[settable_options[i].scategory] = TRUE;
 
     /* create an event box for the option label */
     ebox = gtk_event_box_new();
@@ -1502,12 +1498,12 @@
       gtk_tooltips_set_tip(tips, ebox, buf, NULL);
     }
 
-    if (setting_class_is_changeable(settable_options[i].class)
+    if (setting_class_is_changeable(settable_options[i].sclass)
        && settable_options[i].is_visible) {
       double step, max, min;
 
       /* create the proper entry method depending on the type */
-      switch (settable_options[i].type) {
+      switch (settable_options[i].stype) {
       case SSET_BOOL:
        /* boolean */
        ent = gtk_check_button_new();
@@ -1550,7 +1546,7 @@
       char buf[1024];
 
       if (settable_options[i].is_visible) {
-       switch (settable_options[i].type) {
+       switch (settable_options[i].stype) {
        case SSET_BOOL:
          my_snprintf(buf, sizeof(buf), "%s",
                      settable_options[i].val != 0 ? _("true") : _("false"));
Index: client/repodlgs_common.c
===================================================================
--- client/repodlgs_common.c    (revision 14152)
+++ client/repodlgs_common.c    (working copy)
@@ -18,6 +18,7 @@
 #include <assert.h>
 
 #include "fcintl.h"
+#include "log.h"
 #include "mem.h"               /* free */
 #include "support.h"           /* my_snprintf */
 
@@ -234,7 +235,7 @@
 }
 
 /******************************************************************
- reinitialize the options_settable struct: allocate enough
+ reinitialize the struct options_settable: allocate enough
  space for all the options that the server is going to send us.
 *******************************************************************/
 void handle_options_settable_control(
@@ -244,6 +245,12 @@
 
   settable_options_free();
 
+  /* avoid a malloc of size 0 warning */
+  if (0 == packet->num_categories
+   || 0 == packet->num_settings) {
+    return;
+  }
+
   options_categories = fc_calloc(packet->num_categories,
                                 sizeof(*options_categories));
   num_options_categories = packet->num_categories;
@@ -252,11 +259,6 @@
     options_categories[i] = mystrdup(packet->category_names[i]);
   }
 
-  /* avoid a malloc of size 0 warning */
-  if (packet->num_settings == 0) {
-    return;
-  }
-
   settable_options = fc_calloc(packet->num_settings,
                               sizeof(*settable_options));
   num_settable_options = packet->num_settings;
@@ -272,45 +274,66 @@
 
 /******************************************************************
  Fill the settable_options array with an option.
- If we've filled the last option, popup the dialog.
 *******************************************************************/
 void handle_options_settable(struct packet_options_settable *packet)
 {
   int i = packet->id;
 
-  assert(i >= 0);
+  if (i < 0 || i > num_settable_options) {
+    freelog(LOG_ERROR,
+           "handle_options_settable() bad id %d.",
+           packet->id);
+    return;
+  }
 
-  settable_options[i].name = mystrdup(packet->name);
-  settable_options[i].short_help = mystrdup(packet->short_help);
-  settable_options[i].extra_help = mystrdup(packet->extra_help);
-
-  settable_options[i].type = packet->type;
-  settable_options[i].class = packet->class;
-  settable_options[i].category = packet->category;
+  settable_options[i].stype = packet->stype;
+  settable_options[i].scategory = packet->scategory;
+  settable_options[i].sclass = packet->sclass;
   settable_options[i].is_visible = packet->is_visible;
 
-  switch (settable_options[i].type) {
+  switch (settable_options[i].stype) {
   case SSET_BOOL:
     settable_options[i].val = packet->val;
+    settable_options[i].default_val = packet->default_val;
     settable_options[i].min = FALSE;
-    settable_options[i].max = TRUE;
+    settable_options[i].max = TRUE;            /* server sent FALSE */
     settable_options[i].strval = NULL;
     settable_options[i].default_strval = NULL;
-    return;
+    break;
   case SSET_INT:
     settable_options[i].val = packet->val;
+    settable_options[i].default_val = packet->default_val;
     settable_options[i].min = packet->min;
     settable_options[i].max = packet->max;
     settable_options[i].strval = NULL;
     settable_options[i].default_strval = NULL;
-    return;
+    break;
   case SSET_STRING:
+    settable_options[i].val = packet->val;
+    settable_options[i].default_val = packet->default_val;
+    settable_options[i].min = packet->min;
+    settable_options[i].max = packet->max;
     settable_options[i].strval = mystrdup(packet->strval);
     settable_options[i].default_strval = mystrdup(packet->default_strval);
+    break;
+  default:
+    /* ensure string fields are NULL for free */
+    settable_options[i].name = NULL;
+    settable_options[i].short_help = NULL;
+    settable_options[i].extra_help = NULL;
+    settable_options[i].strval = NULL;
+    settable_options[i].default_strval = NULL;
+
+    freelog(LOG_ERROR,
+           "handle_options_settable() bad type %d.",
+           packet->stype);
     return;
-  }
+  };
 
-  assert(0);
+  /* only set for valid type */
+  settable_options[i].name = mystrdup(packet->name);
+  settable_options[i].short_help = mystrdup(packet->short_help);
+  settable_options[i].extra_help = mystrdup(packet->extra_help);
 }
 
 /****************************************************************************
Index: client/repodlgs_common.h
===================================================================
--- client/repodlgs_common.h    (revision 14152)
+++ client/repodlgs_common.h    (working copy)
@@ -50,12 +50,9 @@
 bool is_report_dialogs_frozen(void);
 
 struct options_settable {
-  char *name;
-  char *short_help;
-  char *extra_help;
-  enum sset_type type;
-  enum sset_class class;
-  unsigned char category;
+  enum sset_type stype;
+  enum sset_class sclass;
+  unsigned char scategory;
   bool is_visible;
 
   int val;
@@ -65,6 +62,10 @@
 
   char *strval;
   char *default_strval;
+
+  char *name;
+  char *short_help;
+  char *extra_help;
 };
 
 extern struct options_settable *settable_options;
Index: client/gui-xaw/repodlgs.c
===================================================================
--- client/gui-xaw/repodlgs.c   (revision 14152)
+++ client/gui-xaw/repodlgs.c   (working copy)
@@ -1269,9 +1269,9 @@
   XtVaSetValues(settable_options_label, XtNwidth, width + 15, NULL);
 
   for (i = 0; i < num_settable_options; i++) {
-    if (setting_class_is_changeable(settable_options[i].class)
+    if (setting_class_is_changeable(settable_options[i].sclass)
        && settable_options[i].is_visible) {
-      switch (settable_options[i].type) {
+      switch (settable_options[i].stype) {
       case SSET_BOOL:
        if (settable_options_widgets[i]) {
          prev_widget =
@@ -1366,9 +1366,9 @@
     int i;
 
     for (i = 0; i < num_settable_options; i++) {
-      if (setting_class_is_changeable(settable_options[i].class)
+      if (setting_class_is_changeable(settable_options[i].sclass)
          && settable_options[i].is_visible) {
-       switch (settable_options[i].type) {
+       switch (settable_options[i].stype) {
        case SSET_BOOL:
          XtVaSetValues(settable_options_widgets[i],
                        XtNstate, settable_options[i].val ? True : False,
@@ -1386,7 +1386,7 @@
        }
       } else {
        if (settable_options[i].is_visible) {
-         switch (settable_options[i].type) {
+         switch (settable_options[i].stype) {
          case SSET_BOOL:
            my_snprintf(buf, sizeof(buf), "%s",
              settable_options[i].val != 0 ? _("true") : _("false"));
@@ -1432,11 +1432,11 @@
     XtPointer dp;
 
     for (i = 0; i < num_settable_options; i++) {
-      if (setting_class_is_changeable(settable_options[i].class)
+      if (setting_class_is_changeable(settable_options[i].sclass)
          && settable_options[i].is_visible) {
        char buffer[MAX_LEN_MSG];
 
-       switch (settable_options[i].type) {
+       switch (settable_options[i].stype) {
        case SSET_BOOL:
          old_b = settable_options[i].val ? True: False;
          XtVaGetValues(settable_options_widgets[i], XtNstate, &b, NULL);
Index: client/gui-win32/repodlgs.c
===================================================================
--- client/gui-win32/repodlgs.c (revision 14152)
+++ client/gui-win32/repodlgs.c (working copy)
@@ -800,8 +800,8 @@
       if (LOWORD(wParam) == IDOK) {
        int i, tab;
         for (i = 0; i < num_settable_options; i++) {
-         tab = categories[settable_options[i].category];
-         if (settable_options[i].type == 0) {
+         tab = categories[settable_options[i].scategory];
+         if (0 == settable_options[i].stype) {
            /* checkbox */
            int val = Button_GetState(GetDlgItem(tab_wnds[tab],
                                      ID_OPTIONS_BASE + i)) == BST_CHECKED;
@@ -811,7 +811,7 @@
                          settable_options[i].name, val);
              send_chat(buffer);
            }
-         } else if (settable_options[i].type == 1) {
+         } else if (1 == settable_options[i].stype) {
            char buf[512];
            int val;
            Edit_GetText(GetDlgItem(tab_wnds[tab], ID_OPTIONS_BASE + i), buf,
@@ -889,15 +889,11 @@
 
   num_tabs = 0;
 
-  used = fc_malloc(num_options_categories * sizeof(bool));
-  categories = fc_malloc(num_options_categories * sizeof(int));
+  used = fc_calloc(num_options_categories, sizeof(bool));
+  categories = fc_calloc(num_options_categories, sizeof(int));
 
-  for(i = 0; i < num_options_categories; i++) {
-    used[i] = FALSE;
-  }
-
   for (i = 0; i < num_settable_options; i++) {
-    used[settable_options[i].category] = TRUE;
+    used[settable_options[i].scategory] = TRUE;
   }
 
   for(i = 0; i < num_options_categories; i++) {
@@ -941,19 +937,19 @@
   }
 
   for (i = 0; i < num_settable_options; i++) {
-    j = categories[settable_options[i].category];
+    j = categories[settable_options[i].scategory];
     hbox = fcwin_hbox_new(tab_wnds[j], FALSE);
     fcwin_box_add_static(hbox, _(settable_options[i].short_help),
                         0, 0, FALSE, TRUE, 5);
     fcwin_box_add_static(hbox, "", 0, 0, TRUE, TRUE, 0);
-    if (settable_options[i].type == 0) {
+    if (0 == settable_options[i].stype) {
       HWND check;
       /* boolean */
       check = fcwin_box_add_checkbox(hbox, "", ID_OPTIONS_BASE + i, 0, FALSE,
                                     TRUE, 5);
       Button_SetCheck(check,
                      settable_options[i].val ? BST_CHECKED : BST_UNCHECKED);
-    } else if (settable_options[i].type == 1) {
+    } else if (1 == settable_options[i].stype) {
       /* integer */
       char buf[80];
       int length;
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to