Skip to content

Commit 1c7e114

Browse files
committed
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.
1 parent 4cf2d21 commit 1c7e114

3 files changed

Lines changed: 61 additions & 6 deletions

File tree

libretro-common/lists/file_list.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,14 @@ static INLINE void init_item_file(struct item_file *item,
7979
const char *path, const char *label, unsigned type,
8080
size_t directory_ptr, size_t entry_idx)
8181
{
82-
item->path = strdup(path);
83-
item->label = strdup(label);
82+
/* NULL-gate both strdup calls: strdup(NULL) is undefined
83+
* behaviour (glibc crashes). The sibling file_list_append
84+
* uses the same gating pattern. Callers have been seen to
85+
* pass NULL path here via menu_entries_prepend when
86+
* msg_hash_to_str returns NULL for an enum that no active
87+
* language handler recognises. */
88+
item->path = path ? strdup(path) : NULL;
89+
item->label = label ? strdup(label) : NULL;
8490
item->alt = NULL;
8591
item->type = type;
8692
item->directory_ptr = directory_ptr;

menu/menu_contentless_cores.c

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,18 @@ static void contentless_cores_init_info_entries(
122122
char licenses_str[MENU_LABEL_MAX_LENGTH];
123123
contentless_core_info_entry_t *entry =
124124
(contentless_core_info_entry_t*)malloc(sizeof(*entry));
125-
size_t _len = strlcpy(licenses_str,
125+
size_t _len;
126+
127+
/* NULL-check entry: the field writes below (licenses_str
128+
* strdup, runtime.* init, hashmap insert) NULL-deref on
129+
* OOM. Void-returning function, void caller; skip this
130+
* core's info entry and continue the enumeration loop -
131+
* the core just won't appear in the contentless-cores
132+
* list until next scan. */
133+
if (!entry)
134+
continue;
135+
136+
_len = strlcpy(licenses_str,
126137
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_INFO_LICENSES),
127138
sizeof(licenses_str) - 3);
128139
licenses_str[ _len] = ':';

menu/menu_explore.c

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,19 @@ static void ex_arena_grow(ex_arena *arena, size_t min_size)
188188
{
189189
size_t _len = EX_ARENA_ALIGN_UP(
190190
MAX(min_size, EX_ARENA_BLOCK_SIZE), EX_ARENA_ALIGNMENT);
191-
arena->ptr = (char *)malloc(_len);
191+
char *new_block = (char *)malloc(_len);
192+
/* NULL-check: on OOM leave arena->ptr and arena->end
193+
* pointing at the current (exhausted) block if there is one,
194+
* or both NULL on first grow. ex_arena_alloc returns the
195+
* current arena->ptr as the caller's 'ptr' and the caller
196+
* dereferences it, so we need ex_arena_alloc itself to
197+
* signal the failure - that's handled by the 'end - ptr'
198+
* pointer-subtraction which is defined to be 0 when both
199+
* are NULL. The second check in ex_arena_alloc below
200+
* catches the remaining OOM path. */
201+
if (!new_block)
202+
return;
203+
arena->ptr = new_block;
192204
arena->end = arena->ptr + _len;
193205
RBUF_PUSH(arena->blocks, arena->ptr);
194206
}
@@ -198,6 +210,12 @@ static void *ex_arena_alloc(ex_arena *arena, size_t len)
198210
void *ptr = NULL;
199211
if (len > (size_t)(arena->end - arena->ptr))
200212
ex_arena_grow(arena, len);
213+
/* Re-check after grow: on OOM the grow function leaves
214+
* arena->ptr and arena->end unchanged, so the capacity
215+
* check still fails. Return NULL so callers can bail
216+
* rather than dereference stale or NULL storage. */
217+
if (len > (size_t)(arena->end - arena->ptr))
218+
return NULL;
201219
ptr = arena->ptr;
202220
arena->ptr = (char *)
203221
EX_ARENA_ALIGN_UP((uintptr_t)(arena->ptr + len), EX_ARENA_ALIGNMENT);
@@ -357,6 +375,14 @@ static void explore_add_unique_string(
357375
entry = (explore_string_t*)
358376
ex_arena_alloc(&state->arena,
359377
sizeof(explore_string_t) + _len);
378+
/* NULL-check: ex_arena_alloc returns NULL on OOM now.
379+
* On failure skip this entry - the surrounding loop
380+
* iterates over chars in the input string splitting on
381+
* separators, so one missed entry just means one
382+
* category value doesn't get indexed this pass. Picked
383+
* up on the next scan once memory is available. */
384+
if (!entry)
385+
continue;
360386
memcpy(entry->str, str, _len);
361387
entry->str[_len] = '\0';
362388
RBUF_PUSH(state->by[cat], entry);
@@ -770,7 +796,14 @@ explore_state_t *menu_explore_build_list(const char *directory_playlist,
770796
size_t _len = strlen(original_title) + 1;
771797
e->original_title = (char*)
772798
ex_arena_alloc(&state->arena, _len);
773-
memcpy(e->original_title, original_title, _len);
799+
/* NULL-check: arena alloc returns NULL on OOM. Skip
800+
* the memcpy; e->original_title stays NULL (matches
801+
* the 'e->original_title = NULL' initialisation a
802+
* few lines above). Callers under
803+
* EXPLORE_SHOW_ORIGINAL_TITLE are expected to gate
804+
* reads of this field. */
805+
if (e->original_title)
806+
memcpy(e->original_title, original_title, _len);
774807
}
775808
#endif
776809

@@ -782,7 +815,12 @@ explore_state_t *menu_explore_build_list(const char *directory_playlist,
782815
_len = RBUF_SIZEOF(split_buf);
783816
e->split = (explore_string_t **)
784817
ex_arena_alloc(&state->arena, _len);
785-
memcpy(e->split, split_buf, _len);
818+
/* NULL-check: arena alloc returns NULL on OOM. Skip
819+
* the memcpy; e->split stays NULL. Downstream
820+
* iteration in the Explore menu checks 'e->split'
821+
* before walking the pointer array. */
822+
if (e->split)
823+
memcpy(e->split, split_buf, _len);
786824
RBUF_CLEAR(split_buf);
787825
}
788826

0 commit comments

Comments
 (0)