menu/xmb: fix five heap-safety issues found during an audit pass

Five independent issues found during a focused audit of menu/drivers/
xmb.c.  Two are real corruption vectors driven by uninitialized memory
and a stale-pointer free ordering; two are size_t underflows in the
ticker helpers that don't fire on the current XMB call sites but
violate the contract a future caller could plausibly hit; and one is
an unused heap allocation on every main-menu init.  Grouped together
because they share an audit context and none is large enough to stand
alone.

* xmb_alloc_node: uninitialized gfx_thumbnail_t fields
  (menu/drivers/xmb.c)

  The comment above xmb_alloc_node deliberately uses malloc instead
  of calloc to avoid zeroing the multi-KB thumbnail_path_data char
  arrays on every node alloc on big lists (MAME, FBA).  Per-field
  init only sets node->thumbnail_icon.icon.texture = 0, leaving the
  rest of the gfx_thumbnail_t substruct (status, flags, alpha,
  delay_timer, width, height) and all of thumbnail_path_data
  uninitialized.

  When a node is later freed without its thumbnail ever having been
  loaded -- the common case for the line-edit path at the bottom of
  the file (xmb_list_init -> xmb_alloc_node, allocated and stored
  into list->list[i].userdata) -- xmb_free_node calls
  gfx_thumbnail_reset, which reads `thumbnail->flags &
  GFX_THUMB_FLAG_FADE_ACTIVE` and `thumbnail->texture` before
  initializing them.  Uninit `flags` may match the bit and trigger
  gfx_animation_kill_by_tag against arbitrary state; uninit
  `texture != 0` feeds a garbage handle to
  video_driver_texture_unload.  The latter is the actual heap
  corruption vector -- driver-side texture maps treat the handle as
  an index/key into their own allocations.

  Separately, the lazy thumbnail-path resolution in the render path
  reads `thumbnail_icon->thumbnail_path_data.icon_path[0]` to decide
  whether resolution is needed; on a freshly-allocated node that
  byte is uninitialized.

  Zero the small gfx_thumbnail_t substruct (~32 bytes) and clear
  icon_path[0] only.  This preserves the original optimization that
  kept thumbnail_path_data's PATH_MAX/NAME_MAX char arrays unzeroed,
  since the path-data fields are written via fill_pathname/strlcpy
  before any read of those fields.

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

  The map's values are non-owning xmb_node_t* pointers into
  xmb->horizontal_list[i].userdata, populated in
  xmb_context_reset_horizontal_list.  In xmb_refresh_horizontal_list,
  xmb_free, and the xmb_init error path the order was: free nodes
  via xmb_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 (the menu is single-threaded and
  xmb_free_list_nodes calls only gfx_thumbnail_reset and free), but
  the invariant isn't enforced and the fix is a one-line reorder.
  Free the map first at all three sites.

* xmb_animation_line_ticker_smooth: size_t underflow in fade-string
  copy length (menu/drivers/xmb.c)

  At the two fade-string memcpy sites:

      if (copy_len >= line_ticker->top_fade_str_len)
         copy_len = line_ticker->top_fade_str_len - 1;

  if top_fade_str_len == 0 the comparison is true, copy_len
  underflows to SIZE_MAX, the memcpy writes effectively unbounded
  bytes, and the trailing NUL store at line_ticker->top_fade_str
  [copy_len] writes at SIZE_MAX offset.  Same pattern for
  bottom_fade_str_len.

  XMB's only caller passes sizeof(stack_array) so the live path is
  safe today, but xmb_animation_line_ticker_smooth takes the public
  gfx_animation_ctx_line_ticker_smooth_t shape.  Add the missing
  `_str_len > 0` guard to each branch.

* xmb_animation_line_ticker: same underflow on line_ticker->len
  (menu/drivers/xmb.c)

  max_copy = line_ticker->len - 1 underflows when len == 0, then
  copy_len > max_copy is false and the memcpy at line ~4566 writes
  the full block size.  The early sanity check guards line_len < 1
  and max_lines < 1 but not len < 1.  Add it for symmetry with the
  smooth variant; same caller-contract argument.

* xmb_menu_init_list: dead heap allocation
  (menu/drivers/xmb.c)

  info.exts = strldup("lpl", sizeof("lpl")) has been allocated on
  every main-menu init since the function was introduced.
  DISPLAYLIST_MAIN_MENU never reads info->exts (only
  directory-scanning displaylists like
  DISPLAYLIST_DATABASE_PLAYLISTS_HORIZONTAL do, where the same line
  legitimately filters .lpl files); the buffer is allocated, never
  read, then freed by menu_displaylist_info_free.  Almost certainly
  copy-pasted from xmb_init_horizontal_list at line ~2927.  Drop it.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-28 21:27:02 +02:00
