Skip to content

Commit 4cf2d21

Browse files
committed
menu/cbs/menu_cbs_ok: NULL-check file_transfer_t allocs in network content and download paths
Three unchecked file_transfer_t allocations in menu_cbs_ok.c, all on user-triggered menu navigation paths. Same underlying pattern across all three: alloc -> immediate ->field write or strlcpy(-> path, ...) which NULL-derefs on OOM. === cb_net_generic parent-dir transfer (line ~5163) === file_transfer_t *transf = (file_transfer_t*)malloc(sizeof(*transf)); parent_dir_encoded[0] = '\0'; transf->enum_idx = MSG_UNKNOWN; /* NULL-deref on OOM */ _len = fill_pathname_parent_dir(transf->path, ...); ... Fires during 'index_dirs' navigation (online updater subdir browsing). Void-returning function, easy to degrade. Fix: wrap the transf-using block in 'if (transf) { ... }'. OOM skips the parent-dir HTTP probe but doesn't crash; the user sees the current listing without a 'go up a level' parent entry. They can retry by navigating back and re- entering the online updater menu. === generic_action_ok_network (line ~5268) === transf = (file_transfer_t*)calloc(1, sizeof(*transf)); strlcpy(transf->path, url_path, sizeof(transf->path)); /* NULL-deref */ task_push_http_transfer_file(url_path_encoded, ...); return generic_action_ok_displaylist_push(...); Fires on every 'fetch online core / content / thumbnail list' menu action. Function returns an int from generic_action_ok_displaylist_push which runs after the transfer queueing. Fix: wrap the HTTP transfer block in 'if (transf) { ... }' so the displaylist push still runs (showing an empty list until retry). Preserves the menu's navigation position and return-int contract. === action_ok_download_generic (line ~5606) === transf = (file_transfer_t*)calloc(1, sizeof(*transf)); transf->enum_idx = enum_idx; /* NULL-deref on OOM */ strlcpy(transf->path, path, sizeof(transf->path)); Fires on every user-initiated download (assets, cores, shader presets, thumbnails, overlays, etc.). Fix: 'if (!transf) return 0;' - same return code as the success path at the bottom of the function. Not using -1 because the menu dispatch treats negative returns as hard failures that may reset navigation state; 0 matches the normal return and leaves the user where they were to retry. === Not a bug, verified clean === Other alloc sites in menu/cbs/ audited during this pass: * menu_cbs_ok.c:5141 - menu->core_buf malloc, NULL-checked with goto finish. * menu_cbs_ok.c:6870 - room_data malloc, NULL-checked with goto done. * menu_cbs_ok.c:6884 - net_st->room_list calloc, NULL-checked with if (room_list) guard around the memcpy loop. * menu_cbs_ok.c:6944 - net_st->room_list calloc in discovered- hosts callback, NULL-checked with goto done. * The 9 cheat_manager_realloc() call sites between lines 4526-4830 are function calls into a previously-patched realloc contract (see '8fe1c05 cheat_manager: fix three OOM bug clusters...') not raw allocations. strdup-based sites checked but not patched (graceful degrade): * menu_cbs_deferred_push.c:84, 339, 342 - info->path_b / info->path / info->label reassignments. Callers free the old string first; a NULL replacement represents 'not set' which downstream displaylist consumers tolerate (menu iteration already handles missing path/label gracefully). * menu_cbs_deferred_push.c:464 - info->exts strdup feeds dir_list_append at menu_displaylist.c:4433 which gates on 'if (ext)' before parsing (see libretro-common/lists/ dir_list.c:326-331). NULL exts = no ext filter, still returns a valid listing. * menu_cbs_info.c:54 - info.label for the DISPLAYLIST_HELP screen. Same reasoning as the deferred_push cases; NULL label is treated as 'default help title' by downstream consumers. Leaving those strdup sites alone keeps the diff minimal and avoids introducing behavioural changes to paths that weren't actually crashing. === Thread-safety === All three fixed functions run on the main thread during menu interaction. HTTP transfer tasks queued via task_push_http_transfer_file run on the task worker thread but we only queue them after the transf payload is fully populated (the NULL-check guards skip queueing, which is what we want - an unqueued task is a no-op). No lock discipline changes. === Reachability === Every fix site is triggered by direct user action in the menu: * generic_action_ok_network + action_ok_download_generic fire on every content/core/thumbnail download attempt from the Online Updater submenu. * cb_net_generic parent-dir transfer fires as a follow-up when entering a subdirectory in the Online Updater.
1 parent 1871bd1 commit 4cf2d21

