Follow-up to 9a20309/f7790dc: the runtime probe unlocked SAVING on cores
with stale metadata, but every LOAD path was still gated on
save-capability, which measures the wrong thing twice over:
- content_load_state() (the funnel all load paths go through) refused
with 'Core does not support save states' based purely on metadata;
- command_load_state_slot() additionally re-checked
core_serialize_size() > 0 before loading - so even a core with
CORRECT metadata could not load a state wherever it cannot currently
serialize. Cores like boom3 legitimately report size 0 at the game's
own main menu, which is exactly where users load a state to resume a
session, while retro_unserialize() performs a full restore from there
without issue.
Principle: for loading, the artifact outranks the metadata. A state
file the user produced (or a populated undo buffer) is sufficient
grounds to attempt the load; retro_unserialize() is the final arbiter
and fails gracefully. Concretely:
- content_load_state(): allow when the state file exists (or when
loading the backup buffer), regardless of metadata;
- content_undo_load_state(): allow when the undo buffer is populated;
- command_load_state_slot(): replace the core_serialize_size() > 0
recheck with path validity;
- command_event_load_entry_state() / command_event_load_auto_state():
drop the early metadata gates; the funnel decides.
Save paths are untouched: they keep the metadata gate with the runtime
probe, which measures exactly the right thing for saving.
Issue #18651 reports that with "Increment Save State Index
Automatically" + "Maximum Auto-Increment Save States to Keep", the
slot index advances but no file is saved in the new slot, the index
sometimes stops advancing partway through the range, and loads then
fail because the slot points at an empty file. This is not reliably
reproducible without device-specific (Android) save-write behavior, so
add targeted logging to pin down which of the linked defects fires,
without changing any behavior:
- command_event() CMD_EVENT_SAVE_STATE: log the auto-index slot
advance (old -> new) that happens *before* the save is attempted.
- command_event_main_state(): capture content_save_state()'s
previously-discarded return value and log it, noting that the actual
disk write is asynchronous so a "queued" save is not yet a
successful one.
- task_save_handler(): log the three terminal outcomes with the slot
and path - the silent file-open failure (prime suspect on Android
scoped storage, where the slot was already advanced), the mid-write
failure, and successful completion. This is what shows whether the
slot the frontend advanced to actually received a file.
- command_event_set_savestate_auto_index(): the on-load scan already
logged the found slot; also log the previous slot so a destructive
reset (slot snapping back to the on-disk max index) is visible.
With these, a user log will show the exact sequence: slot advanced to
N, save task for slot N failed to open / failed to write / completed,
and on reload the slot reset from N back to a lower on-disk index.
That distinguishes "advanced-but-write-failed" from
"scan-reset-the-index" and lets the underlying fix target the right
defect (tying the slot advance to async save-task completion, and
making the on-load scan non-destructive on gaps).
Replaces the per-event reset toggles
frame_time_counter_reset_after_fastforwarding
frame_time_counter_reset_after_load_state
frame_time_counter_reset_after_save_state
with a single combined bool
frame_time_counter_auto_reset
Rationale
=========
The three toggles all do the same thing -- drain the
'Estimated Screen Refresh Rate' sample buffer when an event
known to introduce contaminated samples completes -- on three
separate events. Splitting them was historical (each added at
the time a particular contaminator was identified) rather than
structurally motivated. As a UI, three identically-described
toggles next to each other is harder to reason about than a
single 'do best-effort cleanup' switch.
This becomes more relevant after the previous gated-sampling
commit: with 'Sample Frame Time Only In Stable State' on, the
buffer never accepts contaminated samples in the first place,
making the reset toggles entirely redundant. Folding them into
one bool and hiding it under that condition keeps the submenu
focused on the user's actual mental model -- either prevent
contamination or clean it up afterwards.
Behaviour
=========
* Default: false (matches the prior aggregate -- all three old
toggles defaulted off).
* When true: drains video_st->frame_time_count = 0 after
fast-forward toggle-off, save state, or load state. Same
three events the previous toggles handled; combined under
one switch.
* When 'video_frame_time_sample_gated' is true, this toggle is
hidden from the menu (still readable from config if the user
set it manually, but irrelevant -- no samples to clean up).
Migration
=========
Old config keys
frame_time_counter_reset_after_fastforwarding
frame_time_counter_reset_after_load_state
frame_time_counter_reset_after_save_state
are silently ignored on load (config parser doesn't warn on
unknown bool keys). Users who had any of them enabled will see
the new combined setting at its default (false) and need to
re-enable once if they want the cleanup behaviour. Acceptable
regression given the prior defaults were all off anyway.
Wiring
======
config.def.h DEFAULT_FRAME_TIME_COUNTER_AUTO_RESET
configuration.{c,h} struct field + SETTING_BOOL
command.c load/save state reset sites
runloop.c fastmotion reset sites
(incl. runloop_apply_fastmotion_override
parameter rename)
msg_hash.h enum
msg_hash_lbl_str.h config key string
intl/msg_hash_us.h 'Auto-Reset After Disruptive Events' +
sublabel describing the relationship to
the gated sampling setting
menu/menu_setting.c CONFIG_BOOL registration (single, replacing
three)
menu/menu_displaylist.c build_list keeps the always-shown items
(REFRESH_RATE_AUTO + GATED toggle); the
AUTO_RESET entry is added via an explicit
conditional PARSE_SETTINGS_ENUM call gated
on !video_frame_time_sample_gated
menu/cbs/menu_cbs_sublabel.c sublabel binding (single, replacing three)
Compile-checked: command.o, runloop.o, configuration.o,
intl/msg_hash_us.o, menu/menu_displaylist.o,
menu/cbs/menu_cbs_sublabel.o all clean. menu/menu_setting.o has
the same pre-existing DEFAULT_PLAYLIST_SHOW_INLINE_CORE_NAME
sandbox build failure as before, unrelated.
Follow-up to the cached-frame redesign series (934dcf5..2d9af0d).
After the API migration landed, no TU outside gfx/video_driver.c
referenced frame_cache_data / frame_cache_width /
frame_cache_height / frame_cache_pitch -- the privacy was
enforced by convention. This commit makes it structural: the
four fields move out of video_driver_state_t and become
file-scope statics in video_driver.c.
Effect:
- The fields cannot be referenced from another TU even by
accident, since they're not in the struct anymore and they're
static. Future code can't reintroduce the pre-redesign UAF
pattern by typo.
- sizeof(video_driver_state_t) shrinks by 24 bytes on 64-bit
(one pointer + one size_t + two unsigned). Only one instance
exists, so the size win is symbolic; the readability win is
the point.
- All internal video_driver.c sites that previously did
video_st->frame_cache_* now do bare frame_cache_*. Identical
semantics; the lock-and-callback contract on cached_frame_read
/ publish / invalidate is unchanged.
Stale comments in command.c (the reinit snapshot block) updated
to reference the publish API instead of the no-longer-existing
field. A residual mention of 'frame_cache_data' in runloop.c
and gfx/drivers/d3d12.c is comment-only and accurate as a
description of the cached frame's role.
Compile-checked: gfx/video_driver.c plus every consumer file
that touched the old fields (runloop.c, retroarch.c, command.c,
tasks/task_*.c, cheevos/cheevos.c) all build clean against the
extracted statics.
Commit 5 of 5, closing the frame-cache redesign. Builds on the
new cached_frame_{info,read,is_hw_render} read API (2ceb575) and
the consumer migrations in a9165a8 / 0b7214a / 1f11b2518b.
The redesign in one sentence: turn the bare
video_driver_state_t::frame_cache_data pointer (which any caller
could read at any time, with no lifetime contract) into a private
piece of state guarded by a mutex, accessed only through a
callback-based API whose contract is 'the pointer handed to the
callback is valid for the callback's duration.'
== Concretely ==
New producer-side API:
video_driver_cached_frame_publish(data, w, h, p)
Install a new (data, dims) tuple under the lifetime lock.
Replaces the direct field-write in video_driver_frame() (the
main producer), in the command_event_reinit replay path, in
task_screenshot.c's read_frame_raw block, and in
video_driver_init_internal's dummy_pixels seeding.
video_driver_cached_frame_invalidate(void)
NULL-out the cached frame under the lifetime lock. On
return, the buffer the cached frame pointed at is safe to
free -- any concurrent reader has completed. Replaces every
'frame_cache_data = NULL' direct assignment.
The lock:
static slock_t *cached_frame_lock, HAVE_THREADS-guarded.
Allocated lazily on first video_driver_init_internal; kept for
process lifetime (one-shot allocation, no shutdown free needed
given size). When HAVE_THREADS is undefined, the helpers
degenerate to no-ops, matching the rest of video_driver.c's
conditional threading.
The read side now takes the lock for the duration of the
callback (cached_frame_read), or briefly for the metadata snap
(cached_frame_info / cached_frame_is_hw_render). Producer-side
publish / invalidate take the lock around their (data, dims)
update. The same-thread cached_frame_replay path
(video_driver_cached_frame, called from drivers' read_viewport
setup and the paused / menu-only render) stays lockless: it
runs on the same runloop thread as the producer, so there is no
race for it to lose, and taking the lock here would re-enter
through frame_cb->video_driver_frame->cached_frame_publish and
deadlock (slock is non-recursive).
== Defensive code removed ==
vulkan_deinit_textures: dropped the manual
vulkan_is_mapped_swapchain_texture_ptr check + conditional NULL.
Unconditional cached_frame_invalidate() before the destroy is
both simpler and stronger -- the previous check only covered the
synchronous case where the read happened on the same thread as
the destroy, missing any async consumer mid-read. The lock
covers both. vulkan_is_mapped_swapchain_texture_ptr's body and
forward declaration are removed; it had no other callers.
d3d12_sw_fb_ensure / d3d12_gfx_free: same simplification. The
ptr-equality guard against d3d12->sw_fb.mapped becomes
cached_frame_invalidate() before the Unmap / Release.
== Sites migrated to the API ==
Direct field writes:
gfx/video_driver.c -- video_driver_frame producer,
video_driver_init_internal dummy_pixels seed.
retroarch.c (3 sites) -- driver deinit, full state clear,
pre-init guard.
runloop.c (5 sites) -- core unload / deinit / init,
load_game, reset.
command.c -- command_event_reinit replay phase.
task_screenshot.c -- supports_read_frame_raw publish (also
simplified from a buggy save-and-restore that mixed old
data with new dims on failure).
Sentinel checks (frame_cache_data == RETRO_HW_FRAME_BUFFER_VALID):
runloop.c, retroarch.c, cheevos/cheevos.c, gfx/drivers/ctr_gfx.c
All to video_driver_cached_frame_is_hw_render().
Geometry-fallback reads:
menu/menu_setting.c (two identical sites) -- cached_frame_info
call to pull cache dims as fallback when av_info geometry is
zero.
== Audit ==
After this commit, the only references to frame_cache_data /
frame_cache_width / frame_cache_height / frame_cache_pitch are
inside gfx/video_driver.c (the implementation) and comments /
forward-declarations in gfx/video_driver.h. Grep -r over the
tree returns zero direct accesses from any other TU.
== Historical attribution ==
This work finishes a path Dwedit started in July 2018. His
'cached_frame_hack2' branch on github.com/Dwedit/RetroArch
introduced the lifecycle-boundary NULL-outs that have lived in
master ever since (essentially the same set the runloop carries
today: core unload / deinit / init / reset / load_game / video
destroy / video init). That patch closed the synchronous UAF
case but couldn't reach the async one, where a task worker
captures the pointer before the NULL-out fires and dereferences
after. His parallel 'cached_frame_removal2' branch took the
other direction -- abolish the field entirely, force all
consumers through GPU read_viewport -- and was self-marked 'DO
NOT MERGE' because removal sacrifices the raw-frame path that
video_gpu_screenshot=false, savestate thumbnails, and AI OCR all
depend on.
This series occupies the middle ground neither of his branches
took: keep the field, keep the raw-frame use cases, but make the
field private and gate all access through an API whose lifetime
contract is structurally enforceable. hack2's lifecycle
NULL-outs survive as internal cached_frame_invalidate() calls;
removal2's design pressure ('a pointer guaranteed to be
dangling is dangerous') is honoured by the lock that makes the
pointer no longer dangling once a reader has it.
Commit 4 of 5 in the frame-cache redesign. Migrates the remaining
consumers off direct video_driver_state_t::frame_cache_* field
reads onto the lifetime-safe API introduced in 2ceb575.
tasks/task_save.c (5 sites):
Five identical 'is the cached frame HW-render?' sentinel checks
used to gate the SAVE_TASK_FLAG_HAS_VALID_FB flag on savestate
task creation. All replaced with the new
video_driver_cached_frame_is_hw_render() accessor. Behaviour
is identical (the new helper computes the same
(data && data == RETRO_HW_FRAME_BUFFER_VALID) predicate).
tasks/task_screenshot.c (screenshot_dump savestate path):
Reading frame_cache_width / frame_cache_height to decide the
savestate thumbnail dimensions. Replaced with
cached_frame_info() out-parameters. The remaining direct
frame_cache_data uses in this file (the
supports_read_frame_raw save-and-restore block) are not in
the async UAF surface and will become internal calls when the
field is made private in the next commit.
command.c (command_event_reinit defensive snapshot):
This code was already implementing exactly the
copy-inside-callback pattern that cached_frame_read makes
general -- it was the proof-of-concept that the redesign
generalises. Refactor the snapshot phase to use
cached_frame_read with command_reinit_snapshot_cb as the
callback; the publish phase (writing frame_cache_* fields
to hand the snapshot to the new driver) stays as direct
writes for now and becomes an internal call through the
setter API in commit 5.
Historical note: a 2018 patch series by Dwedit ("cached_frame_hack2"
on github.com/Dwedit/RetroArch) added the lifecycle-boundary
NULL-outs that this codebase has carried since. Those addressed
the synchronous UAF; this series closes the async window that
hack2 didn't reach, while keeping the CPU-side cached frame
that hack2's alternative branch (cached_frame_removal2) would
have removed entirely.
Add null pointer checks after malloc() to prevent crashes when memory
allocation fails. The original code used malloc() return value
immediately without checking for NULL, causing segmentation fault
on allocation failure.
HEAP CORRUPTION VULNERABILITIES FIXED:
- command_read_ram(): malloc() used without null check at line 997
- command_read_memory(): malloc() used without null check at line 1238
- Both functions now check malloc() return value and handle OOM gracefully
- Added error messages for out-of-memory conditions
SECURITY IMPACT:
- Prevents null pointer dereference crashes on low memory conditions
- Improves stability under memory pressure
- Eliminates potential DoS vector via memory exhaustion
- Maintains command interface availability during OOM
This vulnerability could cause crashes when processing memory read
commands under low memory conditions or during targeted memory
exhaustion attacks.
Prevent potential buffer overflow when writing null terminator to stdin_buf.
Add bounds check before the write to ensure stdin_buf_ptr stays within
the CMD_BUF_SIZE buffer boundaries.
UNDEFINED BEHAVIOR FIX:
- stdin_buf is CMD_BUF_SIZE (4096) bytes
- stdin_buf_ptr could potentially exceed bounds after read
- Writing stdin_buf[stdin_buf_ptr] = '\0' without bounds check is UB
- Added defensive bounds checking before null terminator write
This prevents potential out-of-bounds memory access that could lead to
memory corruption or security vulnerabilities.
reinit and core deinit
Two minor doc cleanups - no behavioral change.
command.c: the comment on the cached_snapshot static buffer
in command_event_reinit claimed the buffer is 'freed at
shutdown', but no shutdown free path exists. Update the comment
to describe what actually happens: the static is not freed,
but it's bounded at one framebuffer's worth of memory and
reclaimed by the OS at process exit. Note explicitly that
adding a teardown hook would require wiring the static into
retroarch_deinit_drivers and that we deliberately don't.
retroarch.c: the CMD_EVENT_CORE_DEINIT handler recursively
calls command_event(CMD_EVENT_UNPAUSE, NULL) at its top,
re-entering the same dispatcher. The current UNPAUSE branch
is small enough that the self-call is safe, but the safety
constraint isn't documented. Add a comment so future changes
to UNPAUSE that would touch core state get noticed during
review (we're mid-deinit; touching core state from there
would be a UAF).
Apply strlcpy_append (commit 78c52ab) to three further sites
identified in the wider strlcpy-misuse sweep, all with the
same shape that drove the prior commits c41e955 / 78c52ab /
e446242:
_len = strlcpy(buf, src1, sizeof(buf));
_len += strlcpy(buf + _len, src2, sizeof(buf) - _len);
... (more steps) ...
That chain is unsafe because strlcpy returns strlen(source)
regardless of truncation; on overflow _len overshoots
sizeof(buf) and the next sizeof - _len underflows size_t,
producing an unbounded copy that writes past the buffer.
Sites:
gfx/drivers/gl2.c:4549 - GL device-info string assembly.
gl->device_str is 128 bytes; the chain appends `vendor`,
' ', and `renderer` (all from glGetString). Some drivers
report long vendor/renderer combinations. Not attacker-
reachable in the usual threat model but the pattern is
still wrong, and the `device_str[_len]=' '; device_str
[++_len]='\0'` separator chain in the middle was the
short-chain shape from commit e446242.
menu/menu_displaylist.c:2767 - create_string_list_rdb_entry
_string. Builds out_lbl (NAME_MAX_LENGTH bytes) from
label + "|" + actual_string + "|" + path, plus tmp (128
bytes) from desc + ": " + actual_string. The strings
come from RetroArch database (.rdb) entries; a malicious
.rdb could supply long values to drive the chain
underflow. Modest threat model -- users typically use
the RetroArch-shipped databases, not custom ones -- but
the pattern is the same one we've fixed elsewhere.
command.c:1119 - GET_STATUS network reply. Builds the
reply for the network command interface from the paused/
playing state, system_id or library_name, content
basename, and CRC. reply is 8192 bytes which fits any
realistic content, but ROM basenames can legally be any
length and the chain has 8 steps; a long enough basename
drives the underflow.
Replace each chain with sequences of strlcpy_append calls
on a single rolling cursor. Same correctness story as
e446242: helper handles the bound check at every step,
truncation is silent and contained, no behaviour change
for the buffer-fits-comfortably case.
No new tests -- the strlcpy_append regression test from
78c52ab covers the helper's contract; these are additional
users.
Triaged but not fixed in this commit (safe by construction,
flagged for the record):
network/cloud_sync/webdav.c:401 - 16-call chain, but
the function pre-calculates the exact required length
via strlen() over the same inputs and mallocs that
exact size before the chain runs. Single-threaded.
retroarch.c:8769 - 20-call chain for SIMD feature names
into a 128-byte buffer. Total max ~120 bytes; tight but
no real CPU has all SIMD bits set so practical max is
well under cap.
gfx/video_shader_parse.c:1252, configuration.c:6961/7167,
frontend/drivers/platform_emscripten.c:620 - classifier
false positives (each _len is per-buffer reused, not
accumulated cross-buffer).
Three sibling functions that construct command_t handles
(command_network_new, command_stdin_new, command_emscripten_new)
all followed the same broken allocation order:
cmd = calloc(...);
inner = calloc(...);
if (!cmd)
return NULL;
if (!inner) { free(cmd); return NULL; }
If cmd's calloc fails but inner's succeeds, the '!cmd' early return
leaks inner. Likewise in command_network_new, whose error handler
was actually fine at the end of the function, the code before it
did
netcmd->net_fd = fd;
cmd->userptr = netcmd;
with no NULL check between the two callocs and the assignments - an
OOM on either calloc would segfault on the very next line, never
reaching the error label.
The sibling command_uds_new already uses the correct sequential
pattern: allocate, check, allocate, check-with-cleanup-of-first.
Apply that pattern to the other three.
command_network_new also gets one unrelated but same-function fix:
the 'error:' label's teardown did not call socket_close() on fd,
so the socket_nonblock / socket_bind failure paths (which run AFTER
fd was opened and assigned into netcmd->net_fd) leaked the socket
file descriptor. Moved fd into a local initialised to -1 so the
single 'if (fd >= 0) socket_close(fd)' branch in the error handler
covers every exit path that had a real fd, without relying on
netcmd being non-NULL.
Also moves the 'socket_init' call after the two callocs rather than
interleaved with them. This has no behavioural consequence -
socket_init's side effects (network_init, getaddrinfo) are
idempotent / reversible via freeaddrinfo_retro - but keeps the
allocate-check-allocate-check block together, which is what the
whole refactor is aiming for in the first place.
No scope for OOM-on-small-calloc to actually hit these paths in
current practice (all three functions run once at RetroArch startup,
not per-command-message), so this is correctness-only rather than a
user-visible fix. Still worth doing - the NULL-deref in particular
is a real bug waiting for the right-shaped OOM.
command_event_reinit tears the video driver down via
video_driver_free, which nulls video_st->frame_cache_data (the
pointer was borrowed from the core's own framebuffer and isn't
guaranteed to stay live across the driver swap). The new driver
then comes up with no cached frame to replay, so reinits triggered
from inside the menu (e.g. toggling video_hdr_mode between HDR10
and scRGB on Metal) leave the paused-core background missing until
the user F1-cycles back through the core.
Snapshot the cached frame into a static buffer before reinit,
restore the pointer into video_st->frame_cache_data afterwards, and
call video_driver_cached_frame() to push it through the new
driver's frame callback. The static buffer is realloc'd on demand
and reused across reinits so there's no leak — the core's next real
frame will simply overwrite the frame_cache_data pointer at its
leisure.
In the Metal driver, additionally latch a _frameEverUploaded flag
so that on frame==NULL calls before the first real frame lands
(e.g. in the one-or-two-frame window before the cached replay
completes), we can also fall back to reading
video_st->frame_cache_data directly from inside renderFrame. The
two paths overlap cleanly: the frontend snapshot keeps the cache
pointer valid across reinit, and the driver-side latch guards the
transient.
Fixes paused-core background disappearing after HDR mode toggle on
Metal; harmless elsewhere because other drivers either don't
rebuild their internal frame texture on reinit (keeping content
across the swap by accident) or already have equivalent workarounds.
Files
command.c : snapshot-and-replay around video_driver_reinit.
gfx/drivers/metal.m : _frameEverUploaded latch + cached-frame
fallback in renderFrame.
* Remove strtok_r usage in command_network_send - at least 2x faster on avg
* Don't remove strtok_r in command_parse_msg - replacing it there will make
it slower
* Makefile.ctr: enable gc-sections
This frees ~160k of RAM without any downsides that I know of.
It's not much, but helps pcsx_rearmed on old3ds where it barely fits in RAM.
* remove some dupe logic
Regardless if `save_state_in_background` is enabled or not, it does
exactly the same so the check can be removed.
* optimize RAM state load
content_load_state_from_ram() doesn't need the backup copy,
content_save_state() never touches the ram_buf. It seems the whole
logic was copy-pasted from content_undo_load_state() without thinking.
* avoid useless memory use and copying
Just take the source pointer instead of making a copy and then freeing it.
This is a big deal on slow platforms having to deal with large states,
like psx on old3ds.
* allow the core to disable the savestate undo feature
The undo feature may amount to at least 4 simultaneous buffers:
save undo, load undo, the current save/load buffer, temporary buffer.
When emulating a system with relatively large states on a system with
little RAM (like PSX on old3ds) that amounts to something like ~18MB of
~42MB available to the core. Under memory pressure the libctru variation
in RetroArch tries to salvage some memory from the main stack and linear
memory, but since those are variable, they start to overlap creating
instability.
The solution is to just disable the undo feature.
* Add save checkpoint, prev/next checkpoint commands and hotkeys
* Fix copy paste error in meta binds
* Implement save_replay_checkpoint command
* Implement prev/next checkpoint commands
* Update per @jamiras review
* add command to seek to a specific frame number
* Add message queue toasts for seek forward/back
* Fix basis for seek, produce full message on reply
* fix bugs seeking back during record
* Tidy up
* Allow back and forwards seeking of recording replays while paused
* Update changelog
Lib