Home › forum › tbaMUD Development › Cleared up a couple tiny mem leaks in 3.6.1

Cleared up a couple tiny mem leaks in 3.6.1

Login to reply  Page: « < 1 of 1 > »
19 Jun 2010 - 07:302738
Cleared up a couple tiny mem leaks in 3.6.1
I was playing around with the memory debugging in zmalloc and saw two sources of very small memory leaks just from init/shutdown of the mud. I made the changes listed below to clear them up:

1. ibt lists 
 diff -u --new-file --recursive tbamud-3.61_stock/src/comm.c tbamud-3.61/src/comm.c
--- tbamud-3.61_stock/src/comm.c  2010-01-26 17:42:22.000000000 -0600
+++ tbamud-3.61/src/comm.c      2010-06-19 00:20:16.000000000 -0500
@@ -80,6 +80,7 @@
 #include "spells.h" /* for affect_update */
 #include "modify.h"
 #include "quest.h"
+#include "ibt.h" /* for free_ibt_lists */

 #ifndef INVALID_SOCKET
 #define INVALID_SOCKET (-1)
@@ -365,6 +366,7 @@
     free_invalid_list();    /* ban.c */
     free_save_list();       /* genolc.c */
     free_strings(&config_info, OASIS_CFG); /* oasis_delete.c */
+    free_ibt_lists();             /* ibt.c */
   }

   if (last_act_message)
diff -u --new-file --recursive tbamud-3.61_stock/src/ibt.c tbamud-3.61/src/ibt.c
--- tbamud-3.61_stock/src/ibt.c   2010-01-26 17:42:22.000000000 -0600
+++ tbamud-3.61/src/ibt.c       2010-06-19 00:22:30.000000000 -0500
@@ -1019,3 +1019,16 @@
      break;
   }
 }
+
+/*-------------------------------------------------------------------*/
+void free_ibt_lists()
+{
+  IBT_DATA *first_ibt, *last_ibt;
+  int mode;
+
+  for( mode=0; mode <=2; mode++){
+    first_ibt = get_first_ibt(mode);
+    last_ibt = get_last_ibt(mode);
+    free_ibt_list(first_ibt, last_ibt);
+  }
+}
diff -u --new-file --recursive tbamud-3.61_stock/src/ibt.h tbamud-3.61/src/ibt.h
--- tbamud-3.61_stock/src/ibt.h   2010-01-26 17:42:22.000000000 -0600
+++ tbamud-3.61/src/ibt.h       2010-06-19 00:19:51.000000000 -0500
@@ -96,4 +96,4 @@
 void load_ibt_file(int mode);
 void ibtedit_parse(struct descriptor_data *d, char *arg);
 void ibtedit_string_cleanup(struct descriptor_data *d, int terminator);
-
+void free_ibt_lists();

2. help_table struct 
diff -u --new-file --recursive tbamud-3.61_stock/src/db.c tbamud-3.61/src/db.c
--- tbamud-3.61_stock/src/db.c    2010-01-26 17:42:21.000000000 -0600
+++ tbamud-3.61/src/db.c        2010-06-19 01:44:17.000000000 -0500
@@ -2167,7 +2167,7 @@
 {
   if (help_table) {
     int hp;
-    for (hp = 0; hp < top_of_helpt; hp++) {
+    for (hp = 0; hp <= top_of_helpt; hp++) {
       if (help_table[hp].keywords)
         free(help_table[hp].keywords);
       if (help_table[hp].entry && !help_table[hp].duplicate)

Before:
zmalloc: 11 leaks totalling 161 bytes... close, but not there yet.

After:
zmalloc: Congratulations: leak-free code!
Offline
19 Jun 2010 - 13:202739
Yeah, that latter one is one of the 28 '< top_of's that I think should mostly probably be <= (except top_of_trigt, which I think has different semantics). The rest are in the patch that's coming soon, although I couldn't convince myself for certain whether the hedit.c references are right or not.


Last edited by Dio (19 Jun 2010 - 13:32)
Offline
19 Jun 2010 - 14:552740
The help_table struct one didn't sit well with me so I looked deeper into it. I have a better understanding of what's happening now.

Here's the code where the extra memory was allocated but never freed later:
"db.c" line 2233 
    while (*next_key) {
      el.keywords = strdup(next_key);   // zmalloc pointed at this line for the allocation
      help_table[top_of_helpt++] = el;
      el.duplicate++;
      scan = one_word(scan, next_key);
    }
The usage of top_of_helpt looks fine here. It gets incremented after the assignment. Looking at this alone it looks like we should be fine to just clear from help_table[0] through help_table[top_of_helpt-1] which is what the free function was doing.

During init we have the following:

"db.c" line 1036 of 3945 
  /* Sort the help index. */
  if (mode == DB_BOOT_HLP) {
    qsort(help_table, top_of_helpt, sizeof(struct help_index_element), hsort);
    top_of_helpt--;
  }
I'm not sure why the "top_of_helpt--;" is here but that explains why we have the extra element left over after the free (since the free only frees the elements through help_table[top_of_helpt-1]. This usage is bound to cause trouble for functions like hedit_save_internally and hedit_save_to_disk as those are looping through the table like this:
for (i = 0; i < top_of_helpt; i++) {
Like the free, they'll leave one element behind due to the decrement of top_of_helpt during init.

Removing that "top_of_helpt--;" line in db.c might be the better option. This should keep the other references to this array working properly.


Offline
19 Jun 2010 - 18:202742
I'm not a fan of the top_of_* semantic and particularly not if it varies by type. (In my code, I don't have top_of_mobt and top_of_objt, I have mobt_count and objt_count, which are both more obviously 'the number of entries' and for-loops use standard zero to < count indexing.) Unfortunately I think TBA already has this issue in that it appears to me that top_of_trigt is actually a count not a top entry, but I'm not absolutely certain of that.

I proved that at least one of the uses of top_of_helpt in hedit.c is wrong; ~ is the last entry in the help, and if you type 'hindex ~' there's no output, so I fixed that one. But I couldn't show a bug with the other uses so I left alone. And one of my other fixes on top_of_worldt caused an invalid memory reference, for reasons not entirely clear.

If it was up to me, I'd replace all the top_of_* entries with *_count and set the semantics accordingly.
Offline
20 Jun 2010 - 04:402750
count makes more sense, and I bet that is what Welcor did for trigedit, but left the other OLC's as they were. I removed the top_of_helpt-- and will see if anything strange pops up, thanks for the fixes, added to the SVN.


__________________
Rumble
The Builder Academy
tbamud.com 9091
Offline
20 Jun 2010 - 08:572751
Just been through to change this and yes, this is definitely the right fix.

There's all sorts of odd little bugs at the moment that go away if you get rid of the -- (e.g. top_of_helpt was initialised to 0, but that means '1 entry' - in fact, I bet that explains the bug in free_help_table: if you change it to <= but have no helpfile, then you get an invalid free on exit), so it definitely wants to become a count.
Offline
Login to reply  Page: « < 1 of 1 > »


Home › forum › tbaMUD Development › Cleared up a couple tiny mem leaks in 3.6.1