mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
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:
1 parent
fae20db89a
commit
9a27e66816
1 file changed
+36
-9
+36
-9
@@ -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;
|
||||
|
||||
|
||||
Reference in new issue
Block a user