Skip to content

Commit a0d08c1

Browse files
committed
gfx/gfx_thumbnail: fix four heap-safety issues found during audit pass
Four independent issues found during a focused audit of gfx/ gfx_thumbnail.c, the subsystem touched by all four menu drivers (xmb, ozone, materialui, rgui). Same audit class as the recent xmb (9a27e66), ozone (d56c728), and materialui (d5e598a) commits. * gfx_thumbnail_get_path: missing break on LEFT case (gfx/gfx_thumbnail.c) The switch in gfx_thumbnail_get_path() that selects which path to return based on thumbnail_id is missing a `break` after the GFX_THUMBNAIL_LEFT case. When a caller asks for the LEFT thumbnail and *path_data->left_path is empty, control falls through to GFX_THUMBNAIL_ICON; if *path_data->icon_path is populated, the function returns the icon path *as if it were the left thumbnail*. The caller then loads the icon as the left thumbnail texture. Wrong-content substitution rather than memory corruption, but silent and reachable on any menu where left thumbnails are unset but icons are configured (the default for most playlist configurations on xmb/ozone/materialui). Add the missing break. * gfx_thumbnail_get_content_dir: stack buffer overflow on long directory paths (gfx/gfx_thumbnail.c) The function extracts the basename of the directory portion of path_data->content_path by copying that prefix into a local tmp_buf[NAME_MAX_LENGTH] and then calling path_basename_nocompression() on the copy. The strlcpy size passed is the directory-portion length _len, which is bounded only by `_len < PATH_MAX_LENGTH` (i.e. up to 2047) -- but tmp_buf is only NAME_MAX_LENGTH (256) bytes. Whenever the directory portion exceeds 255 chars, strlcpy writes up to _len-1 bytes into the smaller tmp_buf and corrupts the stack past it. Reachable on systems with deep folder hierarchies; content_path is set from runtime_content_path_basename and other content paths capped at PATH_MAX_LENGTH, so 500+ char directory prefixes are entirely plausible (a savefile path nested under the user's home directory plus a deep ROM organisation scheme). Fix: anchor the copy at the latest position that still lets the directory's last segment fit in tmp_buf, then take the basename from there. We never needed the full prefix in tmp_buf -- only its tail -- so dropping the leading bytes when _len > sizeof(tmp_buf) gives the same answer without the overflow. * gfx_savestate_thumbnail_get_path: _len underflow in path build (gfx/gfx_thumbnail.c) Same unsafe pattern that the recent strlcpy_append sweep (78c52ab / 25ade82 / e446242 / d5e598a) was created to replace, applied here to a mixed strlcpy + snprintf chain: _len = strlcpy(s, state_name, len); if (state_slot > 0) _len += snprintf(s + _len, len - _len, "%d", state_slot); ... strlcpy(s + _len, FILE_PATH_PNG_EXTENSION, len - _len); Both strlcpy and snprintf return their would-be length on truncation, so when state_name approaches @len, _len overshoots @len, the next `len - _len` subtraction underflows size_t to ~SIZE_MAX, and subsequent calls treat the destination as essentially infinite. Reachable: state_name is runloop_st->name.savestate which is sized PATH_MAX_LENGTH; on a deep ROM directory the savestate name itself can fill the buffer, and the slot suffix or .png extension push the running total past @len. Add explicit truncation guards after each step so the chain stays inside the buffer. Also add the missing `!s || !len` guard at the top -- the historical s[0] = '\0' before the NULL check was a latent NULL-deref. * gfx_thumbnail_path_init: malloc -> calloc to zero playlist_index (gfx/gfx_thumbnail.c) gfx_thumbnail_path_init() allocates the path_data struct via malloc and then calls gfx_thumbnail_path_reset(), which resets the string buffers and the three playlist_*_mode fields but leaves playlist_index untouched -- garbage from malloc until the first set_content_*() call writes it. Read sites in xmb (xmb_set_title at xmb.c:2566 and the related fallback at xmb.c:7462) sample thumbnail_path_data->playlist_index before any setter has necessarily run, then pass it to playlist_get_index(). The callee bounds-checks against the playlist's size, so a garbage index just produces a stale pl_entry rather than a crash, but the code is latently UB. Switch to calloc. Cheaper than touching path_reset's API and forecloses the issue for any future read site that doesn't replicate the bounds-check defence.
1 parent d5e598a commit a0d08c1

1 file changed

Lines changed: 67 additions & 4 deletions

File tree

gfx/gfx_thumbnail.c

Lines changed: 67 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,7 @@ static bool gfx_thumbnail_get_path(
300300
*path = path_data->left_path;
301301
return true;
302302
}
303+
break;
303304
case GFX_THUMBNAIL_ICON:
304305
if (*path_data->icon_path)
305306
{
@@ -1316,8 +1317,20 @@ void gfx_thumbnail_path_reset(gfx_thumbnail_path_data_t *path_data)
13161317
* Note: Returned object must be free()d */
13171318
gfx_thumbnail_path_data_t *gfx_thumbnail_path_init(void)
13181319
{
1320+
/* Use calloc rather than malloc. gfx_thumbnail_path_reset()
1321+
* resets the string buffers and the playlist_*_mode fields
1322+
* but does NOT touch playlist_index, leaving it as garbage
1323+
* from malloc until the first set_content_*() call. Read
1324+
* sites in xmb (xmb_set_title) sample
1325+
* thumbnail_path_data->playlist_index before any setter has
1326+
* necessarily run, then pass it to playlist_get_index() --
1327+
* which is bounds-checked, so a garbage index just returns
1328+
* a stale pl_entry rather than crashing, but the code is
1329+
* latently UB and a future read site without the same defence
1330+
* would inherit a real bug. Zero-init via calloc closes
1331+
* the window without churning path_reset's API. */
13191332
gfx_thumbnail_path_data_t *path_data = (gfx_thumbnail_path_data_t*)
1320-
malloc(sizeof(*path_data));
1333+
calloc(1, sizeof(*path_data));
13211334
if (!path_data)
13221335
return NULL;
13231336

@@ -1915,14 +1928,32 @@ size_t gfx_thumbnail_get_content_dir(gfx_thumbnail_path_data_t *path_data,
19151928
size_t _len;
19161929
char *last_slash;
19171930
char tmp_buf[NAME_MAX_LENGTH];
1931+
const char *dir_start;
19181932
if (!path_data || !*path_data->content_path)
19191933
return 0;
19201934
if (!(last_slash = find_last_slash(path_data->content_path)))
19211935
return 0;
19221936
_len = last_slash + 1 - path_data->content_path;
19231937
if (!((_len > 1) && (_len < PATH_MAX_LENGTH)))
19241938
return 0;
1925-
strlcpy(tmp_buf, path_data->content_path, _len * sizeof(char));
1939+
/* The historical implementation copied the whole directory
1940+
* portion of content_path into tmp_buf and then took its
1941+
* basename. But content_path is sized PATH_MAX_LENGTH (2048)
1942+
* and tmp_buf only NAME_MAX_LENGTH (256), so a directory
1943+
* portion longer than 255 chars (reachable on systems with
1944+
* deep folder hierarchies) caused strlcpy to write up to
1945+
* _len-1 bytes into the 256-byte tmp_buf -- a stack overflow.
1946+
*
1947+
* Since the goal is the *basename* of the directory (i.e.
1948+
* the segment between the second-to-last and last slashes),
1949+
* skip the prefix copy: we only need the tail. Anchor the
1950+
* copy at the latest position that still lets the segment
1951+
* fit in tmp_buf, then take its basename. */
1952+
dir_start = path_data->content_path;
1953+
if (_len > sizeof(tmp_buf))
1954+
dir_start = last_slash + 1 - sizeof(tmp_buf);
1955+
strlcpy(tmp_buf, dir_start,
1956+
(last_slash - dir_start + 1) * sizeof(char));
19261957
return strlcpy(s, path_basename_nocompression(tmp_buf), len);
19271958
}
19281959

@@ -1934,17 +1965,49 @@ void gfx_savestate_thumbnail_get_path(
19341965
{
19351966
size_t _len;
19361967

1968+
if (!s || !len)
1969+
return;
1970+
19371971
s[0] = '\0';
19381972

19391973
if (!state_name || !*state_name)
19401974
return;
19411975

19421976
_len = strlcpy(s, state_name, len);
19431977

1978+
/* The historical implementation accumulated _len from
1979+
* strlcpy / snprintf returns and used `len - _len` as the
1980+
* size for subsequent calls. That pattern is NOT
1981+
* self-bounding: strlcpy returns strlen(@src), and snprintf
1982+
* returns the would-be length on truncation, so on any
1983+
* truncating call _len overshoots @len, the next len-_len
1984+
* subtraction underflows size_t to ~SIZE_MAX, and the
1985+
* subsequent strlcpy treats the destination as essentially
1986+
* infinite. Reachable when state_name approaches PATH_MAX_LENGTH
1987+
* (e.g. content loaded from a deep directory tree, since
1988+
* runloop_st->name.savestate is sized PATH_MAX_LENGTH) and
1989+
* the slot suffix or extension push the total past @len.
1990+
*
1991+
* Clamp _len after each truncation so the chain stays inside
1992+
* the buffer instead of running off the end. */
1993+
if (_len >= len)
1994+
_len = len - 1;
1995+
19441996
if (state_slot > 0)
1945-
_len += snprintf(s + _len, len - _len, "%d", state_slot);
1997+
{
1998+
int n = snprintf(s + _len, len - _len, "%d", state_slot);
1999+
if (n < 0)
2000+
return;
2001+
_len += (size_t)n;
2002+
if (_len >= len)
2003+
_len = len - 1;
2004+
}
19462005
else if (state_slot < 0)
1947-
_len = fill_pathname_join_delim(s, state_name, "auto", '.', len);
2006+
{
2007+
_len = fill_pathname_join_delim(s, state_name, "auto", '.', len);
2008+
if (_len >= len)
2009+
_len = len - 1;
2010+
}
19482011

19492012
strlcpy(s + _len, FILE_PATH_PNG_EXTENSION, len - _len);
19502013
}

0 commit comments

Comments
 (0)