Skip to content

Commit 494196f

Browse files
committed
menu/drivers: fix leaks, OOM NULL-derefs and dead-code guards across xmb, ozone, materialui
Five bugs across the xmb, ozone, and materialui menu drivers, surfaced during an alloc-site sweep of menu/drivers/. === menu/drivers/xmb.c: xmb_messagebox leak === static void xmb_messagebox(void *data, const char *message) { xmb_handle_t *xmb = (xmb_handle_t*)data; if (xmb && message && *message) xmb->box_message = strdup(message); } Overwrote any previously-set xmb->box_message without freeing it. The render path at line ~9215 consumes box_message once (strlcpy to stack msg, then free + NULL), but nothing prevents xmb_messagebox from being called twice between renders (rapid error notifications, background task completion triggering a second box before the menu repaints). Pre-patch every additional call leaked the previous message. Fix: free the old box_message before strdup'ing the new one, mirroring ozone's equivalent pending_message handling. === menu/drivers/xmb.c: xmb_init_ribbon OOM NULL-deref === xmb_init_ribbon allocated two scratch arrays and immediately wrote into them in a tight loop: float *dummy = (float*)calloc(4 * vertices_total, sizeof(float)); float *ribbon_verts = (float*)calloc(2 * vertices_total, sizeof(float)); for (r = 0; r < XMB_RIBBON_ROWS - 1; r++) { for (c = 0; c < XMB_RIBBON_COLS; c++) { ... xmb_ribbon_set_vertex(ribbon_verts, i, r, col); ... } } coords.color = dummy; coords.vertex = ribbon_verts; coords.tex_coord = dummy; coords.lut_tex_coord = dummy; ... video_coord_array_append(ca, &coords, coords.vertices); Both callocs were unchecked. xmb_ribbon_set_vertex does 'ribbon_verts[idx++] = ...' unconditionally, and video_coord_array_append reads through coords.color / tex_coord / lut_tex_coord (= dummy) for every vertex - NULL-derefs on OOM. Fix: NULL-check both callocs and skip ribbon init entirely on OOM. The ribbon is a decorative background animation; degraded visuals are strictly better than a crash. Existing free()s at the bottom are NULL-safe. === menu/drivers/xmb.c: .lvw console_name leak === In xmb_context_reset_horizontal_list, the .lpl branch correctly frees the old console_name before strdup'ing the new one: if (node->console_name) free(node->console_name); if (console_name) node->console_name = strdup(console_name); else node->console_name = strdup(path); but the .lvw (Explore View) branch just overwrote: node->console_name = strdup(path + strlen(...) + 2); xmb_context_reset_horizontal_list is called on every theme change / refresh, so without the free the .lvw console_name from the previous reset leaked. Fix: match the .lpl pattern - free old before strdup. === menu/drivers/ozone.c: .lvw console_name leak === Twin of the xmb bug above - ozone_context_reset_horizontal_ list had the same missing-free in its .lvw branch, while the .lpl branch frees correctly. Same fix. === menu/drivers/materialui.c: dead-code !node guard === materialui_list_insert had a classic dead-code NULL-guard: node = (materialui_node_t*)list->list[i].userdata; if (!node) { node = (materialui_node_t*)malloc(sizeof(materialui_node_t)); node->icon_type = MUI_ICON_TYPE_NONE; node->icon_texture_index = 0; ... /* 24 lines of unconditional node-> writes */ } else { gfx_thumbnail_reset(&node->thumbnails.primary); gfx_thumbnail_reset(&node->thumbnails.secondary); thumbnail_reset = true; } if (!node) return; node->icon_type = MUI_ICON_TYPE_NONE; ... The 'if (!node) return' at the bottom of that block only fires after the preceding 24 lines have already unconditionally dereferenced node. On OOM the field writes NULL-deref long before the guard can run - same class of bug as the iohid hoist fixed in 9e840aa. Fix: hoist the NULL-check to immediately after the malloc, and store NULL into list->list[i].userdata so the list entry has the same 'no node' state it would have had if userdata had been NULL from the start. Downstream materialui renders NULL-userdata entries as bare items without the materialui- specific metadata (icons, thumbnails) - degraded but functional. The downstream dead-code 'if (!node) return' at line ~11308 is now truly unreachable from all three prior paths (if-branch succeeds and returns early on OOM, if-branch fills node; else- branch uses non-NULL existing node). Left in place as defensive scaffolding rather than churning the diff further. === Swept-clean in the same pass === Verified NULL-checked / leak-free in the same files: - xmb.c: xmb_alloc_node malloc (guarded), box_message / bg_ file_path other sites (free-before-strdup), init callocs for menu_handle_t and xmb_handle_t (guarded), tab-switch menu_stack label strdups (free-before-strdup), xmb_menu_ init_list (goto error cleanup). - ozone.c: ozone_copy_node malloc (guarded with prior audit comment), ozone_list_deep_copy strdup/malloc patterns (guarded with prior audit comment), pending_message strdup (free-before-strdup), init callocs, ozone_alloc_node malloc (guarded). - materialui.c: playlist.icons malloc (guarded), tab-switch label strdups (free-before-strdup), init callocs (guarded), playlist_file / image_file strdups (downstream NULL-guarded). - rgui.c: all 9 alloc sites guarded. rgui_set_aspect_ratio has the right shape: pre-alloc free at entry, per-step NULL-checks on each buffer alloc, caller cleanup via goto error. The 'leak on mid-function failure' impression was wrong - the next entry (or init failure cleanup) frees whatever partial state survived. Thread-safety: all menu driver paths run on the main menu thread; no concurrency concerns. Reachability: - xmb_messagebox leak: every rapid double-message (cheevo unlock + task completion, rapid config errors, etc.) - xmb_init_ribbon OOM: one-shot at xmb menu driver init. - xmb / ozone .lvw leak: every theme change / refresh while at least one Explore View entry is in the horizontal list. Explore View is opt-in but common. - materialui dead-code guard: every menu list insertion against a fresh slot would crash on OOM pre-patch. materialui_list_insert fires on every menu push - the main menu alone inserts ~20 entries on boot. Scope: local to each file; no API changes, no header changes.
1 parent 552cf85 commit 494196f

