Merge pull request #13593 from Dentomologist/mainwindow_fix_use_after_free_during_dolphin_shutdown

MainWindow: Fix use-after-free during Dolphin shutdown

Source/Core/Common/Config/Config.h

@@ -29,9 +29,9 @@ void AddLayer(std::unique_ptr<ConfigLayerLoader> loader);
 std::shared_ptr<Layer> GetLayer(LayerType layer);
 void RemoveLayer(LayerType layer);
 
-// Returns an ID that can be passed to RemoveConfigChangedCallback().
-// The callback may be called from any thread.
-ConfigChangedCallbackID AddConfigChangedCallback(ConfigChangedCallback func);
+// Returns an ID that should be passed to RemoveConfigChangedCallback() when the callback is no
+// longer needed. The callback may be called from any thread.
+[[nodiscard]] ConfigChangedCallbackID AddConfigChangedCallback(ConfigChangedCallback func);
 void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id);
 void OnConfigChanged();
 

Source/Core/Core/AchievementManager.cpp

@@ -76,7 +76,7 @@ void AchievementManager::Init(void* hwnd)
                              [](const char* message, const rc_client_t* client) {
                                INFO_LOG_FMT(ACHIEVEMENTS, "{}", message);
                              });
-    Config::AddConfigChangedCallback([this] { SetHardcoreMode(); });
+    m_config_changed_callback_id = Config::AddConfigChangedCallback([this] { SetHardcoreMode(); });
     SetHardcoreMode();
     m_queue.Reset("AchievementManagerQueue", [](const std::function<void()>& func) { func(); });
     m_image_queue.Reset("AchievementManagerImageQueue",
@@ -764,6 +764,7 @@ void AchievementManager::Shutdown()
   {
     CloseGame();
     m_queue.Shutdown();
+    Config::RemoveConfigChangedCallback(m_config_changed_callback_id);
     std::lock_guard lg{m_lock};
     // DON'T log out - keep those credentials for next run.
     rc_client_destroy(m_client);

Source/Core/Core/AchievementManager.h

@@ -26,6 +26,7 @@
 #include <rcheevos/include/rc_runtime.h>
 
 #include "Common/CommonTypes.h"
+#include "Common/Config/Config.h"
 #include "Common/Event.h"
 #include "Common/HttpRequest.h"
 #include "Common/JsonUtil.h"
@@ -264,6 +265,7 @@ private:
   bool m_is_runtime_initialized = false;
   UpdateCallback m_update_callback = [](const UpdatedItems&) {};
   std::unique_ptr<DiscIO::Volume> m_loading_volume;
+  Config::ConfigChangedCallbackID m_config_changed_callback_id;
   Badge m_default_player_badge;
   Badge m_default_game_badge;
   Badge m_default_unlocked_badge;

Source/Core/Core/CPUThreadConfigCallback.h

@@ -18,8 +18,9 @@ struct ConfigChangedCallbackID
   bool operator==(const ConfigChangedCallbackID&) const = default;
 };
 
-// returns an ID that can be passed to RemoveConfigChangedCallback()
-ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback func);
+// Returns an ID that should be passed to RemoveConfigChangedCallback() when the callback is no
+// longer needed.
+[[nodiscard]] ConfigChangedCallbackID AddConfigChangedCallback(Config::ConfigChangedCallback func);
 
 void RemoveConfigChangedCallback(ConfigChangedCallbackID callback_id);
 

Source/Core/Core/FreeLookConfig.cpp

@@ -3,6 +3,8 @@
 
 #include "Core/FreeLookConfig.h"
 
+#include <optional>
+
 #include "Core/AchievementManager.h"
 #include "Core/CPUThreadConfigCallback.h"
 #include "Core/Config/AchievementSettings.h"
