From 8b98f60e3c4c8a11f9de382fbf694173855f3a7a Mon Sep 17 00:00:00 2001
From: Weiyi Wang <wwylele@gmail.com>
Date: Tue, 2 Oct 2018 11:06:42 -0400
Subject: input/sdl: lock map mutex after SDL call

Any SDL invocation can call the even callback on the same thread, which can call GetSDLJoystickBySDLID and eventually cause double lock on joystick_map_mutex. To avoid this, lock guard should be placed as closer as possible to the object accessing code, so that any SDL invocation is with the mutex unlocked
---
 src/input_common/sdl/sdl_impl.cpp | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

(limited to 'src/input_common')

diff --git a/src/input_common/sdl/sdl_impl.cpp b/src/input_common/sdl/sdl_impl.cpp
index 83fcf354e..f5dbd015e 100644
--- a/src/input_common/sdl/sdl_impl.cpp
+++ b/src/input_common/sdl/sdl_impl.cpp
@@ -159,9 +159,9 @@ std::shared_ptr<SDLJoystick> SDLState::GetSDLJoystickByGUID(const std::string& g
  * it to a SDLJoystick with the same guid and that port
  */
 std::shared_ptr<SDLJoystick> SDLState::GetSDLJoystickBySDLID(SDL_JoystickID sdl_id) {
-    std::lock_guard<std::mutex> lock(joystick_map_mutex);
     auto sdl_joystick = SDL_JoystickFromInstanceID(sdl_id);
     const std::string guid = GetGUID(sdl_joystick);
+    std::lock_guard<std::mutex> lock(joystick_map_mutex);
     auto map_it = joystick_map.find(guid);
     if (map_it != joystick_map.end()) {
         auto vec_it = std::find_if(map_it->second.begin(), map_it->second.end(),
@@ -193,13 +193,13 @@ std::shared_ptr<SDLJoystick> SDLState::GetSDLJoystickBySDLID(SDL_JoystickID sdl_
 }
 
 void SDLState::InitJoystick(int joystick_index) {
-    std::lock_guard<std::mutex> lock(joystick_map_mutex);
     SDL_Joystick* sdl_joystick = SDL_JoystickOpen(joystick_index);
     if (!sdl_joystick) {
         LOG_ERROR(Input, "failed to open joystick {}", joystick_index);
         return;
     }
     std::string guid = GetGUID(sdl_joystick);
+    std::lock_guard<std::mutex> lock(joystick_map_mutex);
     if (joystick_map.find(guid) == joystick_map.end()) {
         auto joystick = std::make_shared<SDLJoystick>(guid, 0, sdl_joystick);
         joystick_map[guid].emplace_back(std::move(joystick));
@@ -218,16 +218,22 @@ void SDLState::InitJoystick(int joystick_index) {
 }
 
 void SDLState::CloseJoystick(SDL_Joystick* sdl_joystick) {
-    std::lock_guard<std::mutex> lock(joystick_map_mutex);
     std::string guid = GetGUID(sdl_joystick);
-    // This call to guid is safe since the joystick is guaranteed to be in the map
-    auto& joystick_guid_list = joystick_map[guid];
-    const auto joystick_it =
-        std::find_if(joystick_guid_list.begin(), joystick_guid_list.end(),
-                     [&sdl_joystick](const std::shared_ptr<SDLJoystick>& joystick) {
-                         return joystick->GetSDLJoystick() == sdl_joystick;
-                     });
-    (*joystick_it)->SetSDLJoystick(nullptr, [](SDL_Joystick*) {});
+    std::shared_ptr<SDLJoystick> joystick;
+    {
+        std::lock_guard<std::mutex> lock(joystick_map_mutex);
+        // This call to guid is safe since the joystick is guaranteed to be in the map
+        auto& joystick_guid_list = joystick_map[guid];
+        const auto joystick_it =
+            std::find_if(joystick_guid_list.begin(), joystick_guid_list.end(),
+                         [&sdl_joystick](const std::shared_ptr<SDLJoystick>& joystick) {
+                             return joystick->GetSDLJoystick() == sdl_joystick;
+                         });
+        joystick = *joystick_it;
+    }
+    // Destruct SDL_Joystick outside the lock guard because SDL can internally call event calback
+    // which locks the mutex again
+    joystick->SetSDLJoystick(nullptr, [](SDL_Joystick*) {});
 }
 
 void SDLState::HandleGameControllerEvent(const SDL_Event& event) {
-- 
cgit v1.2.3