Skip to content

Commit fd50fe1

Browse files
committed
cheevos: make queued_command atomic to fix bg-thread /
main-thread race rcheevos_locals.queued_command was a plain enum event_command, written from background threads (the rcheevos client's HTTP completion callback at the bottom of rcheevos_client_load_game_callback, and the rewind init/deinit toggles inside rcheevos_apply_hardcore_state) and read on the main thread by rcheevos_test once per frame. Concurrent plain access to a non-atomic field is undefined: there is no memory barrier publishing the writer's prior writes, no guarantee that the reader observes the value at all on weak-memory targets, and on architectures where enum is wider than the naturally-atomic store size, a torn read. Replace the field type with retro_atomic_int_t. Writers go through retro_atomic_store_release_int; the reader in rcheevos_test takes a single acquire-load snapshot at the top of the dispatch block, preventing the three previous reads (the !=NONE check, the ==FINALIZE check, and the dispatch argument) from observing different values if a writer fires between them. Mirrors the atomic discipline already established for gfx_thumbnail_t.status (gfx/gfx_thumbnail.c:50-65). The static initializer for atomic_int with literal 0 is well-defined for all retro_atomic.h backends (C11 stdatomic, C++11 std::atomic, and the volatile-int fallback) - and CMD_EVENT_NONE happens to be 0. This is the first of two patches addressing the queued_command hazard. The second adds a load-generation counter to filter stale bg-thread completions that race with rcheevos_unload, preventing a finalize for game A from being applied to the memory map of game B.
1 parent 3741954 commit fd50fe1

2 files changed

Lines changed: 36 additions & 12 deletions

File tree

cheevos/cheevos.c

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ static rcheevos_locals_t rcheevos_locals =
9090
NULL, /* client */
9191
{{0}},/* memory */
9292
#ifdef HAVE_THREADS
93-
CMD_EVENT_NONE, /* queued_command */
93+
/* queued_command (atomic). CMD_EVENT_NONE == 0; static
94+
* zero-initialization satisfies all retro_atomic.h backends. */
95+
0,
9496
#endif
9597
"", /* user_agent_prefix */
9698
"", /* user_agent_core */
@@ -812,7 +814,8 @@ bool rcheevos_unload(void)
812814
rc_client_unload_game(rcheevos_locals.client);
813815

814816
#ifdef HAVE_THREADS
815-
rcheevos_locals.queued_command = CMD_EVENT_NONE;
817+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
818+
CMD_EVENT_NONE);
816819
#endif
817820

818821
if (rcheevos_locals.memory.count > 0)
@@ -836,7 +839,8 @@ bool rcheevos_unload(void)
836839
}
837840

838841
#ifdef HAVE_THREADS
839-
rcheevos_locals.queued_command = CMD_EVENT_NONE;
842+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
843+
CMD_EVENT_NONE);
840844
#endif
841845

842846
if (!config_get_ptr()->arrays.cheevos_token[0])
@@ -919,7 +923,8 @@ static void rcheevos_toggle_hardcore_active(rcheevos_locals_t* locals)
919923
/* have to "schedule" this.
920924
* CMD_EVENT_REWIND_DEINIT should
921925
* only be called on the main thread */
922-
rcheevos_locals.queued_command = CMD_EVENT_REWIND_DEINIT;
926+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
927+
CMD_EVENT_REWIND_DEINIT);
923928
}
924929
else
925930
#endif
@@ -943,7 +948,8 @@ static void rcheevos_toggle_hardcore_active(rcheevos_locals_t* locals)
943948
/* have to "schedule" this.
944949
* CMD_EVENT_REWIND_INIT should
945950
* only be called on the main thread */
946-
rcheevos_locals.queued_command = CMD_EVENT_REWIND_INIT;
951+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
952+
CMD_EVENT_REWIND_INIT);
947953
}
948954
else
949955
#endif
@@ -1114,14 +1120,23 @@ Test all the achievements (call once per frame).
11141120
void rcheevos_test(void)
11151121
{
11161122
#ifdef HAVE_THREADS
1117-
if (rcheevos_locals.queued_command != CMD_EVENT_NONE)
1123+
/* Snapshot the queued command once with an acquire-load. The
1124+
* matching release-stores are at the writer sites in this file
1125+
* (rcheevos_unload, the rewind toggles, and the bg-thread
1126+
* finalize at the bottom of rcheevos_client_load_game_callback).
1127+
* Without the snapshot the three reads at the previous
1128+
* !=NONE / ==FINALIZE / dispatch sites could each see a
1129+
* different value if a writer fires between them. */
1130+
int cmd = retro_atomic_load_acquire_int(&rcheevos_locals.queued_command);
1131+
if (cmd != CMD_EVENT_NONE)
11181132
{
1119-
if (rcheevos_locals.queued_command == CMD_CHEEVOS_FINALIZE_LOAD)
1133+
if (cmd == CMD_CHEEVOS_FINALIZE_LOAD)
11201134
rcheevos_finalize_game_load_on_ui_thread();
11211135
else
1122-
command_event(rcheevos_locals.queued_command, NULL);
1136+
command_event((enum event_command)cmd, NULL);
11231137

1124-
rcheevos_locals.queued_command = CMD_EVENT_NONE;
1138+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
1139+
CMD_EVENT_NONE);
11251140
}
11261141
#endif
11271142

@@ -1733,7 +1748,8 @@ static void rcheevos_client_load_game_callback(int result,
17331748
/* Have to "schedule" this. Game image should not be
17341749
* loaded into memory on background thread */
17351750
if (!task_is_on_main_thread())
1736-
rcheevos_locals.queued_command = (enum event_command)CMD_CHEEVOS_FINALIZE_LOAD;
1751+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
1752+
CMD_CHEEVOS_FINALIZE_LOAD);
17371753
else
17381754
#endif
17391755
rcheevos_finalize_game_load_on_ui_thread();
@@ -1752,7 +1768,8 @@ bool rcheevos_load(const void *data)
17521768
&& settings->bools.cheevos_enable;
17531769

17541770
#ifdef HAVE_THREADS
1755-
rcheevos_locals.queued_command = CMD_EVENT_NONE;
1771+
retro_atomic_store_release_int(&rcheevos_locals.queued_command,
1772+
CMD_EVENT_NONE);
17561773
#endif
17571774

17581775
/* If achievements are not enabled, or the core doesn't

cheevos/cheevos_locals.h

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
#ifdef HAVE_THREADS
2828
#include <rthreads/rthreads.h>
29+
#include <retro_atomic.h>
2930
#endif
3031

3132
#include <retro_common_api.h>
@@ -85,7 +86,13 @@ typedef struct rcheevos_locals_t
8586
rc_libretro_memory_regions_t memory;/* achievement addresses to core memory mappings */
8687

8788
#ifdef HAVE_THREADS
88-
enum event_command queued_command; /* action queued by background thread to be run on main thread */
89+
/* Action queued by a background thread (e.g. the rcheevos
90+
* client's HTTP completion thread) to be dispatched on the
91+
* main thread by rcheevos_test. Atomic because writers can
92+
* be on a worker thread while the main-thread reader is
93+
* polling per frame; plain enum access would be a data race
94+
* with no memory barrier. */
95+
retro_atomic_int_t queued_command;
8996
#endif
9097

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

0 commit comments

Comments
 (0)