mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
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 (commit9a27e66) 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 in9a27e66. 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 in9a27e66. Drop it.
This commit is contained in:
1 parent
9a27e66816
commit
d56c728293
1 file changed
+25
-4
+25
-4
@@ -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;
|
||||
|
||||
|
||||
Reference in new issue
Block a user