menu + file_list: NULL-check init_item_file strdups, contentless_cores entry, and menu_explore arena

Three related OOM / NULL-deref fixes found during a menu
subsystem sweep.  All share the shape 'alloc/strdup ->
immediate deref without guard' and have been triggered or are
reachable via menu-driven code paths.

=== libretro-common/lists/file_list.c: init_item_file strdup(NULL) ===

    static INLINE void init_item_file(struct item_file *item,
        const char *path, const char *label, unsigned type,
        size_t directory_ptr, size_t entry_idx)
    {
        item->path  = strdup(path);        /* strdup(NULL) = UB */
        item->label = strdup(label);       /* strdup(NULL) = UB */
        ...

strdup(NULL) is undefined behaviour - glibc crashes, musl
returns NULL, some platforms segfault.  This is reachable
because callers of file_list_insert (the only caller of
init_item_file) can pass a NULL path.

Verified via:
  * menu/menu_driver.c:4224 menu_entries_prepend calls
    file_list_insert(list, path, label, ...) with path coming
    unchecked from callers in menu/menu_displaylist.c.
  * menu_displaylist.c call sites (lines 303, 311, 318, 326
    etc.) pass msg_hash_to_str(...) as path.  msg_hash_to_str
    is defined to return NULL for enum values no language
    handler recognises (msg_hash.c:584: 'const char *ret =
    NULL;' + fallthrough default).

The sibling file_list_append at line 117 of the same file
already does the right thing ('if (path) item->path =
strdup(path);').  This patch brings init_item_file in line.

Fix: NULL-gate both strdup calls.

=== menu/menu_contentless_cores.c: entry malloc unchecked ===

    contentless_core_info_entry_t *entry =
          (contentless_core_info_entry_t*)malloc(sizeof(*entry));
    size_t _len = strlcpy(licenses_str, ...);     /* unrelated */
    ...
    entry->licenses_str = strdup(licenses_str);   /* NULL-deref on OOM */
    entry->runtime.runtime_str     = NULL;
    ...
    RHMAP_SET_STR(state->info_entries, core_info->core_file_id.str, entry);

Unchecked malloc; the entry->* field writes below NULL-deref
on OOM.  Fires during contentless cores menu population - one
entry per core that supports SUPPORTS_NO_GAME, typically
hundreds of allocations over the enumeration loop.

Fix: 'if (!entry) continue;' at the top of the if-block, so
the outer for-loop over core_info_list continues to the next
core.  User-visible effect on OOM is that the failing core
doesn't appear in the contentless-cores list; picked up on
the next menu refresh.

=== menu/menu_explore.c: ex_arena_grow malloc unchecked ===

    static void ex_arena_grow(ex_arena *arena, size_t min_size)
    {
       size_t _len = EX_ARENA_ALIGN_UP(...);
       arena->ptr  = (char *)malloc(_len);
       arena->end  = arena->ptr + _len;          /* UB on NULL */
       RBUF_PUSH(arena->blocks, arena->ptr);     /* pushes NULL */
    }

    static void *ex_arena_alloc(ex_arena *arena, size_t len)
    {
       void *ptr = NULL;
       if (len > (size_t)(arena->end - arena->ptr))
          ex_arena_grow(arena, len);
       ptr = arena->ptr;                          /* stale NULL */
       ...
       return ptr;
    }

Arena allocator backing the Explore menu's string/index store.
On OOM malloc returns NULL; the current code computes
'arena->end = NULL + _len' (pointer arithmetic on NULL is UB)
and pushes NULL into the blocks RBUF.  ex_arena_alloc then
returns NULL to the three callers (~line 376 for new
explore_string_t, ~line 797 for e->original_title, ~line 810
for e->split), each of which does an immediate memcpy into
the returned pointer and NULL-derefs.

Fix: three-part change:

  1. ex_arena_grow: malloc-to-tmp; on failure return without
     mutating arena->ptr/end/blocks, so the arena stays in its
     pre-grow (exhausted but valid) state.

  2. ex_arena_alloc: re-check capacity after ex_arena_grow; if
     still insufficient (i.e. grow failed), return NULL so
     callers can bail.

  3. Three callers: NULL-gate the memcpy / continue as
     appropriate.  The for-loop in explore_add_unique_string
     skips one indexing entry; the EXPLORE_SHOW_ORIGINAL_TITLE
     and split blocks leave their fields at their pre-
     initialised NULL values.

The alternative (making ex_arena_grow bool and propagating
through ex_arena_alloc's signature) would have been cleaner
but churns every call site and the internal ex_arena type, so
sticking with NULL-propagation minimises the diff while
covering the crash path.

=== Not a bug, verified clean during this pass ===

  * menu/menu_displaylist.c - zero raw alloc sites; uses
    string_list_new, file_list_* wrappers throughout.  All
    wrappers already NULL-tolerate their results.
  * menu/menu_driver.c - 9 alloc sites, all NULL-checked
    (several already fixed in earlier commits in this series).
  * menu/menu_explore.c:473 (explore_state_t calloc) - NULL-
    checked with early-return.
  * libretro-common/lists/file_list.c:file_list_reserve -
    realloc-to-tmp with NULL-check, clean.
  * libretro-common/lists/file_list.c:file_list_append -
    NULL-gates strdup on path / label (the pattern this patch
    brings init_item_file in line with).

=== Out of scope ===

RBUF_PUSH itself has latent OOM issues: rbuf__grow returns
unchanged buf on realloc failure with capacity unbumped, so the
'(b)[RBUF__HDR(b)->len++] = val' push writes past the allocated
end.  On first-time PUSH (buf == NULL) it can also deref NULL
via RBUF__HDR.  Fixing this is a libretro-common-wide change
affecting every RBUF user in the tree; out of scope for this
patch.  The arena fix above avoids the realloc-failure path
for this one caller by skipping RBUF_PUSH entirely when malloc
fails.

=== Thread-safety ===

All three fix sites are invoked on the menu/main thread during
driver init, menu rebuilds, or user-driven list refreshes.  No
shared-state mutation changes; no lock discipline changes.

=== Reachability ===

  * init_item_file: every menu list population (frequent, every
    navigation).
  * contentless_cores: menu open with contentless cores present
    in the core list.
  * ex_arena_grow: first time the Explore menu is opened with
    a non-trivial playlist collection, then on every rescan.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-24 02:39:02 +02:00
1 parent 4cf2d214ff
commit 1c7e114364
3 files changed
+61 -6

No files matched your search

+8 -2
View File
@@ -79,8 +79,14 @@ static INLINE void init_item_file(struct item_file *item,
const char *path, const char *label, unsigned type,
size_t directory_ptr, size_t entry_idx)
{
item->path = strdup(path);
item->label = strdup(label);
/* NULL-gate both strdup calls: strdup(NULL) is undefined
* behaviour (glibc crashes). The sibling file_list_append
* uses the same gating pattern. Callers have been seen to
* pass NULL path here via menu_entries_prepend when
* msg_hash_to_str returns NULL for an enum that no active
* language handler recognises. */
item->path = path ? strdup(path) : NULL;
item->label = label ? strdup(label) : NULL;
item->alt = NULL;
item->type = type;
item->directory_ptr = directory_ptr;
+12 -1
View File
@@ -122,7 +122,18 @@ static void contentless_cores_init_info_entries(
char licenses_str[MENU_LABEL_MAX_LENGTH];
contentless_core_info_entry_t *entry =
(contentless_core_info_entry_t*)malloc(sizeof(*entry));
size_t _len = strlcpy(licenses_str,
size_t _len;
/* NULL-check entry: the field writes below (licenses_str
* strdup, runtime.* init, hashmap insert) NULL-deref on
* OOM. Void-returning function, void caller; skip this
* core's info entry and continue the enumeration loop -
* the core just won't appear in the contentless-cores
* list until next scan. */
if (!entry)
continue;
_len = strlcpy(licenses_str,
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_INFO_LICENSES),
sizeof(licenses_str) - 3);
licenses_str[ _len] = ':';
+41 -3
View File
@@ -188,7 +188,19 @@ static void ex_arena_grow(ex_arena *arena, size_t min_size)
{
size_t _len = EX_ARENA_ALIGN_UP(
MAX(min_size, EX_ARENA_BLOCK_SIZE), EX_ARENA_ALIGNMENT);
arena->ptr = (char *)malloc(_len);
char *new_block = (char *)malloc(_len);
/* NULL-check: on OOM leave arena->ptr and arena->end
* pointing at the current (exhausted) block if there is one,
* or both NULL on first grow. ex_arena_alloc returns the
* current arena->ptr as the caller's 'ptr' and the caller
* dereferences it, so we need ex_arena_alloc itself to
* signal the failure - that's handled by the 'end - ptr'
* pointer-subtraction which is defined to be 0 when both
* are NULL. The second check in ex_arena_alloc below
* catches the remaining OOM path. */
if (!new_block)
return;
arena->ptr = new_block;
arena->end = arena->ptr + _len;
RBUF_PUSH(arena->blocks, arena->ptr);
}
@@ -198,6 +210,12 @@ static void *ex_arena_alloc(ex_arena *arena, size_t len)
void *ptr = NULL;
if (len > (size_t)(arena->end - arena->ptr))
ex_arena_grow(arena, len);
/* Re-check after grow: on OOM the grow function leaves
* arena->ptr and arena->end unchanged, so the capacity
* check still fails. Return NULL so callers can bail
* rather than dereference stale or NULL storage. */
if (len > (size_t)(arena->end - arena->ptr))
return NULL;
ptr = arena->ptr;
arena->ptr = (char *)
EX_ARENA_ALIGN_UP((uintptr_t)(arena->ptr + len), EX_ARENA_ALIGNMENT);
@@ -357,6 +375,14 @@ static void explore_add_unique_string(
entry = (explore_string_t*)
ex_arena_alloc(&state->arena,
sizeof(explore_string_t) + _len);
/* NULL-check: ex_arena_alloc returns NULL on OOM now.
* On failure skip this entry - the surrounding loop
* iterates over chars in the input string splitting on
* separators, so one missed entry just means one
* category value doesn't get indexed this pass. Picked
* up on the next scan once memory is available. */
if (!entry)
continue;
memcpy(entry->str, str, _len);
entry->str[_len] = '\0';
RBUF_PUSH(state->by[cat], entry);
@@ -770,7 +796,14 @@ explore_state_t *menu_explore_build_list(const char *directory_playlist,
size_t _len = strlen(original_title) + 1;
e->original_title = (char*)
ex_arena_alloc(&state->arena, _len);
memcpy(e->original_title, original_title, _len);
/* NULL-check: arena alloc returns NULL on OOM. Skip
* the memcpy; e->original_title stays NULL (matches
* the 'e->original_title = NULL' initialisation a
* few lines above). Callers under
* EXPLORE_SHOW_ORIGINAL_TITLE are expected to gate
* reads of this field. */
if (e->original_title)
memcpy(e->original_title, original_title, _len);
}
#endif
@@ -782,7 +815,12 @@ explore_state_t *menu_explore_build_list(const char *directory_playlist,
_len = RBUF_SIZEOF(split_buf);
e->split = (explore_string_t **)
ex_arena_alloc(&state->arena, _len);
memcpy(e->split, split_buf, _len);
/* NULL-check: arena alloc returns NULL on OOM. Skip
* the memcpy; e->split stays NULL. Downstream
* iteration in the Explore menu checks 'e->split'
* before walking the pointer array. */
if (e->split)
memcpy(e->split, split_buf, _len);
RBUF_CLEAR(split_buf);
}