Skip to content

Commit 0ce7158

Browse files
committed
cheevos: drop stale rc_client load completions via generation
counter Even with the atomic queued_command from the previous patch, a race remains: a background load callback for game A can complete *after* the user has closed content and started loading game B. The bg thread atomically writes CMD_CHEEVOS_FINALIZE_LOAD to queued_command; on the next frame, rcheevos_test dispatches rcheevos_finalize_game_load_on_ui_thread - applying game A's finalization (achievement runtime initialization, memory map hookup, hardcore enforcement) to game B's rcheevos_locals state. Add a load_generation atomic counter to rcheevos_locals_t. Bump it (atomic_inc) at the top of rcheevos_unload and rcheevos_load, before any other state mutation. Capture the current value as the userdata argument to rc_client_begin_identify_and_load_game (passed through as void* via intptr_t cast - the rc_client library treats userdata opaquely). The bg-thread callback checks the captured generation against the live counter at two points: - At the top of rcheevos_client_load_game_callback, before any work that mutates rcheevos_locals (rcheevos_init_memory, rc_client_set_read_memory_function, rcheevos_finalize_game_load). Stale callbacks return immediately without side effects. - Just before publishing CMD_CHEEVOS_FINALIZE_LOAD to queued_command. Between the entry check and here the bg thread runs a long sequence (network/file I/O, can take seconds); the user may unload mid-callback. The re-check is cheap and prevents a stale FINALIZE_LOAD from queuing. Both checks are HAVE_THREADS-gated and skipped when the callback runs on the main thread (which only happens when the rc_client synchronously dispatches; in that case the userdata sentinel is NULL but task_is_on_main_thread() returns true so the check is skipped). The kickoff site is also gated: under HAVE_THREADS the userdata is the captured generation, otherwise NULL. Closes the second half of the cheevos queued_command hazard. The first half (atomicity) was addressed in fd50fe1.
1 parent fd50fe1 commit 0ce7158

2 files changed

Lines changed: 92 additions & 2 deletions

File tree

cheevos/cheevos.c

Lines changed: 79 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@ static rcheevos_locals_t rcheevos_locals =
9393
/* queued_command (atomic). CMD_EVENT_NONE == 0; static
9494
* zero-initialization satisfies all retro_atomic.h backends. */
9595
0,
96+
/* load_generation (atomic). Starts at 0; bumped by
97+
* rcheevos_unload and rcheevos_load. */
98+
0,
9699
#endif
97100
"", /* user_agent_prefix */
98101
"", /* user_agent_core */
@@ -806,6 +809,17 @@ bool rcheevos_unload(void)
806809
{
807810
const bool was_loaded = rcheevos_is_game_loaded();
808811

812+
#ifdef HAVE_THREADS
813+
/* Bump the load generation FIRST, before any other state
814+
* mutation. Any background load callback already in flight
815+
* captured the previous generation at submit time; bumping
816+
* here makes its eventual generation check fail, so it
817+
* silently drops without writing FINALIZE_LOAD into
818+
* queued_command. The atomic store synchronizes with the
819+
* acquire-load on the bg thread. */
820+
retro_atomic_inc_int(&rcheevos_locals.load_generation);
821+
#endif
822+
809823
#ifdef HAVE_GFX_WIDGETS
810824
rcheevos_hide_widgets(gfx_widgets_ready());
811825
gfx_widget_set_cheevos_set_loading(false);
@@ -1663,6 +1677,29 @@ static void rcheevos_client_load_game_callback(int result,
16631677
const settings_t *settings = config_get_ptr();
16641678
const rc_client_game_t *game = rc_client_get_game_info(client);
16651679

1680+
#ifdef HAVE_THREADS
1681+
/* Stale-load filter. The userdata is the load_generation
1682+
* captured at rc_client_begin_identify_and_load_game time;
1683+
* if it no longer matches, the user has unloaded or started
1684+
* a new load while this one was in flight, and any further
1685+
* mutation of rcheevos_locals here would target the new
1686+
* session's state.
1687+
*
1688+
* The check is HAVE_THREADS-gated because only the threaded
1689+
* code path captures a real generation as userdata; the
1690+
* single-threaded build passes NULL (which would compare
1691+
* against the initial generation 0 and either match-or-miss
1692+
* unpredictably as the counter wraps). */
1693+
if (!task_is_on_main_thread())
1694+
{
1695+
intptr_t captured_gen = (intptr_t)userdata;
1696+
int current_gen = retro_atomic_load_acquire_int(
1697+
&rcheevos_locals.load_generation);
1698+
if ((intptr_t)current_gen != captured_gen)
1699+
return;
1700+
}
1701+
#endif
1702+
16661703
#if defined(HAVE_GFX_WIDGETS)
16671704
gfx_widget_set_cheevos_set_loading(false);
16681705
#endif
@@ -1748,8 +1785,22 @@ static void rcheevos_client_load_game_callback(int result,
17481785
/* Have to "schedule" this. Game image should not be
17491786
* loaded into memory on background thread */
17501787
if (!task_is_on_main_thread())
1788+
{
1789+
/* Re-check the generation just before publishing.
1790+
* Between the entry check and here we ran a long
1791+
* sequence of work (potentially seconds with slow
1792+
* network or disk); the user may have unloaded or
1793+
* started a new load in that window. The captured
1794+
* generation is in userdata. */
1795+
intptr_t captured_gen = (intptr_t)userdata;
1796+
int current_gen = retro_atomic_load_acquire_int(
1797+
&rcheevos_locals.load_generation);
1798+
if ((intptr_t)current_gen != captured_gen)
1799+
return;
1800+
17511801
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
17521802
CMD_CHEEVOS_FINALIZE_LOAD);
1803+
}
17531804
else
17541805
#endif
17551806
rcheevos_finalize_game_load_on_ui_thread();
@@ -1768,6 +1819,14 @@ bool rcheevos_load(const void *data)
17681819
&& settings->bools.cheevos_enable;
17691820

17701821
#ifdef HAVE_THREADS
1822+
/* Bump the load generation. Any background callback from a
1823+
* prior rc_client_begin_identify_and_load_game whose
1824+
* userdata-captured generation no longer matches will drop
1825+
* silently rather than writing FINALIZE_LOAD into
1826+
* queued_command and causing a stale finalize to be applied
1827+
* to the new game's state. See the matching comment in
1828+
* cheevos_locals.h. */
1829+
retro_atomic_inc_int(&rcheevos_locals.load_generation);
17711830
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
17721831
CMD_EVENT_NONE);
17731832
#endif
@@ -1882,8 +1941,26 @@ bool rcheevos_load(const void *data)
18821941
}
18831942
#endif
18841943

