From e628fe3cb2f412eb7709b40656daed76e36c5cdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Wed, 30 Sep 2026 11:12:44 -0600 Subject: [PATCH] Achievements: Fix leaderboard screen use-after-free on close rc_client frees a request's async handle right after calling back. The error path didn't clear pendingAsyncCall_, so closing the screen after a failed fetch aborted a freed handle. Switching between Top players and Around me also overwrote the handle without aborting the old request, which could then call back into a closed screen, or leak an entry list. Co-Authored-By: Claude Opus 5.5 (1M context) --- UI/RetroAchievementScreens.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/UI/RetroAchievementScreens.cpp b/UI/RetroAchievementScreens.cpp index 1a24823a5e..3198ba8de0 100644 --- a/UI/RetroAchievementScreens.cpp +++ b/UI/RetroAchievementScreens.cpp @@ -258,20 +258,28 @@ RetroAchievementsLeaderboardScreen::RetroAchievementsLeaderboardScreen(const Pat void RetroAchievementsLeaderboardScreen::FetchEntries() { auto callback = [](int result, const char *error_message, rc_client_leaderboard_entry_list_t *list, rc_client_t *client, void *userdata) { + RetroAchievementsLeaderboardScreen *thiz = (RetroAchievementsLeaderboardScreen *)userdata; + // rc_client frees the handle after this returns, so it must not be aborted later. + thiz->pendingAsyncCall_ = nullptr; if (result != RC_OK) { g_OSD.Show(OSDType::MESSAGE_ERROR, error_message, 10.0f); return; } - - RetroAchievementsLeaderboardScreen *thiz = (RetroAchievementsLeaderboardScreen *)userdata; + if (thiz->pendingEntryList_) { + rc_client_destroy_leaderboard_entry_list(thiz->pendingEntryList_); + } thiz->pendingEntryList_ = list; - thiz->pendingAsyncCall_ = nullptr; }; + // An aborted request doesn't call back, so only the latest request can reach the callback. + if (pendingAsyncCall_) { + rc_client_abort_async(Achievements::GetClient(), pendingAsyncCall_); + pendingAsyncCall_ = nullptr; + } + // Store the handle so the destructor can abort it if we're closed before the // response arrives - without this, the callback above would run later against - // a freed `this` (its pendingAsyncCall_ null-check in the destructor was - // otherwise always a no-op since this was never assigned). + // a freed `this`. if (nearMe_) { pendingAsyncCall_ = rc_client_begin_fetch_leaderboard_entries_around_user(Achievements::GetClient(), leaderboardID_, 10, callback, this); } else {