Skip to content

Commit b36e9f4

Browse files
committed
menu: fix OOM NULL-derefs in ozone list cloning and menu_list constructor
Three unchecked allocation sites across two menu files, all on hot paths (menu navigation and menu init). === menu/drivers/ozone.c: ozone_copy_node === ozone_node_t *new_node = (ozone_node_t*)malloc(sizeof(*new_node)); *new_node = *old_node; /* NULL-deref on OOM */ new_node->fullpath = old_node->fullpath ? strdup(old_node->fullpath) : NULL; return new_node; Unchecked malloc; the immediate '*new_node = *old_node' struct copy NULL-derefs on OOM. This function is called by ozone_list_deep_copy on every node that has an attached ozone_node_t userdata, which is every entry in ozone's scrolling content lists during navigation-induced list copies (scroll, filter, submenu entry/exit). Fix: NULL-check and return NULL. The single caller (ozone_list_deep_copy) already handles a NULL return cleanly - it writes the result directly into dst->list[j].userdata, and downstream code (ozone_free_list_nodes, ozone's draw paths) already tolerates NULL userdata entries (represents 'non-ozone-specific' list items like folder entries). === menu/drivers/ozone.c: ozone_list_deep_copy actiondata branch === if (src_adata) { void *data = malloc(sizeof(menu_file_list_cbs_t)); memcpy(data, src_adata, sizeof(menu_file_list_cbs_t)); dst->list[j].actiondata = data; } Same pattern: unchecked malloc, immediate memcpy NULL-derefs on OOM. Hit for every list entry that has an actiondata callback block (which is most of them - menu cbs get attached during setting construction). Fix: NULL-check the malloc; on failure leave dst->list[j].actiondata = NULL. The surrounding code already handles NULL actiondata (matches the 'no src_adata' branch directly above - entries without cbs just skip action dispatch). === menu/menu_driver.c: menu_list_new inner mallocs === for (i = 0; i < list->menu_stack_size; i++) { list->menu_stack[i] = (file_list_t*) malloc(sizeof(*list->menu_stack[i])); list->menu_stack[i]->list = NULL; /* NULL-deref */ list->menu_stack[i]->capacity = 0; list->menu_stack[i]->size = 0; } for (i = 0; i < list->selection_buf_size; i++) { list->selection_buf[i] = (file_list_t*) malloc(sizeof(*list->selection_buf[i])); list->selection_buf[i]->list = NULL; /* NULL-deref */ list->selection_buf[i]->capacity = 0; list->selection_buf[i]->size = 0; } Two sibling-drift sites. Each malloc is unchecked and the immediate field writes NULL-deref on OOM. Both loops init the menu's double-buffered navigation stacks; menu_list_new runs once per menu-driver init, so this crashes RetroArch startup if OOM hits at exactly the wrong moment. Fix: NULL-check each malloc, goto error on failure. menu_list_free already handles partially-populated menu_stack / selection_buf arrays correctly (has 'if (!menu_list->menu_stack[i]) continue' guards, and the outer arrays are calloc'd so unallocated slots past the failure point are already NULL). === Thread-safety === ozone_list_deep_copy runs on the main/menu thread during list refresh; menu_list_new runs during menu-driver init before any worker threads are started. No lock discipline changes. === Reachability === * ozone_copy_node: every ozone list refresh with non-empty content (scrolling, filtering, submenu nav). * ozone_list_deep_copy actiondata: every ozone list entry with attached cbs (which is most). * menu_list_new: RetroArch startup menu-driver init. Typically only succeeds or fails once at boot, but the size-2 initial lists are small so this fails only on catastrophic memory pressure. === Not a bug, verified clean === Also audited but found properly NULL-checked or gracefully degrading: * menu/drivers/materialui.c:2365, 9092 - use the 'if (!(x = alloc(...)))' idiom. * menu/drivers/rgui.c:6829 - 'if (!(x = calloc(...)))'. * menu/menu_setting.c:714 (SETTINGS_LIST_APPEND_internal) realloc-to-tmp with NULL check; caller gates config_* on its return. * menu/menu_setting.c:25854, 25915 - NULL-checked with proper unwind. * menu/menu_driver.c:4161, 4254 - NULL-checked. * menu/menu_screensaver.c:309 - NULL-checked. * menu/cbs/ and menu/widgets/ - no raw alloc sites found.
1 parent 5874334 commit b36e9f4

2 files changed

Lines changed: 29 additions & 2 deletions

File tree

menu/drivers/ozone.c

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4623,6 +4623,14 @@ static ozone_node_t *ozone_copy_node(const ozone_node_t *old_node)
46234623
{
46244624
ozone_node_t *new_node = (ozone_node_t*)malloc(sizeof(*new_node));
46254625

4626+
/* NULL-check the malloc: '*new_node = *old_node' on the next
4627+
* line NULL-derefs on OOM. Caller in ozone_list_deep_copy
4628+
* handles a NULL return - it just leaves
4629+
* dst->list[j].userdata = NULL, which matches the 'no
4630+
* src_udata' branch. */
4631+
if (!new_node)
4632+
return NULL;
4633+
46264634
*new_node = *old_node;
46274635
new_node->fullpath = old_node->fullpath
46284636
? strdup(old_node->fullpath)
@@ -4662,8 +4670,17 @@ static void ozone_list_deep_copy(const file_list_t *src,
46624670
if (src_adata)
46634671
{
46644672
void *data = malloc(sizeof(menu_file_list_cbs_t));
4665-
memcpy(data, src_adata, sizeof(menu_file_list_cbs_t));
4666-
dst->list[j].actiondata = data;
4673+
/* NULL-check the malloc before the memcpy on the next
4674+
* line NULL-derefs. On OOM leave actiondata NULL -
4675+
* matches the 'no src_adata' branch above; file_list
4676+
* consumers already handle NULL actiondata entries. */
4677+
if (data)
4678+
{
4679+
memcpy(data, src_adata, sizeof(menu_file_list_cbs_t));
4680+
dst->list[j].actiondata = data;
4681+
}
4682+
else
4683+
dst->list[j].actiondata = NULL;
46674684
}
46684685

46694686
++j;

menu/menu_driver.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1422,6 +1422,13 @@ static menu_list_t *menu_list_new(const menu_ctx_driver_t *menu_driver_ctx)
14221422
{
14231423
list->menu_stack[i] = (file_list_t*)
14241424
malloc(sizeof(*list->menu_stack[i]));
1425+
/* NULL-check before the three field writes below NULL-deref
1426+
* on OOM. menu_list_free tolerates a partially-populated
1427+
* menu_stack (has 'if (!menu_list->menu_stack[i]) continue'
1428+
* guards), and the array itself was calloc'd above so
1429+
* unallocated slots past 'i' are already NULL. */
1430+
if (!list->menu_stack[i])
1431+
goto error;
14251432
list->menu_stack[i]->list = NULL;
14261433
list->menu_stack[i]->capacity = 0;
14271434
list->menu_stack[i]->size = 0;
@@ -1431,6 +1438,9 @@ static menu_list_t *menu_list_new(const menu_ctx_driver_t *menu_driver_ctx)
14311438
{
14321439
list->selection_buf[i] = (file_list_t*)
14331440
malloc(sizeof(*list->selection_buf[i]));
1441+
/* Same pattern as the menu_stack loop above. */
1442+
if (!list->selection_buf[i])
1443+
goto error;
14341444
list->selection_buf[i]->list = NULL;
14351445
list->selection_buf[i]->capacity = 0;
14361446
list->selection_buf[i]->size = 0;

0 commit comments

Comments
 (0)