3 files changed

Lines changed: 60 additions & 0 deletions

File tree

menu/drivers/materialui.c

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11252,6 +11252,24 @@ static void materialui_list_insert(void *userdata,
1125211252
{
1125311253
node = (materialui_node_t*)malloc(sizeof(materialui_node_t));
1125411254

11255+
/* NULL-check the malloc: the field writes below
11256+
* (node->icon_type etc., 24+ lines) unconditionally
11257+
* dereference node and NULL-deref on OOM. The existing
11258+
* 'if (!node) return;' at line ~11290 was dead code -
11259+
* the dereference at line 11255 crashed first.
11260+
*
11261+
* On failure we need to replicate what the 'if (!node)
11262+
* return;' below was intended to do: store NULL into the
11263+
* list entry's userdata so subsequent reads see no node
11264+
* and skip node-specific rendering. The item stays in
11265+
* the list but renders without the materialui-specific
11266+
* node metadata (icons, thumbnails). */
11267+
if (!node)
11268+
{
11269+
list->list[i].userdata = NULL;
11270+
return;
11271+
}
11272+
1125511273
node->icon_type = MUI_ICON_TYPE_NONE;
1125611274
node->icon_texture_index = 0;
1125711275
node->entry_width = 0.0f;

menu/drivers/ozone.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5235,6 +5235,13 @@ static void ozone_context_reset_horizontal_list(ozone_handle_t *ozone)
52355235
else if (string_ends_with_size(ozone->horizontal_list.list[i].label, ".lvw",
52365236
strlen(ozone->horizontal_list.list[i].label), STRLEN_CONST(".lvw")))
52375237
{
5238+
/* Free any previously-set console_name before
5239+
* overwriting; matches the .lpl branch above.
5240+
* ozone_context_reset_horizontal_list is called on
5241+
* every theme change / refresh, so without this free
5242+
* the console_name from the previous reset leaks. */
5243+
if (node->console_name)
5244+
free(node->console_name);
52385245
node->console_name = strdup(path + strlen(msg_hash_to_str(MENU_ENUM_LABEL_EXPLORE_VIEW)) + 2);
52395246
node->icon = ozone->icons_textures[OZONE_ENTRIES_ICONS_TEXTURE_CURSOR];
52405247
}

menu/drivers/xmb.c

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1094,7 +1094,19 @@ static void xmb_messagebox(void *data, const char *message)
10941094
{
10951095
xmb_handle_t *xmb = (xmb_handle_t*)data;
10961096
if (xmb && message && *message)
1097+
{
1098+
/* Free any previously-set box_message before overwriting.
1099+
* The render path at line ~9215 consumes box_message
1100+
* exactly once (strlcpy to stack msg, then free + NULL),
1101+
* but nothing prevents xmb_messagebox from being called
1102+
* twice between renders (e.g. rapid error notifications,
1103+
* or a second messagebox triggered by a background task
1104+
* completion before the menu has repainted). Pre-patch
1105+
* the second strdup leaked the first one. */
1106+
if (xmb->box_message)
1107+
free(xmb->box_message);
10971108
xmb->box_message = strdup(message);
1109+
}
10981110
}
10991111

11001112
static void xmb_render_messagebox_internal(
@@ -3085,6 +3097,13 @@ static void xmb_context_reset_horizontal_list(xmb_handle_t *xmb)
30853097
else if (string_ends_with_size(xmb->horizontal_list.list[i].label, ".lvw",
30863098
strlen(xmb->horizontal_list.list[i].label), STRLEN_CONST(".lvw")))
30873099
{
3100+
/* Free any previously-set console_name before
3101+
* overwriting; matches the .lpl branch above.
3102+
* xmb_context_reset_horizontal_list is called on every
3103+
* theme change / refresh, so without this free the
3104+
* console_name from the previous reset leaks. */
3105+
if (node->console_name)
3106+
free(node->console_name);
30883107
node->console_name = strdup(path + strlen(msg_hash_to_str(MENU_ENUM_LABEL_EXPLORE_VIEW)) + 2);
30893108
node->icon = xmb->textures.list[XMB_TEXTURE_CURSOR];
30903109
}
@@ -9284,6 +9303,22 @@ static void xmb_init_ribbon(xmb_handle_t * xmb)
92849303
float *dummy = (float*)calloc(4 * vertices_total, sizeof(float));
92859304
float *ribbon_verts = (float*)calloc(2 * vertices_total, sizeof(float));
92869305

9306+
/* NULL-check both callocs: the for-loop below unconditionally
9307+
* writes into ribbon_verts via xmb_ribbon_set_vertex, and the
9308+
* video_coord_array_append call at the bottom passes dummy
9309+
* as color/tex_coord/lut_tex_coord which the underlying
9310+
* append implementation would copy from (reading NULL).
9311+
* Skip ribbon init entirely on OOM - the ribbon is a
9312+
* decorative background animation; its absence is visually
9313+
* degraded but not functionally broken. The free()s at
9314+
* the bottom are NULL-safe. */
9315+
if (!dummy || !ribbon_verts)
9316+
{
9317+
free(dummy);
9318+
free(ribbon_verts);
9319+
return;
9320+
}
9321+
92879322
/* Set up vertices */
92889323
for (r = 0; r < XMB_RIBBON_ROWS - 1; r++)
92899324
{

0 commit comments

Comments
 (0)