Merge pull request #5399 from heuripedes/master

Fix xmb_node_t memleaks

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,

menu/drivers/xmb.c

@@ -448,21 +448,38 @@ static void xmb_free_node(xmb_node_t *node)
     free(node);
 }
 
-/* NOTE: This is faster than memcpy()ing xmb_node_t in most cases
+/**
- * because most nodes have small (less than 200 bytes) fullpath */
+ * @brief frees all xmb_node_t in a file_list_t
-static xmb_node_t *xmb_copy_node(void *p)
+ *
+ * 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)
+   {
+      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;
+
+      if (actiondata)
+         file_list_free_actiondata(list, i);
+   }
+}
+
+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            = *old_node;
-   new_node->label_alpha  = old_node->label_alpha;
+   new_node->fullpath   = old_node->fullpath ? strdup(old_node->fullpath) : NULL;
-   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;
 
    return new_node;
 }
@@ -1969,7 +1986,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 +3510,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 +3526,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 +3923,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_free_list_nodes(list, false);
-   {
-      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;
-   }
 }
 
 static void xmb_list_deep_copy(const file_list_t *src, file_list_t *dst)
@@ -3911,15 +3934,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)
+   /* use true here because file_list_copy() doesn't free actiondata */
-   {
+   xmb_free_list_nodes(dst, true);
-      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 */
-   }
-
    file_list_copy(src, dst);
 
    size = dst->size;
@@ -3930,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)
       {
@@ -4402,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,