1 parent fae20db89a
commit 9a27e66816
1 file changed
+36 -9
+36 -9
View File
@@ -640,7 +640,20 @@ const char* xmb_theme_ident(void)
}
/* NOTE: This exists because calloc()ing xmb_node_t is expensive
* when you can have big lists like MAME and fba playlists */
* when you can have big lists like MAME and fba playlists.
*
* Per-field init only — DO NOT add a wholesale memset of the
* thumbnail_path_data substruct here, that's the multi-KB block
* we're explicitly avoiding zeroing on the hot path. Only the
* small `gfx_thumbnail_t icon` substruct and the icon_path's
* first byte must be zero, because:
* - xmb_free_node -> gfx_thumbnail_reset reads `texture` and
* `flags` before initializing them; uninit `texture != 0`
* would feed garbage to video_driver_texture_unload, and
* uninit `flags & FADE_ACTIVE` would call
* gfx_animation_kill_by_tag on stale state.
* - The lazy thumbnail path resolution in xmb_render reads
* `icon_path[0]` to decide whether resolution is needed. */
static xmb_node_t *xmb_alloc_node(void)
{
xmb_node_t *node = (xmb_node_t*)malloc(sizeof(*node));
@@ -651,7 +664,9 @@ static xmb_node_t *xmb_alloc_node(void)
node->alpha = node->label_alpha = 0;
node->zoom = node->x = node->y = 0;
node->icon = node->content_icon = 0;
node->thumbnail_icon.icon.texture = 0;
memset(&node->thumbnail_icon.icon, 0,
sizeof(node->thumbnail_icon.icon));
node->thumbnail_icon.thumbnail_path_data.icon_path[0] = '\0';
node->fullpath = NULL;
node->console_name = NULL;
@@ -3153,9 +3168,14 @@ static void xmb_refresh_horizontal_list(xmb_handle_t *xmb)
xmb_context_destroy_horizontal_list(xmb);
/* 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. Reverse
* the order to keep the dangling-pointer window closed. */
RHMAP_FREE(xmb->playlist_db_node_map);
xmb_free_list_nodes(&xmb->horizontal_list, false);
file_list_deinitialize(&xmb->horizontal_list);
RHMAP_FREE(xmb->playlist_db_node_map);
menu_st->flags |= MENU_ST_FLAG_PREVENT_POPULATE;
@@ -4445,7 +4465,8 @@ static bool xmb_animation_line_ticker(gfx_animation_t *p_anim, gfx_animation_ctx
return false;
if ( (!line_ticker->str || !*line_ticker->str)
|| (line_ticker->line_len < 1)
|| (line_ticker->max_lines < 1))
|| (line_ticker->max_lines < 1)
|| (line_ticker->len < 1))
goto end;
/* Line wrap input string */
@@ -5023,7 +5044,8 @@ static bool xmb_animation_line_ticker_smooth(gfx_animation_t *p_anim, gfx_animat
bottom_fade_line_index %= num_lines;
}
if (top_fade_line_index < num_lines)
if (top_fade_line_index < num_lines
&& line_ticker->top_fade_str_len > 0)
{
size_t copy_len = line_lengths[top_fade_line_index];
if (copy_len >= line_ticker->top_fade_str_len)
@@ -5033,7 +5055,8 @@ static bool xmb_animation_line_ticker_smooth(gfx_animation_t *p_anim, gfx_animat
line_ticker->top_fade_str[copy_len] = '\0';
}
if (bottom_fade_line_index < num_lines)
if (bottom_fade_line_index < num_lines
&& line_ticker->bottom_fade_str_len > 0)
{
size_t copy_len = line_lengths[bottom_fade_line_index];
if (copy_len >= line_ticker->bottom_fade_str_len)
@@ -9534,9 +9557,12 @@ static void *xmb_init(void **userdata, bool video_is_threaded)
error:
free(menu);
/* See comment in xmb_refresh_horizontal_list: the map's
* values are non-owning pointers into the horizontal_list,
* so it must be freed before the nodes are. */
RHMAP_FREE(xmb->playlist_db_node_map);
xmb_free_list_nodes(&xmb->horizontal_list, false);
file_list_deinitialize(&xmb->horizontal_list);
RHMAP_FREE(xmb->playlist_db_node_map);
return NULL;
}
@@ -9551,9 +9577,11 @@ static void xmb_free(void *data)
xmb_icon_load_gen++;
xmb_ctx_icon_load_gen++;
/* See comment in xmb_refresh_horizontal_list: free the
* db_node_map before the nodes its values point into. */
RHMAP_FREE(xmb->playlist_db_node_map);
xmb_free_list_nodes(&xmb->horizontal_list, false);
file_list_deinitialize(&xmb->horizontal_list);
RHMAP_FREE(xmb->playlist_db_node_map);
video_coord_array_free(&xmb->raster_block.carr);
video_coord_array_free(&xmb->raster_block2.carr);
@@ -10038,7 +10066,6 @@ static bool xmb_menu_init_list(void *data)
menu_displaylist_info_init(&info);
info.label = strdup(msg_hash_to_str(MENU_ENUM_LABEL_MAIN_MENU));
info.exts = strldup("lpl", sizeof("lpl"));
info.type_default = FILE_TYPE_PLAIN;
info.enum_idx = MENU_ENUM_LABEL_MAIN_MENU;