mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
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:
1 parent
4cf2d214ff
commit
1c7e114364
3 files changed
+61
-6
No files matched your search
@@ -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;
|
||||
|
||||
@@ -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
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user