menu/ozone: fix three heap-safety issues found during an audit pass

Three issues found during a focused audit of menu/drivers/ozone.c.
The first is a latent double-free in the deep-copy helper that the
struct's own comment warned about; the second is a dangling-pointer
window during teardown across three sites; the third is a dead heap
allocation on every main-menu init.  Same audit class as the recent
xmb pass (commit 9a27e66) since ozone shares much of its data-flow
shape with xmb.

* ozone_copy_node: latent double-free of console_name
  (menu/drivers/ozone.c)

  ozone_node has a comment above its definition saying

      /* If you change this struct, also
         change ozone_alloc_node and
         ozone_copy_node */

  ozone_alloc_node was kept in sync when console_name was added,
  ozone_copy_node was not.  ozone_copy_node does

      *new_node          = *old_node;
      new_node->fullpath = old_node->fullpath
            ? strdup(old_node->fullpath) : NULL;

  -- bitwise-copying every pointer field including console_name,
  then deep-copying only fullpath.  When either the source or the
  copy is later passed to ozone_free_node, the free(node->
  console_name) at the top of that function frees the same buffer
  twice.

  The bug is latent today: console_name is only populated on
  horizontal_list nodes (sidebar playlist tabs), and the only
  caller of ozone_copy_node is ozone_list_deep_copy, which copies
  the vertical selection_buf into selection_buf_old for the
  scroll-transition cache.  Vertical entries don't carry a
  console_name, so no double-free actually fires from current
  call sites.  But the precondition is unenforced -- any future
  feature that copies a horizontal node (or even adds a second
  string field to ozone_node and forgets the same step) re-opens
  the bug.  Mirror fullpath's strdup-or-NULL pattern.

* playlist_db_node_map freed after the nodes it borrows from
  (menu/drivers/ozone.c)

  Same shape as the xmb fix in 9a27e66.  The map's values are
  non-owning ozone_node_t* pointers into ozone->horizontal_list[i].
  userdata, populated in ozone_context_reset_horizontal_list.  In
  ozone_refresh_horizontal_list, the ozone_init error path, and
  ozone_free, the order was: free nodes via ozone_free_list_nodes,
  then RHMAP_FREE the map.  Between those two calls the map holds
  dangling pointers.

  No re-entry path I could trace would actually read the map during
  that window today, but the invariant isn't enforced and the fix
  is a one-line reorder per site.  Free the map first at all three.

* ozone_menu_init_list: dead heap allocation
  (menu/drivers/ozone.c)

  info.exts = strldup("lpl", sizeof("lpl")) is allocated on every
  main-menu init.  DISPLAYLIST_MAIN_MENU never reads info->exts
  (only directory-scanning displaylists like
  DISPLAYLIST_DATABASE_PLAYLISTS_HORIZONTAL do, where the same
  call legitimately filters .lpl files); the buffer is allocated,
  never read, then freed by menu_displaylist_info_free.  Almost
  certainly copy-pasted from ozone_init_horizontal_list at line
  ~5046, same as the xmb instance fixed in 9a27e66.  Drop it.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-28 21:38:57 +02:00
1 parent 9a27e66816
commit d56c728293
1 file changed
+25 -4
+25 -4
View File
@@ -4632,9 +4632,23 @@ static ozone_node_t *ozone_copy_node(const ozone_node_t *old_node)
return NULL;
*new_node = *old_node;
/* Deep-copy heap-owned strings. Bitwise copy above aliased
* the source pointers, so without a strdup here both the
* source and the copy free() the same buffers in their
* respective ozone_free_node() — double free.
*
* console_name is currently only populated on horizontal_list
* entries (sidebar playlist tabs) which never reach
* ozone_copy_node today (it's called from ozone_list_deep_copy
* on the vertical selection_buf), so the bug is latent. The
* struct comment above ozone_node already warned that any
* change to ozone_node must update this function. */
new_node->fullpath = old_node->fullpath
? strdup(old_node->fullpath)
: NULL;
new_node->console_name = old_node->console_name
? strdup(old_node->console_name)
: NULL;
return new_node;
}
@@ -5254,9 +5268,13 @@ static void ozone_refresh_horizontal_list(ozone_handle_t *ozone,
struct menu_state *menu_st = menu_state_get_ptr();
ozone_context_destroy_horizontal_list(ozone);
/* Free the db_node_map BEFORE the nodes it borrows from.
* The map's values are non-owning pointers into the
* horizontal_list's userdata slots, so once those slots are
* free()d the map's stored pointers are dangling. */
RHMAP_FREE(ozone->playlist_db_node_map);
ozone_free_list_nodes(&ozone->horizontal_list, false);
file_list_deinitialize(&ozone->horizontal_list);
RHMAP_FREE(ozone->playlist_db_node_map);
menu_st->flags |= MENU_ST_FLAG_PREVENT_POPULATE;
@@ -9413,11 +9431,13 @@ static void *ozone_init(void **userdata, bool video_is_threaded)
error:
if (ozone)
{
/* See comment in ozone_refresh_horizontal_list: free
* db_node_map before the nodes its values point into. */
RHMAP_FREE(ozone->playlist_db_node_map);
ozone_free_list_nodes(&ozone->horizontal_list, false);
ozone_free_list_nodes(&ozone->selection_buf_old, false);
file_list_deinitialize(&ozone->horizontal_list);
file_list_deinitialize(&ozone->selection_buf_old);
RHMAP_FREE(ozone->playlist_db_node_map);
}
if (menu)
@@ -9444,11 +9464,13 @@ static void ozone_free(void *data)
video_coord_array_free(&ozone->fonts.entries_sublabel.raster_block.carr);
video_coord_array_free(&ozone->fonts.sidebar.raster_block.carr);
/* See comment in ozone_refresh_horizontal_list: free
* db_node_map before the nodes its values point into. */
RHMAP_FREE(ozone->playlist_db_node_map);
ozone_free_list_nodes(&ozone->selection_buf_old, false);
ozone_free_list_nodes(&ozone->horizontal_list, false);
file_list_deinitialize(&ozone->selection_buf_old);
file_list_deinitialize(&ozone->horizontal_list);
RHMAP_FREE(ozone->playlist_db_node_map);
if (ozone->pending_message && *ozone->pending_message)
free(ozone->pending_message);
@@ -13033,7 +13055,6 @@ static bool ozone_menu_init_list(void *data)
menu_displaylist_info_init(&info);
info.label = strdup(MENU_ENUM_LABEL_MAIN_MENU_STR);
info.exts = strldup("lpl", sizeof("lpl"));
info.type_default = FILE_TYPE_PLAIN;
info.enum_idx = MENU_ENUM_LABEL_MAIN_MENU;