1885-
rc_client_begin_identify_and_load_game(rcheevos_locals.client, console_id,
1886-
info->path, (const uint8_t*)info->data, info->size, rcheevos_client_load_game_callback, NULL);
1944+
{
1945+
#ifdef HAVE_THREADS
1946+
/* Capture the current load generation; the callback
1947+
* compares this against the live value to detect a
1948+
* stale completion (i.e. the user closed/changed
1949+
* content while the load was in flight). The cast
1950+
* loses information only if HAVE_THREADS is enabled
1951+
* and a generation counter overflows intptr_t, which
1952+
* would require ~2^31 (or ~2^63) load events. */
1953+
intptr_t gen = (intptr_t)retro_atomic_load_acquire_int(
1954+
&rcheevos_locals.load_generation);
1955+
rc_client_begin_identify_and_load_game(rcheevos_locals.client, console_id,
1956+
info->path, (const uint8_t*)info->data, info->size,
1957+
rcheevos_client_load_game_callback, (void*)gen);
1958+
#else
1959+
rc_client_begin_identify_and_load_game(rcheevos_locals.client, console_id,
1960+
info->path, (const uint8_t*)info->data, info->size,
1961+
rcheevos_client_load_game_callback, NULL);
1962+
#endif
1963+
}
18871964
}
18881965

18891966
return true;

cheevos/cheevos_locals.h

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,19 @@ typedef struct rcheevos_locals_t
9393
* polling per frame; plain enum access would be a data race
9494
* with no memory barrier. */
9595
retro_atomic_int_t queued_command;
96+
97+
/* Bumped on every load lifecycle reset (rcheevos_load and
98+
* rcheevos_unload). Captured at rc_client_begin_identify_and_load_game
99+
* time and passed through the callback's userdata; the
100+
* background callback compares its captured value against the
101+
* current value at completion and drops stale completions
102+
* whose target rcheevos_locals state has been reset out from
103+
* under them. Without this, a load for game A that completes
104+
* after the user has closed content and started loading game B
105+
* would write FINALIZE_LOAD into queued_command and cause the
106+
* main thread to apply game A's finalization to game B's
107+
* memory map. */
108+
retro_atomic_int_t load_generation;
96109
#endif
97110

98111
char user_agent_prefix[128]; /* RetroArch/OS version information */

0 commit comments

Comments
 (0)