mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-09 03:29:43 -10:00
menu/xmb, menu/ozone: fix leak + null-deref + needless strdup in update_savestate_thumbnail_path
Both xmb and ozone have an identical three-bug stack in their
update_savestate_thumbnail_path implementations, matching the pattern
already fixed for materialui in commit 93449d3:
1. Null-deref: xmb->savestate_thumbnail_file_path (xmb) /
ozone->savestate_thumbnail_file_path (ozone) is dereferenced by
strdup BEFORE the 'if (!xmb) return;' / 'if (!ozone) return;'
guard further down. If data was ever NULL, the guard would
never be reached.
2. Leak: the strdup result is assigned to a const char *
(current_path) and never freed on any exit path - not when
savestate_thumbnail is disabled, not when the entry label check
fails, not on the success path either. Called on every selection
change in the state-slot menu, so the leak is quickly user-visible
with typical navigation.
3. Needless strdup: savestate_thumbnail_file_path is a fixed-size
char[PATH_MAX_LENGTH] embedded in the struct, not a pointer.
Heap allocation is unnecessary; a stack buffer of the same size
does the job, sidesteps both the leak and the OOM failure mode
(where strdup returning NULL silently made string_is_equal
compare against NULL - undefined behaviour).
Fix both: declare 'char old_path[PATH_MAX_LENGTH]' AFTER the NULL
guard, strlcpy the current path into it before the field is cleared,
and compare against it further down. No heap allocation, no leak,
no dereference before NULL check.
Thread-safety: unchanged. Menu driver callbacks run on the main
thread; xmb_handle_t / ozone_handle_t state is not shared across
threads. The only writes touch the driver-specific savestate
thumbnail path and thumbnails.savestate state, both already single-
thread owned.
With this commit the three menu drivers that implement savestate
thumbnails (materialui, xmb, ozone) all use the same corrected
pattern.
This commit is contained in:
1 parent
e9769838be
commit
741ead4374
2 files changed
+27
-6
No files matched your search
+12
-3
@@ -3892,12 +3892,21 @@ static void ozone_update_savestate_thumbnail_path(void *data, unsigned i)
|
||||
{
|
||||
settings_t *settings = config_get_ptr();
|
||||
ozone_handle_t *ozone = (ozone_handle_t*)data;
|
||||
bool savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
|
||||
const char *current_path = strdup(ozone->savestate_thumbnail_file_path);
|
||||
bool savestate_thumbnail;
|
||||
/* Stack-local snapshot of the current path; see materialui (93449d3)
|
||||
* and xmb equivalents for rationale. Fixes three stacked problems:
|
||||
* null-deref on !data via the strdup-before-guard ordering, leak of
|
||||
* a never-freed heap string assigned to const char *, and needless
|
||||
* heap allocation for a value that lives in a fixed-size
|
||||
* char[PATH_MAX_LENGTH] ivar. */
|
||||
char old_path[PATH_MAX_LENGTH];
|
||||
|
||||
if (!ozone)
|
||||
return;
|
||||
|
||||
savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
|
||||
strlcpy(old_path, ozone->savestate_thumbnail_file_path, sizeof(old_path));
|
||||
|
||||
if (ozone->flags2 & OZONE_FLAG2_SELECTION_CORE_IS_VIEWER_REAL)
|
||||
ozone->flags2 |= OZONE_FLAG2_SELECTION_CORE_IS_VIEWER;
|
||||
else
|
||||
@@ -3950,7 +3959,7 @@ static void ozone_update_savestate_thumbnail_path(void *data, unsigned i)
|
||||
strlcpy(ozone->savestate_thumbnail_file_path, path,
|
||||
sizeof(ozone->savestate_thumbnail_file_path));
|
||||
|
||||
if (!string_is_equal(current_path,
|
||||
if (!string_is_equal(old_path,
|
||||
ozone->savestate_thumbnail_file_path))
|
||||
gfx_thumbnail_reset(&ozone->thumbnails.savestate);
|
||||
|
||||
|
||||
+15
-3
@@ -1305,12 +1305,24 @@ static void xmb_update_savestate_thumbnail_path(void *data, unsigned i)
|
||||
{
|
||||
xmb_handle_t *xmb = (xmb_handle_t*)data;
|
||||
settings_t *settings = config_get_ptr();
|
||||
bool savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
|
||||
const char *current_path = strdup(xmb->savestate_thumbnail_file_path);
|
||||
bool savestate_thumbnail;
|
||||
/* Snapshot the current path on the stack before we clear it, so we
|
||||
* can compare against the new path at the end to decide whether to
|
||||
* reset the thumbnail cache. Previously this was a heap strdup
|
||||
* assigned to a const char * that was never freed (leak on every
|
||||
* selection change in the state-slot menu), performed BEFORE the
|
||||
* !xmb NULL check below (null-deref if data was ever NULL), and
|
||||
* used heap allocation for a value that lives in a fixed-size
|
||||
* char[PATH_MAX_LENGTH] ivar. Same fix pattern as the materialui
|
||||
* equivalent (93449d3): stack buffer, after the NULL guard. */
|
||||
char old_path[PATH_MAX_LENGTH];
|
||||
|
||||
if (!xmb)
|
||||
return;
|
||||
|
||||
savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
|
||||
strlcpy(old_path, xmb->savestate_thumbnail_file_path, sizeof(old_path));
|
||||
|
||||
if (xmb->skip_thumbnail_reset)
|
||||
return;
|
||||
|
||||
@@ -1356,7 +1368,7 @@ static void xmb_update_savestate_thumbnail_path(void *data, unsigned i)
|
||||
strlcpy(xmb->savestate_thumbnail_file_path, path,
|
||||
sizeof(xmb->savestate_thumbnail_file_path));
|
||||
|
||||
if (!string_is_equal(current_path, xmb->savestate_thumbnail_file_path))
|
||||
if (!string_is_equal(old_path, xmb->savestate_thumbnail_file_path))
|
||||
gfx_thumbnail_reset(&xmb->thumbnails.savestate);
|
||||
|
||||
xmb->fullscreen_thumbnails_available = true;
|
||||
|
||||
Reference in new issue
Block a user