Wireshark · Wireshark-dev: Re: [Wireshark-dev] Memory leak

["Didier" <dgautheron@xxxxxxxx>]

On Thu, 19 Jul 2007 14:16:49 -0400, Jeff Morriss wrote
> Didier wrote:
> > Hi,
> >
> > 1) It seems that since some glib 2.0 version g_mem_chunk_destroy doesn't
> > free
>
> The docs certainly seem to indicate that the memory should actually
> be freed:
>
> http://www.gtk.org/api/2.6/glib/glib-Memory-Chunks.html#g-mem-chunk-destroy
>
> and my (basic) attempt to follow the code goes down to
> g_slice_free1() which appears to at least _try_ to free the memory.
> Am I missing something?
In my understanding now g-mem-chunk-destroy only free the mem chunk object
(aka sizeof  structure g_mem_chunk) not the allocated memory as with gtk1.
You have to call g_mem_chunk_free for each allocated chunk.

> 
> [That's not to say I don't see memory usage growing when I reload a 
> capture file, but I'm not convinced this is the source.]
Attached a small patch which replace gmemchunk with se_alloc (I didn't   
change cf name and header file on purpose but a proper patch should).

The drawback is that if DEBUG_USE_CANARIES is defined in epan/emem.c (the
default) the virtual size is a lot bigger. There's still leaks though.

> 
> > 2)COPY_ADDRESS is still misused in a lot of place it g_malloc address
> > space
> > but many don't free it.
> 
> There is (also) an SE_COPY_ADDRESS; is there any reason not to make
> all COPY_ADDRESS calls seasonal?
It makes sense for some but a lot of them are in taps code with a live time
between ep and se.
Some leaks are on every access cf. dissectors/packet-jxta.c

Didier

=== modified file 'file.c'
--- file.c	2007-07-19 22:04:10 +0000
+++ file.c	2007-07-19 23:50:31 +0000
@@ -75,6 +75,7 @@
 #include <epan/dissectors/packet-data.h>
 #include <epan/dissectors/packet-ber.h>
 #include <epan/timestamp.h>
+#include <epan/emem.h>
 #include "file_util.h"
 
 
@@ -256,11 +257,7 @@
   nstime_set_unset(&first_ts);
   nstime_set_unset(&prev_dis_ts);
 
-  cf->plist_chunk = g_mem_chunk_new("frame_data_chunk",
-	sizeof(frame_data),
-	FRAME_DATA_CHUNK_SIZE * sizeof(frame_data),
-	G_ALLOC_AND_FREE);
-  g_assert(cf->plist_chunk);
+  cf->plist_chunk = NULL;
 
   /* change the time formats now, as we might have a new precision */
   cf_change_time_formats(cf);
@@ -308,10 +305,8 @@
   /* ...which means we have nothing to save. */
   cf->user_saved = FALSE;
 
-  if (cf->plist_chunk != NULL) {
-    g_mem_chunk_destroy(cf->plist_chunk);
-    cf->plist_chunk = NULL;
-  }
+  cf->plist_chunk = NULL;
+
   if (cf->rfcode != NULL) {
     dfilter_free(cf->rfcode);
     cf->rfcode = NULL;
@@ -1063,8 +1058,11 @@
   int row = -1;
 
   /* Allocate the next list entry, and add it to the list. */
-  fdata = g_mem_chunk_alloc(cf->plist_chunk);
-
+  if (cf->plist_chunk)
+    fdata = cf->plist_chunk;
+  else
+    cf->plist_chunk = fdata = se_alloc(sizeof (frame_data)); 
+ 
   fdata->num = 0;
   fdata->next = NULL;
   fdata->prev = NULL;
@@ -1108,16 +1106,7 @@
     cf->f_datalen = offset + phdr->caplen;
     fdata->num = cf->count;
     row = add_packet_to_packet_list(fdata, cf, dfcode, pseudo_header, buf, TRUE);
-  } else {
-    /* XXX - if we didn't have read filters, or if we could avoid
-       allocating the "frame_data" structure until we knew whether
-       the frame passed the read filter, we could use a G_ALLOC_ONLY
-       memory chunk...
-
-       ...but, at least in one test I did, where I just made the chunk
-       a G_ALLOC_ONLY chunk and read in a huge capture file, it didn't
-       seem to save a noticeable amount of time or space. */
-    g_mem_chunk_free(cf->plist_chunk, fdata);
+    cf->plist_chunk = NULL;
   }
 
   return row;