1 file changed

Lines changed: 38 additions & 14 deletions

File tree

menu/cbs/menu_cbs_ok.c

Lines changed: 38 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -5161,20 +5161,29 @@ static void cb_net_generic(retro_task_t *task,
51615161
size_t _len;
51625162
char parent_dir_encoded[DIR_MAX_LENGTH];
51635163
file_transfer_t *transf = (file_transfer_t*)malloc(sizeof(*transf));
5164-
parent_dir_encoded[0] = '\0';
5164+
/* NULL-check transf: the field writes below (->enum_idx,
5165+
* ->path via strlcpy through fill_pathname_parent_dir)
5166+
* NULL-deref on OOM. On failure skip the parent-dir
5167+
* HTTP transfer entirely - the 'index_dirs' feature
5168+
* degrades gracefully (no parent-dir nav up from the
5169+
* current listing) rather than crashing. */
5170+
if (transf)
5171+
{
5172+
parent_dir_encoded[0] = '\0';
51655173

5166-
transf->enum_idx = MSG_UNKNOWN;
5174+
transf->enum_idx = MSG_UNKNOWN;
51675175

5168-
_len = fill_pathname_parent_dir(transf->path,
5169-
state->path, sizeof(transf->path));
5170-
strlcpy(transf->path + _len,
5171-
FILE_PATH_INDEX_DIRS_URL,
5172-
sizeof(transf->path) - _len);
5176+
_len = fill_pathname_parent_dir(transf->path,
5177+
state->path, sizeof(transf->path));
5178+
strlcpy(transf->path + _len,
5179+
FILE_PATH_INDEX_DIRS_URL,
5180+
sizeof(transf->path) - _len);
51735181

5174-
net_http_urlencode_full(parent_dir_encoded, transf->path,
5175-
sizeof(parent_dir_encoded));
5176-
task_push_http_transfer_file(parent_dir_encoded, true,
5177-
"index_dirs", cb_net_generic_subdir, transf);
5182+
net_http_urlencode_full(parent_dir_encoded, transf->path,
5183+
sizeof(parent_dir_encoded));
5184+
task_push_http_transfer_file(parent_dir_encoded, true,
5185+
"index_dirs", cb_net_generic_subdir, transf);
5186+
}
51785187
}
51795188

51805189
if (state)
@@ -5257,10 +5266,19 @@ static int generic_action_ok_network(const char *path,
52575266
generic_action_ok_command(CMD_EVENT_NETWORK_INIT);
52585267

52595268
transf = (file_transfer_t*)calloc(1, sizeof(*transf));
5260-
strlcpy(transf->path, url_path, sizeof(transf->path));
5269+
/* NULL-check transf: strlcpy(transf->path, ...) on the next
5270+
* line NULL-derefs on OOM. Skip the HTTP transfer entirely -
5271+
* the displaylist push below still runs but will show an
5272+
* empty content list until the user retries. Degrading to
5273+
* 'no network content available this session' is strictly
5274+
* better than crashing mid-menu-navigation. */
5275+
if (transf)
5276+
{
5277+
strlcpy(transf->path, url_path, sizeof(transf->path));
52615278

5262-
net_http_urlencode_full(url_path_encoded, url_path, sizeof(url_path_encoded));
5263-
task_push_http_transfer_file(url_path_encoded, suppress_msg, url_label, callback, transf);
5279+
net_http_urlencode_full(url_path_encoded, url_path, sizeof(url_path_encoded));
5280+
task_push_http_transfer_file(url_path_encoded, suppress_msg, url_label, callback, transf);
5281+
}
52645282

52655283
return generic_action_ok_displaylist_push(
52665284
path, NULL,
@@ -5586,6 +5604,12 @@ static int action_ok_download_generic(const char *path,
55865604
fill_pathname_join_special(s2, s, path, sizeof(s2));
55875605

55885606
transf = (file_transfer_t*)calloc(1, sizeof(*transf));
5607+
/* NULL-check transf: the ->enum_idx write and strlcpy(transf->
5608+
* path, ...) below NULL-deref on OOM. Return 0 (same as the
5609+
* success path below) to leave the menu state unchanged; the
5610+
* user can re-trigger the download once memory is available. */
5611+
if (!transf)
5612+
return 0;
55895613
transf->enum_idx = enum_idx;
55905614
strlcpy(transf->path, path, sizeof(transf->path));
55915615

0 commit comments

Comments
 (0)