Skip to content

Commit 741ead4

Browse files
committed
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.
1 parent e976983 commit 741ead4

2 files changed

Lines changed: 27 additions & 6 deletions

File tree

menu/drivers/ozone.c

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3892,12 +3892,21 @@ static void ozone_update_savestate_thumbnail_path(void *data, unsigned i)
38923892
{
38933893
settings_t *settings = config_get_ptr();
38943894
ozone_handle_t *ozone = (ozone_handle_t*)data;
3895-
bool savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
3896-
const char *current_path = strdup(ozone->savestate_thumbnail_file_path);
3895+
bool savestate_thumbnail;
3896+
/* Stack-local snapshot of the current path; see materialui (93449d3)
3897+
* and xmb equivalents for rationale. Fixes three stacked problems:
3898+
* null-deref on !data via the strdup-before-guard ordering, leak of
3899+
* a never-freed heap string assigned to const char *, and needless
3900+
* heap allocation for a value that lives in a fixed-size
3901+
* char[PATH_MAX_LENGTH] ivar. */
3902+
char old_path[PATH_MAX_LENGTH];
38973903

38983904
if (!ozone)
38993905
return;
39003906

3907+
savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
3908+
strlcpy(old_path, ozone->savestate_thumbnail_file_path, sizeof(old_path));
3909+
39013910
if (ozone->flags2 & OZONE_FLAG2_SELECTION_CORE_IS_VIEWER_REAL)
39023911
ozone->flags2 |= OZONE_FLAG2_SELECTION_CORE_IS_VIEWER;
39033912
else
@@ -3950,7 +3959,7 @@ static void ozone_update_savestate_thumbnail_path(void *data, unsigned i)
39503959
strlcpy(ozone->savestate_thumbnail_file_path, path,
39513960
sizeof(ozone->savestate_thumbnail_file_path));
39523961

3953-
if (!string_is_equal(current_path,
3962+
if (!string_is_equal(old_path,
39543963
ozone->savestate_thumbnail_file_path))
39553964
gfx_thumbnail_reset(&ozone->thumbnails.savestate);
39563965

menu/drivers/xmb.c

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1305,12 +1305,24 @@ static void xmb_update_savestate_thumbnail_path(void *data, unsigned i)
13051305
{
13061306
xmb_handle_t *xmb = (xmb_handle_t*)data;
13071307
settings_t *settings = config_get_ptr();
1308-
bool savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
1309-
const char *current_path = strdup(xmb->savestate_thumbnail_file_path);
1308+
bool savestate_thumbnail;
1309+
/* Snapshot the current path on the stack before we clear it, so we
1310+
* can compare against the new path at the end to decide whether to
1311+
* reset the thumbnail cache. Previously this was a heap strdup
1312+
* assigned to a const char * that was never freed (leak on every
1313+
* selection change in the state-slot menu), performed BEFORE the
1314+
* !xmb NULL check below (null-deref if data was ever NULL), and
1315+
* used heap allocation for a value that lives in a fixed-size
1316+
* char[PATH_MAX_LENGTH] ivar. Same fix pattern as the materialui
1317+
* equivalent (93449d3): stack buffer, after the NULL guard. */
1318+
char old_path[PATH_MAX_LENGTH];
13101319

13111320
if (!xmb)
13121321
return;
13131322

1323+
savestate_thumbnail = settings->bools.savestate_thumbnail_enable;
1324+
strlcpy(old_path, xmb->savestate_thumbnail_file_path, sizeof(old_path));
1325+
13141326
if (xmb->skip_thumbnail_reset)
13151327
return;
13161328

@@ -1356,7 +1368,7 @@ static void xmb_update_savestate_thumbnail_path(void *data, unsigned i)
13561368
strlcpy(xmb->savestate_thumbnail_file_path, path,
13571369
sizeof(xmb->savestate_thumbnail_file_path));
13581370

1359-
if (!string_is_equal(current_path, xmb->savestate_thumbnail_file_path))
1371+
if (!string_is_equal(old_path, xmb->savestate_thumbnail_file_path))
13601372
gfx_thumbnail_reset(&xmb->thumbnails.savestate);
13611373

13621374
xmb->fullscreen_thumbnails_available = true;

0 commit comments

Comments
 (0)