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;
};
___________________________________________________________________________
Sent via: Wireshark-dev mailing list <[email protected]>
Archives: http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
mailto:[email protected]?subject=unsubscribe
