minor memory leak

I ran the fuzzer with MONITOR_HEAP enabled and heaputil found a dozen
or so un-free'd allocations, all made by the same dupstr() call in
special_handling() for "symset" and "roguesymset".  (Reproducible with
a few tens of thousands of fuzzer moves, although you have to take
over from the fuzzer and make a clean exit rather than just interrupt
it or there'll be lots of other un-free'd memory.)  I haven't actually
figured out how/why it was leaking, but reorganizing the code has made
the leak go away (according to a couple of even longer fuzzer runs) so
I'm settling for that.

diff --git a/src/files.c b/src/files.c
index a73349c3e73f9534319b0d0635354fa2fd1883e3..00d1fe9726ecca1e9f783238b93d63db9f05e1ed 100644 (file)
--- a/src/files.c
+++ b/src/files.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 files.c $NHDT-Date: 1545702598 2018/12/25 01:49:58 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.248 $ */
+/* NetHack 3.6 files.c $NHDT-Date: 1546144856 2018/12/30 04:40:56 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.249 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Derek S. Ray, 2015. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -3264,37 +3264,26 @@ int which_set;
 
             switch (symp->idx) {
             case 0:
-                tmpsp =
-                    (struct symsetentry *) alloc(sizeof (struct symsetentry));
-                tmpsp->next = (struct symsetentry *) 0;
-                if (!symset_list) {
-                    symset_list = tmpsp;
-                    symset_count = 0;
-                } else {
-                    symset_count++;
-                    tmpsp->next = symset_list;
-                    symset_list = tmpsp;
-                }
-                tmpsp->idx = symset_count;
+                tmpsp = (struct symsetentry *) alloc(sizeof *tmpsp);
+                tmpsp->next = symset_list;
+                symset_list = tmpsp;
+                tmpsp->idx = symset_count++;
                 tmpsp->name = dupstr(bufp);
                 tmpsp->desc = (char *) 0;
-                tmpsp->nocolor = 0;
+                tmpsp->handling = H_UNK;
                 /* initialize restriction bits */
+                tmpsp->nocolor = 0;
                 tmpsp->primary = 0;
                 tmpsp->rogue = 0;
                 break;
             case 2:
                 /* handler type identified */
                 tmpsp = symset_list; /* most recent symset */
-                tmpsp->handling = H_UNK;
-                i = 0;
-                while (known_handling[i]) {
+                for (i = 0; known_handling[i]; ++i)
                     if (!strcmpi(known_handling[i], bufp)) {
                         tmpsp->handling = i;
-                        break; /* while loop */
+                        break; /* for loop */
                     }
-                    i++;
-                }
                 break;
             case 3:                  /* description:something */
                 tmpsp = symset_list; /* most recent symset */

diff --git a/src/options.c b/src/options.c
index 17965cb058e8318799586fafe053df145fb4f6b2..564abf833caf1d209c34bb5a7b32f55584fed6bc 100644 (file)
--- a/src/options.c
+++ b/src/options.c
@@ -1,4 +1,4 @@
-/* NetHack 3.6 options.c       $NHDT-Date: 1544773907 2018/12/14 07:51:47 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.347 $ */
+/* NetHack 3.6 options.c       $NHDT-Date: 1546144857 2018/12/30 04:40:57 $  $NHDT-Branch: NetHack-3.6.2-beta01 $:$NHDT-Revision: 1.349 $ */
 /* Copyright (c) Stichting Mathematisch Centrum, Amsterdam, 1985. */
 /*-Copyright (c) Michael Allison, 2008. */
 /* NetHack may be freely redistributed.  See license for details. */
