Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Bug 7775] Wireshark leaks memory when selecting packets

[Jakub Zawadzki <darkjames-ws@xxxxxxxxxxxx>]

On Mon, Oct 08, 2012 at 06:29:33PM -0400, Evan Huus wrote:
> It doesn't crash, yes, but it leaks again. I've added
> emem_destroy_chunk() for now in revision 45412, 

I forgot to note that emem_destroy_chunk() works only with chunks
created with emem_create_chunk()

emem_create_chunk_gp() changes npc->buf, and amount_free_init
So it leaks 2 * page_size (8KiB) per packet ;|

Still I don't understand why it fails assertion when doing mprotect()
[bug #7814].

> until I can spend some time figuring out the correct logic for sharing freed blocks.

I tried different approach, use one pool for ep_ memory, but each chunk can have different ->id,
attaching proof.

Heh, I start to think that we should just use Boehm GC...

diff --git a/epan/emem.c b/epan/emem.c
index e6239cb..43c9466 100644
--- a/epan/emem.c
+++ b/epan/emem.c
@@ -117,13 +117,15 @@ typedef struct _emem_chunk_t {
 	unsigned int	free_offset_init;
 	unsigned int	free_offset;
 	void		*canary_last;
+	void            *id;
 } emem_chunk_t;
 
-struct _emem_pool_t {
+typedef struct _emem_pool_t {
 	emem_chunk_t *free_list;
 	emem_chunk_t *used_list;
 
 	emem_tree_t *trees;		/* only used by se_mem allocator */
+	void *id;
 
 	guint8 canary[EMEM_CANARY_DATA_SIZE];
 	void *(*memory_alloc)(size_t size, struct _emem_pool_t *);
@@ -156,11 +158,9 @@ struct _emem_pool_t {
 	 */
 	gboolean debug_verify_pointers;
 
-};
+} emem_pool_t;
 
-static GSList *ep_pool_stack = NULL;
-static emem_pool_t ep_fake_pool;
-static emem_pool_t *ep_packet_mem = &ep_fake_pool;
+static emem_pool_t ep_packet_mem;
 static emem_pool_t se_packet_mem;
 
 /*
@@ -267,11 +267,11 @@ ep_check_canary_integrity(const char* fmt, ...)
 	g_vsnprintf(here, sizeof(here), fmt, ap);
 	va_end(ap);
 
-	for (npc = ep_packet_mem->free_list; npc != NULL; npc = npc->next) {
+	for (npc = ep_packet_mem.free_list; npc != NULL; npc = npc->next) {
 		void *canary_next = npc->canary_last;
 
 		while (canary_next != NULL) {
-			canary_next = emem_canary_next(ep_packet_mem->canary, canary_next, NULL);
+			canary_next = emem_canary_next(ep_packet_mem.canary, canary_next, NULL);
 			/* XXX, check if canary_next is inside allocated memory? */
 
 			if (canary_next == (void *) -1)
@@ -301,21 +301,21 @@ emem_init_chunk(emem_pool_t *mem)
  * up.
  */
 static void
-ep_init_chunk(emem_pool_t *mem)
+ep_init_chunk(void)
 {
-	mem->free_list=NULL;
-	mem->used_list=NULL;
-	mem->trees=NULL;	/* not used by this allocator */
+	ep_packet_mem.free_list=NULL;
+	ep_packet_mem.used_list=NULL;
+	ep_packet_mem.trees=NULL;	/* not used by this allocator */
 
-	mem->debug_use_chunks = (getenv("WIRESHARK_DEBUG_EP_NO_CHUNKS") == NULL);
-	mem->debug_use_canary = mem->debug_use_chunks && (getenv("WIRESHARK_DEBUG_EP_NO_CANARY") == NULL);
-	mem->debug_verify_pointers = (getenv("WIRESHARK_EP_VERIFY_POINTERS") != NULL);
+	ep_packet_mem.debug_use_chunks = (getenv("WIRESHARK_DEBUG_EP_NO_CHUNKS") == NULL);
+	ep_packet_mem.debug_use_canary = ep_packet_mem.debug_use_chunks && (getenv("WIRESHARK_DEBUG_EP_NO_CANARY") == NULL);
+	ep_packet_mem.debug_verify_pointers = (getenv("WIRESHARK_EP_VERIFY_POINTERS") != NULL);
 
 #ifdef DEBUG_INTENSE_CANARY_CHECKS
 	intense_canary_checking = (getenv("WIRESHARK_DEBUG_EP_INTENSE_CANARY") != NULL);
 #endif
 
-	emem_init_chunk(mem);
+	emem_init_chunk(&ep_packet_mem);
 }
 
 /* Initialize the capture-lifetime memory allocation pool.
@@ -343,8 +343,8 @@ se_init_chunk(void)
 void
 emem_init(void)
 {
+	ep_init_chunk();
 	se_init_chunk();
-	ep_init_chunk(&ep_fake_pool);
 
 	if (getenv("WIRESHARK_DEBUG_SCRUB_MEMORY"))
 		debug_use_memory_scrubber  = TRUE;
@@ -395,21 +395,21 @@ print_alloc_stats()
 
 	fprintf(stderr, "\n-------- EP allocator statistics --------\n");
 	fprintf(stderr, "%s chunks, %s canaries, %s memory scrubber\n",
-	       ep_packet_mem->debug_use_chunks ? "Using" : "Not using",
-	       ep_packet_mem->debug_use_canary ? "using" : "not using",
+	       ep_packet_mem.debug_use_chunks ? "Using" : "Not using",
+	       ep_packet_mem.debug_use_canary ? "using" : "not using",
 	       debug_use_memory_scrubber ? "using" : "not using");
 
-	if (! (ep_packet_mem->free_list || !ep_packet_mem->used_list)) {
+	if (! (ep_packet_mem.free_list || !ep_packet_mem.used_list)) {
 		fprintf(stderr, "No memory allocated\n");
 		ep_stat = FALSE;
 	}
-	if (ep_packet_mem->debug_use_chunks && ep_stat) {
+	if (ep_packet_mem.debug_use_chunks && ep_stat) {
 		/* Nothing interesting without chunks */
 		/*  Only look at the used_list since those chunks are fully
 		 *  used.  Looking at the free list would skew our view of what
 		 *  we have wasted.
 		 */
-		for (chunk = ep_packet_mem->used_list; chunk; chunk = chunk->next) {
+		for (chunk = ep_packet_mem.used_list; chunk; chunk = chunk->next) {
 			num_chunks++;
 			total_used += (chunk->amount_free_init - chunk->amount_free);
 			total_allocation += chunk->amount_free_init;
@@ -563,8 +563,8 @@ emem_verify_pointer(const emem_pool_t *hdr, const void *ptr)
 gboolean
 ep_verify_pointer(const void *ptr)
 {
-	if (ep_packet_mem->debug_verify_pointers)
-		return emem_verify_pointer(ep_packet_mem, ptr);
+	if (ep_packet_mem.debug_verify_pointers)
+		return emem_verify_pointer(&ep_packet_mem, ptr);
 	else
 		return FALSE;
 }
@@ -797,21 +797,24 @@ emem_alloc_chunk(size_t size, emem_pool_t *mem)
 	/* make sure we dont try to allocate too much (arbitrary limit) */
 	DISSECTOR_ASSERT(size<(EMEM_PACKET_CHUNK_SIZE>>2));
 
-	if (!mem->free_list)
+	if (!mem->free_list) {
 		mem->free_list = emem_create_chunk_gp(EMEM_PACKET_CHUNK_SIZE);
+		mem->free_list->id = mem->id;
 
 	/* oops, we need to allocate more memory to serve this request
 	 * than we have free. move this node to the used list and try again
 	 */
-	if(asize > mem->free_list->amount_free) {
+	} else if (mem->free_list->id != mem->id || asize > mem->free_list->amount_free) {
 		emem_chunk_t *npc;
 		npc=mem->free_list;
 		mem->free_list=mem->free_list->next;
 		npc->next=mem->used_list;
 		mem->used_list=npc;
 
-		if (!mem->free_list)
+		if (!mem->free_list) {
 			mem->free_list = emem_create_chunk_gp(EMEM_PACKET_CHUNK_SIZE);
+			mem->free_list->id = mem->id;
+		}
 	}
 
 	free_list = mem->free_list;
@@ -872,7 +875,7 @@ emem_alloc(size_t size, emem_pool_t *mem)
 void *
 ep_alloc(size_t size)
 {
-	return emem_alloc(size, ep_packet_mem);
+	return emem_alloc(size, &ep_packet_mem);
 }
 
 /* allocate 'size' amount of memory with an allocation lifetime until the
@@ -1189,27 +1192,46 @@ ep_strconcat(const gchar *string1, ...)
 
 /* release all allocated memory back to the pool. */
 static void
-emem_free_all(emem_pool_t *mem)
+emem_free_id(emem_pool_t *mem, void *id)
 {
 	gboolean use_chunks = mem->debug_use_chunks;
 
-	emem_chunk_t *npc;
+	emem_chunk_t *npc, *list, *prev;
 	emem_tree_t *tree_list;
 
-	/* move all used chunks over to the free list */
-	while(mem->used_list){
-		npc=mem->used_list;
-		mem->used_list=mem->used_list->next;
-		npc->next=mem->free_list;
-		mem->free_list=npc;
+	/* move all used id-chunks over to the free list */
+	list = mem->used_list;
+	prev = NULL;
+	while (list) {
+		npc = list;
+		list = list->next;
+
+		/* npc->id can be NULL when it don't care about id's (se_), or temporary UI memory (which can be freed any time) */
+		if (npc->id && npc->id != id) {
+			prev = npc;
+			continue;
+		}
+
+		if (prev == NULL)
+			mem->used_list = npc->next;
+		else
+			prev->next = npc->next;
+
+		npc->next = mem->free_list;
+		mem->free_list = npc;
 	}
 
 	/* clear them all out */
-	npc = mem->free_list;
-	while (npc != NULL) {
-		if (use_chunks) {
-			emem_chunk_t *next = npc->next;
+	list = mem->free_list;
+	while (list != NULL) {
+		npc = list;
+		list = list->next;
+
+		/* npc->id can be NULL when it don't care about id's (se_), or temporary UI memory (which can be freed any time) */
+		if (npc->id && npc->id != id)
+			continue;
 
+		if (use_chunks) {
 			while (npc->canary_last != NULL) {
 				npc->canary_last = emem_canary_next(mem->canary, npc->canary_last, NULL);
 				/* XXX, check if canary_last is inside allocated memory? */
@@ -1222,21 +1244,20 @@ emem_free_all(emem_pool_t *mem)
 					  (npc->free_offset - npc->free_offset_init),
 					  FALSE);
 
-			emem_destroy_chunk(npc);
-
-			npc = next;
+			npc->amount_free = npc->amount_free_init;
+			npc->free_offset = npc->free_offset_init;
 		} else {
-			emem_chunk_t *next = npc->next;
-
 			emem_scrub_memory(npc->buf, npc->amount_free_init, FALSE);
 
 			g_free(npc->buf);
 			g_free(npc);
-			npc = next;
 		}
 	}
 
-	mem->free_list = NULL;
+	if (!use_chunks) {
+		/* We've freed all this memory already */
+		mem->free_list = NULL;
+	}
 
 	/* release/reset all allocated trees */
 	for(tree_list=mem->trees;tree_list;tree_list=tree_list->next){
@@ -1308,41 +1329,23 @@ emem_free_all(emem_pool_t *mem)
  * [3] https://www.wireshark.org/lists/wireshark-dev/201208/msg00128.html
  * [4] https://anonsvn.wireshark.org/viewvc?view=revision&revision=45389
  */
-emem_pool_t *
-ep_create_pool(void)
-{
-	emem_pool_t *mem;
-
-	mem = g_malloc(sizeof(emem_pool_t));
-
-	ep_init_chunk(mem);
 
-	if (ep_pool_stack == NULL) {
-		emem_free_all(&ep_fake_pool);
-	}
-
-	ep_pool_stack = g_slist_prepend(ep_pool_stack, mem);
+void *
+ep_set_id(void *id)
+{
+	void *old;
 
-	ep_packet_mem = mem;
+	old = ep_packet_mem.id;
+	ep_packet_mem.id = id;
 
-	return mem;
+	return old;
 }
 
+/* release all allocated memory back to the pool. */
 void
-ep_free_pool(emem_pool_t *mem)
+ep_free_id(void *id)
 {
-	ep_pool_stack = g_slist_remove(ep_pool_stack, mem);
-
-	emem_free_all(mem);
-
-	g_free(mem);
-
-	if (ep_pool_stack == NULL) {
-		ep_packet_mem = &ep_fake_pool;
-	}
-	else {
-		ep_packet_mem = ep_pool_stack->data;
-	}
+	emem_free_id(&ep_packet_mem, id);
 }
 
 /* release all allocated memory back to the pool. */
@@ -1353,7 +1356,7 @@ se_free_all(void)
 	print_alloc_stats();
 #endif
 
-	emem_free_all(&se_packet_mem);
+	emem_free_id(&se_packet_mem, NULL);
 }
 
 void
diff --git a/epan/emem.h b/epan/emem.h
index a338ac5..384fb53 100644
--- a/epan/emem.h
+++ b/epan/emem.h
@@ -34,8 +34,6 @@
  */
 void emem_init(void);
 
-typedef struct _emem_pool_t emem_pool_t;
-
 /* Functions for handling memory allocation and garbage collection with
  * a packet lifetime scope.
  * These functions are used to allocate memory that will only remain persistent
@@ -48,9 +46,6 @@ typedef struct _emem_pool_t emem_pool_t;
  * the previous packet is freed.
  */
 
-emem_pool_t *ep_create_pool(void);
-void ep_free_pool(emem_pool_t *ep_packet_mem);
-
 /** Allocate memory with a packet lifetime scope */
 void *ep_alloc(size_t size) G_GNUC_MALLOC;
 #define ep_new(type) ((type*)ep_alloc(sizeof(type)))
@@ -92,6 +87,8 @@ gchar *ep_strconcat(const gchar *string, ...) G_GNUC_MALLOC G_GNUC_NULL_TERMINAT
  */
 gchar** ep_strsplit(const gchar* string, const gchar* delimiter, int max_tokens);
 
+void *ep_set_id(void *id);
+void ep_free_id(void *id);
 
 /** a stack implemented using ephemeral allocators */
 
diff --git a/epan/epan.c b/epan/epan.c
index 3bb6eda..acfe0dc 100644
--- a/epan/epan.c
+++ b/epan/epan.c
@@ -164,8 +164,6 @@ epan_dissect_init(epan_dissect_t *edt, const gboolean create_proto_tree, const g
 
 	edt->pi.dependent_frames = NULL;
 
-	edt->mem = ep_create_pool();
-
 	return edt;
 }
 
@@ -190,7 +188,11 @@ void
 epan_dissect_run(epan_dissect_t *edt, void* pseudo_header,
         const guint8* data, frame_data *fd, column_info *cinfo)
 {
+	void *old_ep_id;
+
+	old_ep_id = ep_set_id(edt);
 	dissect_packet(edt, pseudo_header, data, fd, cinfo);
+	(void) ep_set_id(old_ep_id);
 }
 
 void
@@ -210,7 +212,7 @@ epan_dissect_cleanup(epan_dissect_t* edt)
 		proto_tree_free(edt->tree);
 	}
 
-	ep_free_pool(edt->mem);
+	ep_free_id(edt);
 }
 
 void
diff --git a/epan/epan_dissect.h b/epan/epan_dissect.h
index 5969182..ff48107 100644
--- a/epan/epan_dissect.h
+++ b/epan/epan_dissect.h
@@ -31,7 +31,6 @@ extern "C" {
 #include "tvbuff.h"
 #include "proto.h"
 #include "packet_info.h"
-#include "emem.h"
 
 /* Dissection of a single byte array. Holds tvbuff info as
  * well as proto_tree info. As long as the epan_dissect_t for a byte
@@ -42,7 +41,6 @@ extern "C" {
 struct _epan_dissect_t {
 	tvbuff_t	*tvb;
 	proto_tree	*tree;
-	emem_pool_t     *mem;
 	packet_info	pi;
 };