Skip to content

Commit d56c728

Browse files
committed
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.
1 parent 9a27e66 commit d56c728

1 file changed

Lines changed: 25 additions & 4 deletions

File tree

menu/drivers/ozone.c

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4632,9 +4632,23 @@ static ozone_node_t *ozone_copy_node(const ozone_node_t *old_node)
46324632
return NULL;
46334633

46344634
*new_node = *old_node;
4635+
/* Deep-copy heap-owned strings. Bitwise copy above aliased
4636+
* the source pointers, so without a strdup here both the
4637+
* source and the copy free() the same buffers in their
4638+
* respective ozone_free_node() — double free.
4639+
*
4640+
* console_name is currently only populated on horizontal_list
4641+
* entries (sidebar playlist tabs) which never reach
4642+
* ozone_copy_node today (it's called from ozone_list_deep_copy
4643+
* on the vertical selection_buf), so the bug is latent. The
4644+
* struct comment above ozone_node already warned that any
4645+
* change to ozone_node must update this function. */
46354646
new_node->fullpath = old_node->fullpath
46364647
? strdup(old_node->fullpath)
46374648
: NULL;
4649+
new_node->console_name = old_node->console_name
4650+
? strdup(old_node->console_name)
4651+
: NULL;
46384652

46394653
return new_node;
46404654
}
@@ -5254,9 +5268,13 @@ static void ozone_refresh_horizontal_list(ozone_handle_t *ozone,
52545268
struct menu_state *menu_st = menu_state_get_ptr();
52555269

52565270
ozone_context_destroy_horizontal_list(ozone);
5271+
/* Free the db_node_map BEFORE the nodes it borrows from.
5272+
* The map's values are non-owning pointers into the
5273+
* horizontal_list's userdata slots, so once those slots are
5274+
* free()d the map's stored pointers are dangling. */
5275+
RHMAP_FREE(ozone->playlist_db_node_map);
52575276
ozone_free_list_nodes(&ozone->horizontal_list, false);
52585277
file_list_deinitialize(&ozone->horizontal_list);
5259-
RHMAP_FREE(ozone->playlist_db_node_map);
52605278

52615279
menu_st->flags |= MENU_ST_FLAG_PREVENT_POPULATE;
52625280

@@ -9413,11 +9431,13 @@ static void *ozone_init(void **userdata, bool video_is_threaded)
94139431
error:
94149432
if (ozone)
94159433
{
9434+
/* See comment in ozone_refresh_horizontal_list: free
9435+
* db_node_map before the nodes its values point into. */
9436+
RHMAP_FREE(ozone->playlist_db_node_map);
94169437
ozone_free_list_nodes(&ozone->horizontal_list, false);
94179438
ozone_free_list_nodes(&ozone->selection_buf_old, false);
94189439
file_list_deinitialize(&ozone->horizontal_list);
94199440
file_list_deinitialize(&ozone->selection_buf_old);
9420-
RHMAP_FREE(ozone->playlist_db_node_map);
94219441
}
94229442

94239443
if (menu)
@@ -9444,11 +9464,13 @@ static void ozone_free(void *data)
94449464
video_coord_array_free(&ozone->fonts.entries_sublabel.raster_block.carr);
94459465
video_coord_array_free(&ozone->fonts.sidebar.raster_block.carr);
94469466

9467+
/* See comment in ozone_refresh_horizontal_list: free
9468+
* db_node_map before the nodes its values point into. */
9469+
RHMAP_FREE(ozone->playlist_db_node_map);
94479470
ozone_free_list_nodes(&ozone->selection_buf_old, false);
94489471
ozone_free_list_nodes(&ozone->horizontal_list, false);
94499472
file_list_deinitialize(&ozone->selection_buf_old);
94509473
file_list_deinitialize(&ozone->horizontal_list);
9451-
RHMAP_FREE(ozone->playlist_db_node_map);
94529474

94539475
if (ozone->pending_message && *ozone->pending_message)
94549476
free(ozone->pending_message);
@@ -13033,7 +13055,6 @@ static bool ozone_menu_init_list(void *data)
1303313055
menu_displaylist_info_init(&info);
1303413056

1303513057
info.label = strdup(MENU_ENUM_LABEL_MAIN_MENU_STR);
13036-
info.exts = strldup("lpl", sizeof("lpl"));
1303713058
info.type_default = FILE_TYPE_PLAIN;
1303813059
info.enum_idx = MENU_ENUM_LABEL_MAIN_MENU;
1303913060

0 commit comments

Comments
 (0)