@@ -14,7 +16,8 @@ namespace FreeLook
 {
 static Config s_config;
 static Config s_active_config;
-static bool s_has_registered_callback = false;
+static std::optional<CPUThreadConfigCallback::ConfigChangedCallbackID>
+    s_config_changed_callback_id = std::nullopt;
 
 Config& GetConfig()
 {
@@ -39,14 +42,23 @@ Config::Config()
 
 void Config::Refresh()
 {
-  if (!s_has_registered_callback)
+  if (!s_config_changed_callback_id.has_value())
   {
-    CPUThreadConfigCallback::AddConfigChangedCallback([] { s_config.Refresh(); });
-    s_has_registered_callback = true;
+    s_config_changed_callback_id =
+        CPUThreadConfigCallback::AddConfigChangedCallback([] { s_config.Refresh(); });
   }
 
   camera_config.control_type = ::Config::Get(::Config::FL1_CONTROL_TYPE);
   enabled = ::Config::Get(::Config::FREE_LOOK_ENABLED) &&
             !AchievementManager::GetInstance().IsHardcoreModeActive();
 }
+
+void Config::Shutdown()
+{
+  if (!s_config_changed_callback_id.has_value())
+    return;
+
+  CPUThreadConfigCallback::RemoveConfigChangedCallback(*s_config_changed_callback_id);
+  s_config_changed_callback_id.reset();
+}
 }  // namespace FreeLook

Source/Core/Core/FreeLookConfig.h

@@ -27,6 +27,7 @@ struct Config final
 {
   Config();
   void Refresh();
+  void Shutdown();
 
   CameraConfig camera_config;
   bool enabled;

Source/Core/Core/FreeLookManager.cpp

@@ -325,8 +325,9 @@ InputConfig* GetInputConfig()
 void Shutdown()
 {
   s_config.UnregisterHotplugCallback();
-
   s_config.ClearControllers();
+
+  GetConfig().Shutdown();
 }
 
 void Initialize()

Source/Core/DolphinQt/MainWindow.cpp

@@ -34,6 +34,7 @@
 #include <qpa/qplatformnativeinterface.h>
 #endif
 
+#include "Common/Config/Config.h"
 #include "Common/ScopeGuard.h"
 #include "Common/Version.h"
 #include "Common/WindowSystemInfo.h"
@@ -277,7 +278,7 @@ MainWindow::MainWindow(Core::System& system, std::unique_ptr<BootParameters> boo
   if (AchievementManager::GetInstance().IsHardcoreModeActive())
     Settings::Instance().SetDebugModeEnabled(false);
   // This needs to trigger on both RA_HARDCORE_ENABLED and RA_ENABLED
-  Config::AddConfigChangedCallback(
+  m_config_changed_callback_id = Config::AddConfigChangedCallback(
       [this]() { QueueOnObject(this, [this] { this->OnHardcoreChanged(); }); });
   // If hardcore is enabled when the emulator starts, make sure it turns off what it needs to
   if (Config::Get(Config::RA_HARDCORE_ENABLED))
@@ -351,6 +352,7 @@ MainWindow::~MainWindow()
   Settings::Instance().ResetNetPlayServer();
 
 #ifdef USE_RETRO_ACHIEVEMENTS
+  Config::RemoveConfigChangedCallback(m_config_changed_callback_id);
   AchievementManager::GetInstance().Shutdown();
 #endif  // USE_RETRO_ACHIEVEMENTS
 

Source/Core/DolphinQt/MainWindow.h

@@ -11,6 +11,10 @@
 #include <optional>
 #include <string>
 
+#ifdef USE_RETRO_ACHIEVEMENTS
+#include "Common/Config/Config.h"
+#endif  // USE_RETRO_ACHIEVEMENTS
+
 #include "Core/Boot/Boot.h"
 
 class QMenu;
@@ -261,6 +265,7 @@ private:
 
 #ifdef USE_RETRO_ACHIEVEMENTS
   AchievementsWindow* m_achievements_window = nullptr;
+  Config::ConfigChangedCallbackID m_config_changed_callback_id;
 #endif  // USE_RETRO_ACHIEVEMENTS
 
   AssemblerWidget* m_assembler_widget;

Source/Core/DolphinQt/Settings.cpp

@@ -59,7 +59,7 @@ Settings::Settings()
     });
   });
 
