Skip to content

Commit aeaaa23

Browse files
committed
tasks/database: null-check malloc and plug leak in manual-scan rdb path setup
task_manual_content_scan_handler has a one-time setup block that builds the path to the selected .rdb file when db_selection == MANUAL_CONTENT_SCAN_SELECT_DB_SPECIFIC. It heap-allocates two PATH_MAX_LENGTH buffers: char* rdb_name = (char*)malloc(str_len); char* rdb_fullpath = (char*)malloc(str_len); and then immediately passes them to fill_pathname / fill_pathname_join_special. Neither of those helpers guards against a NULL destination (both call strlcpy(s, ...) on their first argument with no check), so if either malloc fails the subsequent call segfaults. Additionally, the adjacent goto task_finished triggered by 'string_list_new failure' walked out of the block without freeing rdb_name / rdb_fullpath, leaking ~8 KiB on that OOM path. Three-part fix: * After the two mallocs, check both and take the task_finished exit if either failed, freeing whichever one did succeed (free(NULL) is defined as a no-op, so no conditional needed on the companion buffer). * On the pre-existing 'string_list_new failed' exit, free the two buffers before taking task_finished. * Drop the redundant 'if (rdb_name) free(rdb_name);' / 'if (rdb_fullpath) free(rdb_fullpath);' on the success path; the NULL-check is already implied by free's own semantics, so the conditionals are noise that misleads readers into thinking the pointers might legitimately be NULL by this point. Scope: this only runs once per manual content scan (it's inside the initialisation branch that executes when dbstate->list is still NULL), so the fix is correctness-only, not performance. Thread-safety: unchanged. Task handler runs on the task worker thread; the mallocs, buffers, and dbstate are all local-or-owned by this task. No shared state touched. Separately: a longer-standing TODO at line 901 in database_info_list_iterate_found_match flags two per-match PATH_MAX_LENGTH heap allocations as 'needlessly large'. That one's more complicated because db_crc there can legitimately grow to the full PATH_MAX (carries db_state->serial which is declared as char[4096]), so shrinking it would need auditing all serial sources for their real max length. Deferred.
1 parent 741ead4 commit aeaaa23

1 file changed

Lines changed: 24 additions & 5 deletions

File tree

tasks/task_database.c

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1919,6 +1919,21 @@ static void task_manual_content_scan_handler(retro_task_t *task)
19191919
union string_list_elem_attr attr;
19201920
attr.i = 0;
19211921

1922+
/* Bail out if either heap allocation failed.
1923+
* fill_pathname and fill_pathname_join_special
1924+
* both call strlcpy on their destination buffer
1925+
* with no NULL guard, so proceeding with a NULL
1926+
* rdb_name / rdb_fullpath would segfault. Free
1927+
* the one that did succeed (free(NULL) is a
1928+
* no-op so no conditional needed) before taking
1929+
* the task-finished exit. */
1930+
if (!rdb_name || !rdb_fullpath)
1931+
{
1932+
free(rdb_name);
1933+
free(rdb_fullpath);
1934+
goto task_finished;
1935+
}
1936+
19221937
fill_pathname(rdb_name,
19231938
manual_scan->task_config->database_name,
19241939
".rdb", str_len);
@@ -1929,13 +1944,17 @@ static void task_manual_content_scan_handler(retro_task_t *task)
19291944

19301945
dbstate->list = string_list_new();
19311946
if (!dbstate->list)
1932-
goto task_finished;
1933-
string_list_append(dbstate->list, rdb_fullpath, attr);
1934-
if (rdb_name)
1947+
{
1948+
/* Earlier code goto'd here without freeing
1949+
* the two buffers above, leaking ~8 KiB on
1950+
* this OOM path. Free them explicitly. */
19351951
free(rdb_name);
1936-
if (rdb_fullpath)
19371952
free(rdb_fullpath);
1938-
1953+
goto task_finished;
1954+
}
1955+
string_list_append(dbstate->list, rdb_fullpath, attr);
1956+
free(rdb_name);
1957+
free(rdb_fullpath);
19391958
}
19401959
else
19411960
{

0 commit comments

Comments
 (0)