From 65ec1e973a341c7098a4e71df0f6a8926bf9bcd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Higor=20Eur=C3=ADpedes?= Date: Sun, 3 Sep 2017 09:48:31 -0300 Subject: [PATCH 1/4] (xmb) Add xmb_free_list_nodes() Fixes some node->fullpath memleaks. --- menu/drivers/xmb.c | 76 +++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 24 deletions(-) diff --git a/menu/drivers/xmb.c b/menu/drivers/xmb.c index 6b59b5d009..b029f17840 100755 --- a/menu/drivers/xmb.c +++ b/menu/drivers/xmb.c @@ -448,6 +448,35 @@ static void xmb_free_node(xmb_node_t *node) free(node); } +/** + * @brief frees all xmb_node_t in a file_list_t + * + * file_list_t asumes userdata holds a simple structure and + * free()'s it. Can't change this at the time because other + * code depends on this behavior. + * + * @param list + * @param actiondata whether to free actiondata too + */ +static void xmb_free_list_nodes(file_list_t *list, bool actiondata) +{ + unsigned i, size = file_list_get_size(list); + + for (i = 0; i < size; ++i) + { + void *node = file_list_get_userdata_at_offset(list, i); + + if (node) + xmb_free_node((xmb_node_t*)node); + + /* file_list_set_userdata() doesn't accept NULL */ + list->list[i].userdata = NULL; + + if (actiondata) + file_list_free_actiondata(list, i); + } +} + /* NOTE: This is faster than memcpy()ing xmb_node_t in most cases * because most nodes have small (less than 200 bytes) fullpath */ static xmb_node_t *xmb_copy_node(void *p) @@ -1969,7 +1998,10 @@ static void xmb_refresh_horizontal_list(xmb_handle_t *xmb) { xmb_context_destroy_horizontal_list(xmb); if (xmb->horizontal_list) + { + xmb_free_list_nodes(xmb->horizontal_list, false); file_list_free(xmb->horizontal_list); + } xmb->horizontal_list = NULL; menu_driver_ctl(RARCH_MENU_CTL_SET_PREVENT_POPULATE, NULL); @@ -3490,7 +3522,10 @@ error: free(xmb->selection_buf_old); xmb->selection_buf_old = NULL; if (xmb->horizontal_list) + { + xmb_free_list_nodes(xmb->horizontal_list, false); file_list_free(xmb->horizontal_list); + } xmb->horizontal_list = NULL; } return NULL; @@ -3503,22 +3538,32 @@ static void xmb_free(void *data) if (xmb) { if (xmb->menu_stack_old) + { + xmb_free_list_nodes(xmb->menu_stack_old, false); file_list_free(xmb->menu_stack_old); - xmb->menu_stack_old = NULL; + } if (xmb->selection_buf_old) + { + xmb_free_list_nodes(xmb->selection_buf_old, false); file_list_free(xmb->selection_buf_old); - xmb->selection_buf_old = NULL; + } + if (xmb->horizontal_list) + { + xmb_free_list_nodes(xmb->horizontal_list, false); file_list_free(xmb->horizontal_list); - xmb->horizontal_list = NULL; + } + + xmb->menu_stack_old = NULL; + xmb->selection_buf_old = NULL; + xmb->horizontal_list = NULL; video_coord_array_free(&xmb->raster_block.carr); video_coord_array_free(&xmb->raster_block2.carr); } font_driver_bind_block(NULL, NULL); - } static void xmb_context_bg_destroy(xmb_handle_t *xmb) @@ -3890,17 +3935,7 @@ static void xmb_list_clear(file_list_t *list) menu_animation_ctl(MENU_ANIMATION_CTL_KILL_BY_TAG, &tag); - for (i = 0; i < size; ++i) - { - xmb_node_t *node = (xmb_node_t*) - menu_entries_get_userdata_at_offset(list, i); - - if (!node) - continue; - - xmb_free_node(node); - list->list[i].userdata = NULL; - } + xmb_free_list_nodes(list, false); } static void xmb_list_deep_copy(const file_list_t *src, file_list_t *dst) @@ -3911,15 +3946,8 @@ static void xmb_list_deep_copy(const file_list_t *src, file_list_t *dst) menu_animation_ctl(MENU_ANIMATION_CTL_KILL_BY_TAG, &tag); - for (i = 0; i < size; ++i) - { - xmb_node_t *node = (xmb_node_t*)menu_entries_get_userdata_at_offset(dst, i); - - xmb_free_node(node); - dst->list[i].userdata = NULL; - file_list_free_actiondata(dst, i); /* this one was allocated by us */ - } - + /* use true here because file_list_copy() doesn't free actiondata */ + xmb_free_list_nodes(dst, true); file_list_copy(src, dst); size = dst->size; From a7e6b02107d48d77714c53f35d26cbae19776f55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Higor=20Eur=C3=ADpedes?= Date: Sun, 3 Sep 2017 09:55:29 -0300 Subject: [PATCH 2/4] (xmb) Refactor xmb_copy_node() xmb_node_t is nowhere as big as it used to be, so copying is fine now. --- menu/drivers/xmb.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/menu/drivers/xmb.c b/menu/drivers/xmb.c index b029f17840..e90f9a9cf1 100755 --- a/menu/drivers/xmb.c +++ b/menu/drivers/xmb.c @@ -464,10 +464,7 @@ static void xmb_free_list_nodes(file_list_t *list, bool actiondata) for (i = 0; i < size; ++i) { - void *node = file_list_get_userdata_at_offset(list, i); - - if (node) - xmb_free_node((xmb_node_t*)node); + xmb_free_node((xmb_node_t*)file_list_get_userdata_at_offset(list, i)); /* file_list_set_userdata() doesn't accept NULL */ list->list[i].userdata = NULL; @@ -477,21 +474,12 @@ static void xmb_free_list_nodes(file_list_t *list, bool actiondata) } } -/* NOTE: This is faster than memcpy()ing xmb_node_t in most cases - * because most nodes have small (less than 200 bytes) fullpath */ -static xmb_node_t *xmb_copy_node(void *p) +static xmb_node_t *xmb_copy_node(const xmb_node_t *old_node) { - xmb_node_t *old_node = (xmb_node_t*)p; xmb_node_t *new_node = (xmb_node_t*)malloc(sizeof(*new_node)); - new_node->alpha = old_node->alpha; - new_node->label_alpha = old_node->label_alpha; - new_node->zoom = old_node->zoom; - new_node->x = old_node->x; - new_node->y = old_node->y; - new_node->icon = old_node->icon; - new_node->content_icon = old_node->content_icon; - new_node->fullpath = old_node->fullpath ? strdup(old_node->fullpath) : NULL; + *new_node = *old_node; + new_node->fullpath = old_node->fullpath ? strdup(old_node->fullpath) : NULL; return new_node; } @@ -3958,7 +3946,7 @@ static void xmb_list_deep_copy(const file_list_t *src, file_list_t *dst) void *src_adata = (void*)menu_entries_get_actiondata_at_offset(src, i); if (src_udata) - file_list_set_userdata(dst, i, xmb_copy_node(src_udata)); + file_list_set_userdata(dst, i, (xmb_node_t*)xmb_copy_node(src_udata)); if (src_adata) { From 718eeb4a39fd2b4505e9959084848c9021973e05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Higor=20Eur=C3=ADpedes?= Date: Sun, 3 Sep 2017 10:25:15 -0300 Subject: [PATCH 3/4] (xmb) Fix memleak Due to file_list_free()'s behavior of free()ing userdata as if it was a simple, contiguous structure xmb_node_t's fullpath member wasn't being free()'d. --- menu/drivers/xmb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/menu/drivers/xmb.c b/menu/drivers/xmb.c index e90f9a9cf1..477d49abfe 100755 --- a/menu/drivers/xmb.c +++ b/menu/drivers/xmb.c @@ -4418,7 +4418,7 @@ menu_ctx_driver_t menu_ctx_xmb = { xmb_menu_init_list, xmb_list_insert, NULL, - NULL, + xmb_list_clear, /* free */ xmb_list_clear, xmb_list_cache, xmb_list_push, From 9a50f5f8b10711f32990a77477cd828be5589a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Higor=20Eur=C3=ADpedes?= Date: Sun, 3 Sep 2017 10:30:01 -0300 Subject: [PATCH 4/4] (file_list.c) Document file_list_free() behavior --- libretro-common/include/lists/file_list.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libretro-common/include/lists/file_list.h b/libretro-common/include/lists/file_list.h index 95b0eb5d2c..67d3875fc9 100644 --- a/libretro-common/include/lists/file_list.h +++ b/libretro-common/include/lists/file_list.h @@ -59,6 +59,16 @@ void *file_list_get_userdata_at_offset(const file_list_t *list, void *file_list_get_actiondata_at_offset(const file_list_t *list, size_t index); +/** + * @brief frees the list + * + * NOTE: This function will also free() the entries actiondata + * and userdata fields if they are non-null. If you store complex + * or non-contiguous data there, make sure you free it's fields + * before calling this function or you might get a memory leak. + * + * @param list + */ void file_list_free(file_list_t *list); bool file_list_append(file_list_t *userdata, const char *path,