-  Config::AddConfigChangedCallback([this] {
+  m_config_changed_callback_id = Config::AddConfigChangedCallback([this] {
     static std::atomic<bool> do_once{true};
     if (do_once.exchange(false))
     {
@@ -94,7 +94,10 @@ Settings::Settings()
   });
 }
 
-Settings::~Settings() = default;
+Settings::~Settings()
+{
+  Config::RemoveConfigChangedCallback(m_config_changed_callback_id);
+}
 
 void Settings::UnregisterDevicesChangedCallback()
 {

Source/Core/DolphinQt/Settings.h

@@ -10,6 +10,7 @@
 #include <QRadioButton>
 #include <QSettings>
 
+#include "Common/Config/Config.h"
 #include "Core/Config/MainSettings.h"
 #include "DiscIO/Enums.h"
 #include "InputCommon/ControllerInterface/ControllerInterface.h"
@@ -232,6 +233,7 @@ private:
   std::shared_ptr<NetPlay::NetPlayClient> m_client;
   std::shared_ptr<NetPlay::NetPlayServer> m_server;
   ControllerInterface::HotplugCallbackHandle m_hotplug_callback_handle;
+  Config::ConfigChangedCallbackID m_config_changed_callback_id;
 };
 
 Q_DECLARE_METATYPE(Core::State);

Source/Core/UICommon/UICommon.cpp

@@ -50,6 +50,9 @@
 #include "UICommon/DiscordPresence.h"
 #include "UICommon/USBUtils.h"
 
+#include "VideoCommon/VideoBackendBase.h"
+#include "VideoCommon/VideoConfig.h"
+
 #ifdef HAVE_QTDBUS
 #include "UICommon/DBusUtils.h"
 #endif
@@ -58,8 +61,6 @@
 #include <IOKit/pwr_mgt/IOPMLib.h>
 #endif
 
-#include "VideoCommon/VideoBackendBase.h"
-
 namespace UICommon
 {
 static Config::ConfigChangedCallbackID s_config_changed_callback_id;
@@ -130,14 +131,17 @@ void Init()
   Core::RestoreWiiSettings(Core::RestoreReason::CrashRecovery);
 
   Config::Init();
-  Config::AddConfigChangedCallback(InitCustomPaths);
+  const auto config_changed_callback = []() {
+    InitCustomPaths();
+    RefreshConfig();
+  };
+  s_config_changed_callback_id = Config::AddConfigChangedCallback(config_changed_callback);
   Config::AddLayer(ConfigLoaders::GenerateBaseConfigLoader());
   SConfig::Init();
   Discord::Init();
   Common::Log::LogManager::Init();
   VideoBackendBase::ActivateBackend(Config::Get(Config::MAIN_GFX_BACKEND));
 
-  s_config_changed_callback_id = Config::AddConfigChangedCallback(RefreshConfig);
   RefreshConfig();
 }
 
@@ -149,6 +153,7 @@ void Shutdown()
   WiimoteReal::Shutdown();
   Common::Log::LogManager::Shutdown();
   Discord::Shutdown();
+  g_Config.Shutdown();
   SConfig::Shutdown();
   Config::Shutdown();
 }

Source/Core/VideoCommon/VideoConfig.cpp

@@ -4,6 +4,7 @@
 #include "VideoCommon/VideoConfig.h"
 
 #include <algorithm>
+#include <optional>
 
 #include "Common/CPUDetect.h"
 #include "Common/CommonTypes.h"
@@ -33,7 +34,8 @@
 VideoConfig g_Config;
 VideoConfig g_ActiveConfig;
 BackendInfo g_backend_info;
-static bool s_has_registered_callback = false;
+static std::optional<CPUThreadConfigCallback::ConfigChangedCallbackID>
+    s_config_changed_callback_id = std::nullopt;
 
 static bool IsVSyncActive(bool enabled)
 {
@@ -50,14 +52,14 @@ void UpdateActiveConfig()
 
 void VideoConfig::Refresh()
 {
-  if (!s_has_registered_callback)
+  if (!s_config_changed_callback_id.has_value())
   {
     // There was a race condition between the video thread and the host thread here, if
     // corrections need to be made by VerifyValidity(). Briefly, the config will contain
     // invalid values. Instead, pause the video thread first, update the config and correct
     // it, then resume emulation, after which the video thread will detect the config has
     // changed and act accordingly.
-    CPUThreadConfigCallback::AddConfigChangedCallback([]() {
+    const auto config_changed_callback = []() {
       auto& system = Core::System::GetInstance();
 
       const bool lock_gpu_thread = Core::IsRunning(system);
@@ -69,8 +71,10 @@ void VideoConfig::Refresh()
 
       if (lock_gpu_thread)
         system.GetFifo().PauseAndLock(false, true);
-    });
-    s_has_registered_callback = true;
+    };
+
+    s_config_changed_callback_id =
+        CPUThreadConfigCallback::AddConfigChangedCallback(config_changed_callback);
   }
 
   bVSync = Config::Get(Config::GFX_VSYNC);
@@ -212,6 +216,15 @@ void VideoConfig::VerifyValidity()
   }
 }
 
+void VideoConfig::Shutdown()
+{
+  if (!s_config_changed_callback_id.has_value())
+    return;
+
+  CPUThreadConfigCallback::RemoveConfigChangedCallback(*s_config_changed_callback_id);
+  s_config_changed_callback_id.reset();
+}
+
 bool VideoConfig::UsingUberShaders() const
 {
   return iShaderCompilationMode == ShaderCompilationMode::SynchronousUberShaders ||

Source/Core/VideoCommon/VideoConfig.h

@@ -190,6 +190,7 @@ struct VideoConfig final
   VideoConfig() = default;
   void Refresh();
   void VerifyValidity();
+  static void Shutdown();
 
   // General
   bool bVSync = false;