@@ -5201,8 +5201,7 @@ boolean setinitial, setfromfile;
     } else if (!strcmp("symset", optname)
                || !strcmp("roguesymset", optname)) {
         menu_item *symset_pick = (menu_item *) 0;
-        boolean primaryflag = (*optname == 's'),
-                rogueflag = (*optname == 'r'),
+        boolean rogueflag = (*optname == 'r'),
                 ready_to_switch = FALSE,
                 nothing_to_do = FALSE;
         char *symset_name, fmtstr[20];
@@ -5210,37 +5209,33 @@ boolean setinitial, setfromfile;
         int res, which_set, setcount = 0, chosen = -2;
 
         which_set = rogueflag ? ROGUESET : PRIMARY;
-
+        symset_list = (struct symsetentry *) 0;
         /* clear symset[].name as a flag to read_sym_file() to build list */
         symset_name = symset[which_set].name;
         symset[which_set].name = (char *) 0;
-        symset_list = (struct symsetentry *) 0;
 
         res = read_sym_file(which_set);
+
+        /* put symset name back */
+        symset[which_set].name = symset_name;
+
         if (res && symset_list) {
-            char symsetchoice[BUFSZ];
-            int let = 'a', biggest = 0, thissize = 0;
+            int thissize, biggest = 0;
 
-            sl = symset_list;
-            while (sl) {
+            for (sl = symset_list; sl; sl = sl->next) {
                 /* check restrictions */
-                if ((!rogueflag && sl->rogue)
-                    || (!primaryflag && sl->primary)) {
-                    sl = sl->next;
+                if (rogueflag ? sl->primary : sl->rogue)
                     continue;
-                }
+
                 setcount++;
                 /* find biggest name */
-                if (sl->name)
-                    thissize = strlen(sl->name);
+                thissize = sl->name ? (int) strlen(sl->name) : 0;
                 if (thissize > biggest)
                     biggest = thissize;
-                sl = sl->next;
             }
             if (!setcount) {
-                pline("There are no appropriate %ssymbol sets available.",
-                      (rogueflag) ? "rogue level "
-                                  : (primaryflag) ? "primary " : "");
+                pline("There are no appropriate %s symbol sets available.",
+                      rogueflag ? "rogue level" : "primary");
                 return TRUE;
             }
 
@@ -5249,29 +5244,20 @@ boolean setinitial, setfromfile;
             start_menu(tmpwin);
             any = zeroany;
             any.a_int = 1;
-            add_menu(tmpwin, NO_GLYPH, &any, let++, 0, ATR_NONE,
+            add_menu(tmpwin, NO_GLYPH, &any, 0, 0, ATR_NONE,
                      "Default Symbols", MENU_UNSELECTED);
 
-            sl = symset_list;
-            while (sl) {
+            for (sl = symset_list; sl; sl = sl->next) {
                 /* check restrictions */
-                if ((!rogueflag && sl->rogue)
-                    || (!primaryflag && sl->primary)) {
-                    sl = sl->next;
+                if (rogueflag ? sl->primary : sl->rogue)
                     continue;
-                }
+
                 if (sl->name) {
                     any.a_int = sl->idx + 2;
-                    Sprintf(symsetchoice, fmtstr, sl->name,
-                            sl->desc ? sl->desc : "");
-                    add_menu(tmpwin, NO_GLYPH, &any, let, 0, ATR_NONE,
-                             symsetchoice, MENU_UNSELECTED);
-                    if (let == 'z')
-                        let = 'A';
-                    else
-                        let++;
+                    Sprintf(buf, fmtstr, sl->name, sl->desc ? sl->desc : "");
+                    add_menu(tmpwin, NO_GLYPH, &any, 0, 0, ATR_NONE,
+                             buf, MENU_UNSELECTED);
                 }
-                sl = sl->next;
             }
             Sprintf(buf, "Select %ssymbol set:",
                     rogueflag ? "rogue level " : "");
@@ -5284,30 +5270,20 @@ boolean setinitial, setfromfile;
 
             if (chosen > -1) {
                 /* chose an actual symset name from file */
-                sl = symset_list;
-                while (sl) {
-                    if (sl->idx == chosen) {
-                        if (symset_name) {
-                            free((genericptr_t) symset_name);
-                            symset_name = (char *) 0;
-                        }
-                        /* free the now stale attributes */
-                        clear_symsetentry(which_set, TRUE);
-
-                        /* transfer only the name of the symbol set */
-                        symset[which_set].name = dupstr(sl->name);
-                        ready_to_switch = TRUE;
+                for (sl = symset_list; sl; sl = sl->next)
+                    if (sl->idx == chosen)
                         break;
-                    }
-                    sl = sl->next;
+                if (sl) {
+                    /* free the now stale attributes */
+                    clear_symsetentry(which_set, TRUE);
+
+                    /* transfer only the name of the symbol set */
+                    symset[which_set].name = dupstr(sl->name);
+                    ready_to_switch = TRUE;
                 }
             } else if (chosen == -1) {
                 /* explicit selection of defaults */
                 /* free the now stale symset attributes */
-                if (symset_name) {
-                    free((genericptr_t) symset_name);
-                    symset_name = (char *) 0;
-                }
                 clear_symsetentry(which_set, TRUE);
             } else
                 nothing_to_do = TRUE;
@@ -5322,26 +5298,18 @@ boolean setinitial, setfromfile;
         }
 
         /* clean up */
-        while (symset_list) {
-            sl = symset_list;
+        while ((sl = symset_list) != 0) {
+            symset_list = sl->next;
             if (sl->name)
-                free((genericptr_t) sl->name);
-            sl->name = (char *) 0;
-
+                free((genericptr_t) sl->name), sl->name = (char *) 0;
             if (sl->desc)
-                free((genericptr_t) sl->desc);
-            sl->desc = (char *) 0;
-
-            symset_list = sl->next;
+                free((genericptr_t) sl->desc), sl->desc = (char *) 0;
             free((genericptr_t) sl);
         }
 
         if (nothing_to_do)
             return TRUE;
 
-        if (!symset[which_set].name && symset_name)
-            symset[which_set].name = symset_name; /* not dupstr() here */
-
         /* Set default symbols and clear the handling value */
         if (rogueflag)
             init_r_symbols();
@@ -5349,6 +5317,7 @@ boolean setinitial, setfromfile;
             init_l_symbols();
 
         if (symset[which_set].name) {
+            /* non-default symbols */
             if (read_sym_file(which_set)) {
                 ready_to_switch = TRUE;
             } else {
@@ -5367,7 +5336,6 @@ boolean setinitial, setfromfile;
             assign_graphics(PRIMARY);
         preference_update("symset");
         need_redraw = TRUE;
-        return TRUE;
 
     } else {
         /* didn't match any of the special options */