Revamped joystick locking

This makes the joystick locking more robust by holding the lock while updating joysticks.

The lock should be held when calling any SDL joystick function on a different thread than the one calling SDL_PumpEvents() and SDL_JoystickUpdate().

It is now possible to hold the lock while reinitializing the joystick subsystem, however any open joysticks will become invalid and potentially cause crashes if used afterwards.

Fixes https://github.com/libsdl-org/SDL/issues/6063

include/SDL_joystick.h

@@ -124,6 +124,11 @@ typedef enum
  * the API functions that take a joystick index will be valid, and joystick
  * and game controller events will not be delivered.
  *
+ * As of SDL 2.26.0, you can take the joystick lock around reinitializing
+ * the joystick subsystem, to prevent other threads from seeing joysticks
+ * in an uninitialized state. However, all open joysticks will be closed
+ * and calling SDL functions using them will potentially cause a crash.
+ *
  * \since This function is available since SDL 2.0.7.
  */
 extern DECLSPEC void SDLCALL SDL_LockJoysticks(void);

src/joystick/SDL_joystick.c

@@ -106,13 +106,21 @@ static SDL_JoystickDriver *SDL_joystick_drivers[] = {
     &SDL_DUMMY_JoystickDriver
 #endif
 };
-static SDL_bool SDL_joystick_allows_background_events = SDL_FALSE;
+static SDL_bool SDL_joysticks_initialized;
+static SDL_bool SDL_joysticks_quitting = SDL_FALSE;
 static SDL_Joystick *SDL_joysticks = NULL;
-static SDL_bool SDL_updating_joystick = SDL_FALSE;
 static SDL_mutex *SDL_joystick_lock = NULL; /* This needs to support recursive locks */
+static int SDL_joysticks_locked;
 static SDL_atomic_t SDL_next_joystick_instance_id;
 static int SDL_joystick_player_count = 0;
 static SDL_JoystickID *SDL_joystick_players = NULL;
+static SDL_bool SDL_joystick_allows_background_events = SDL_FALSE;
+
+SDL_bool
+SDL_JoysticksInitialized(void)
+{
+    return SDL_joysticks_initialized;
+}
 
 void
 SDL_LockJoysticks(void)
@@ -120,21 +128,76 @@ SDL_LockJoysticks(void)
     if (SDL_joystick_lock) {
         SDL_LockMutex(SDL_joystick_lock);
     }
+
+    ++SDL_joysticks_locked;
 }
 
 void
 SDL_UnlockJoysticks(void)
 {
+    --SDL_joysticks_locked;
+
     if (SDL_joystick_lock) {
         SDL_UnlockMutex(SDL_joystick_lock);
+
+        /* The last unlock after joysticks are uninitialized will cleanup the mutex,
+         * allowing applications to lock joysticks while reinitializing the system.
+         */
+        if (!SDL_joysticks_locked &&
+            !SDL_joysticks_initialized) {
+            SDL_DestroyMutex(SDL_joystick_lock);
+            SDL_joystick_lock = NULL;
+        }
     }
 }
 
+static void
+SDL_AssertJoysticksLocked(void)
+{
+    SDL_assert(SDL_joysticks_locked > 0);
+}
+
+SDL_bool
+SDL_JoysticksQuitting(void)
+{
+    return SDL_joysticks_quitting;
+}
+
+/*
+ * Get the driver and device index for an API device index
+ * This should be called while the joystick lock is held, to prevent another thread from updating the list
+ */
+static SDL_bool
+SDL_GetDriverAndJoystickIndex(int device_index, SDL_JoystickDriver **driver, int *driver_index)
+{
+    int i, num_joysticks, total_joysticks = 0;
+
+    SDL_AssertJoysticksLocked();
+
+    if (device_index >= 0) {
+        for (i = 0; i < SDL_arraysize(SDL_joystick_drivers); ++i) {
+            num_joysticks = SDL_joystick_drivers[i]->GetCount();
+            if (device_index < num_joysticks) {
+                *driver = SDL_joystick_drivers[i];
+                *driver_index = device_index;
+                return SDL_TRUE;
+            }
+            device_index -= num_joysticks;
+            total_joysticks += num_joysticks;
+        }
+    }
+
+    SDL_SetError("There are %d joysticks available", total_joysticks);
+    return SDL_FALSE;
+}
+
 static int
 SDL_FindFreePlayerIndex()
 {
     int player_index;
 
+    SDL_AssertJoysticksLocked();
+
     for (player_index = 0; player_index < SDL_joystick_player_count; ++player_index) {
         if (SDL_joystick_players[player_index] == -1) {
             return player_index;
@@ -148,6 +211,8 @@ SDL_GetPlayerIndexForJoystickID(SDL_JoystickID instance_id)
 {
     int player_index;
 
+    SDL_AssertJoysticksLocked();
+
     for (player_index = 0; player_index < SDL_joystick_player_count; ++player_index) {
         if (instance_id == SDL_joystick_players[player_index]) {
             break;
@@ -162,6 +227,8 @@ SDL_GetPlayerIndexForJoystickID(SDL_JoystickID instance_id)
 static SDL_JoystickID
 SDL_GetJoystickIDForPlayerIndex(int player_index)
 {
+    SDL_AssertJoysticksLocked();
+
     if (player_index < 0 || player_index >= SDL_joystick_player_count) {
         return -1;
     }
@@ -176,6 +243,8 @@ SDL_SetJoystickIDForPlayerIndex(int player_index, SDL_JoystickID instance_id)
     int device_index;
     int existing_player_index;
 
+    SDL_AssertJoysticksLocked();
+
     if (player_index >= SDL_joystick_player_count) {
         SDL_JoystickID *new_players = (SDL_JoystickID *)SDL_realloc(SDL_joystick_players, (player_index + 1)*sizeof(*SDL_joystick_players));
         if (!new_players) {
@@ -229,29 +298,39 @@ SDL_JoystickInit(void)
 {
     int i, status;
 
-    SDL_GameControllerInitMappings();
-
     /* Create the joystick list lock */
     if (!SDL_joystick_lock) {
         SDL_joystick_lock = SDL_CreateMutex();
     }
 
-    /* See if we should allow joystick events while in the background */
-    SDL_AddHintCallback(SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS,
-                        SDL_JoystickAllowBackgroundEventsChanged, NULL);
-
 #if !SDL_EVENTS_DISABLED
     if (SDL_InitSubSystem(SDL_INIT_EVENTS) < 0) {
         return -1;
     }
 #endif /* !SDL_EVENTS_DISABLED */
 
+    SDL_LockJoysticks();
+
+    SDL_joysticks_initialized = SDL_TRUE;
+
+    SDL_GameControllerInitMappings();
+
+    /* See if we should allow joystick events while in the background */
+    SDL_AddHintCallback(SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS,
+                        SDL_JoystickAllowBackgroundEventsChanged, NULL);
+
     status = -1;
     for (i = 0; i < SDL_arraysize(SDL_joystick_drivers); ++i) {
         if (SDL_joystick_drivers[i]->Init() >= 0) {
             status = 0;
         }
     }
+    SDL_UnlockJoysticks();
+
+    if (status < 0) {
+        SDL_JoystickQuit();
+    }
+
     return status;
 }
 
@@ -279,32 +358,6 @@ SDL_JoystickID SDL_GetNextJoystickInstanceID()
     return SDL_AtomicIncRef(&SDL_next_joystick_instance_id);
 }
 
-/*
- * Get the driver and device index for an API device index
- * This should be called while the joystick lock is held, to prevent another thread from updating the list
- */
-SDL_bool
-SDL_GetDriverAndJoystickIndex(int device_index, SDL_JoystickDriver **driver, int *driver_index)
-{
-    int i, num_joysticks, total_joysticks = 0;
-
-    if (device_index >= 0) {
-        for (i = 0; i < SDL_arraysize(SDL_joystick_drivers); ++i) {
-            num_joysticks = SDL_joystick_drivers[i]->GetCount();
-            if (device_index < num_joysticks) {
-                *driver = SDL_joystick_drivers[i];
-                *driver_index = device_index;
-                return SDL_TRUE;
-            }
-            device_index -= num_joysticks;
-            total_joysticks += num_joysticks;
-        }
-    }
-
-    SDL_SetError("There are %d joysticks available", total_joysticks);
-    return SDL_FALSE;
-}
-
 /*
  * Get the implementation dependent name of a joystick
  */
@@ -1138,11 +1191,6 @@ SDL_JoystickClose(SDL_Joystick *joystick)
         return;
     }
 
-    if (SDL_updating_joystick) {
-        SDL_UnlockJoysticks();
-        return;
-    }
-
     if (joystick->rumble_expiration) {
         SDL_JoystickRumble(joystick, 0, 0, 0);
     }
@@ -1194,13 +1242,9 @@ SDL_JoystickQuit(void)
 {
     int i;
 
-    /* Make sure we're not getting called in the middle of updating joysticks */
     SDL_LockJoysticks();
-    while (SDL_updating_joystick) {
-        SDL_UnlockJoysticks();
-        SDL_Delay(1);
-        SDL_LockJoysticks();
-    }
+
+    SDL_joysticks_quitting = SDL_TRUE;
 
     /* Stop the event polling */
     while (SDL_joysticks) {
@@ -1218,7 +1262,6 @@ SDL_JoystickQuit(void)
         SDL_joystick_players = NULL;
         SDL_joystick_player_count = 0;
     }
-    SDL_UnlockJoysticks();
 
 #if !SDL_EVENTS_DISABLED
     SDL_QuitSubSystem(SDL_INIT_EVENTS);
@@ -1227,13 +1270,12 @@ SDL_JoystickQuit(void)
     SDL_DelHintCallback(SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS,
                         SDL_JoystickAllowBackgroundEventsChanged, NULL);
 
-    if (SDL_joystick_lock) {
-        SDL_mutex *mutex = SDL_joystick_lock;
-        SDL_joystick_lock = NULL;
-        SDL_DestroyMutex(mutex);
-    }
-
     SDL_GameControllerQuitMappings();
+
+    SDL_joysticks_quitting = SDL_FALSE;
+    SDL_joysticks_initialized = SDL_FALSE;
+
+    SDL_UnlockJoysticks();
 }
 
 
@@ -1301,7 +1343,12 @@ void SDL_PrivateJoystickAdded(SDL_JoystickID device_instance)
         return;
     }
 
-    SDL_LockJoysticks();
+    SDL_AssertJoysticksLocked();
+
+    if (SDL_JoysticksQuitting()) {
+        return;
+    }
+
     if (SDL_GetDriverAndJoystickIndex(device_index, &driver, &driver_device_index)) {
         player_index = driver->GetDevicePlayerIndex(driver_device_index);
     }
@@ -1311,7 +1358,6 @@ void SDL_PrivateJoystickAdded(SDL_JoystickID device_instance)
     if (player_index >= 0) {
         SDL_SetJoystickIDForPlayerIndex(player_index, device_instance);
     }
-    SDL_UnlockJoysticks();
 
 #if !SDL_EVENTS_DISABLED
     {
@@ -1425,6 +1471,8 @@ void SDL_PrivateJoystickRemoved(SDL_JoystickID device_instance)
     SDL_Event event;
 #endif
 
+    SDL_AssertJoysticksLocked();
+
     /* Find this joystick... */
     device_index = 0;
     for (joystick = SDL_joysticks; joystick; joystick = joystick->next) {
@@ -1450,12 +1498,10 @@ void SDL_PrivateJoystickRemoved(SDL_JoystickID device_instance)
     UpdateEventsForDeviceRemoval(device_index, SDL_CONTROLLERDEVICEADDED);
 #endif /* !SDL_EVENTS_DISABLED */
 
-    SDL_LockJoysticks();
     player_index = SDL_GetPlayerIndexForJoystickID(device_instance);
     if (player_index >= 0) {
         SDL_joystick_players[player_index] = -1;
     }
-    SDL_UnlockJoysticks();
 }
 
 int
@@ -1464,6 +1510,8 @@ SDL_PrivateJoystickAxis(SDL_Joystick *joystick, Uint8 axis, Sint16 value)
     int posted;
     SDL_JoystickAxisInfo *info;
 
+    SDL_AssertJoysticksLocked();
+
     /* Make sure we're not getting garbage or duplicate events */
     if (axis >= joystick->naxes) {
         return 0;
@@ -1527,6 +1575,8 @@ SDL_PrivateJoystickHat(SDL_Joystick *joystick, Uint8 hat, Uint8 value)
 {
     int posted;
 
+    SDL_AssertJoysticksLocked();
+
     /* Make sure we're not getting garbage or duplicate events */
     if (hat >= joystick->nhats) {
         return 0;
@@ -1568,6 +1618,8 @@ SDL_PrivateJoystickBall(SDL_Joystick *joystick, Uint8 ball,
 {
     int posted;
 
+    SDL_AssertJoysticksLocked();
+
     /* Make sure we're not getting garbage events */
     if (ball >= joystick->nballs) {
         return 0;
@@ -1618,6 +1670,8 @@ SDL_PrivateJoystickButton(SDL_Joystick *joystick, Uint8 button, Uint8 state)
     }
 #endif /* !SDL_EVENTS_DISABLED */
 
+    SDL_AssertJoysticksLocked();
+
     /* Make sure we're not getting garbage or duplicate events */
     if (button >= joystick->nbuttons) {
         return 0;
@@ -1654,7 +1708,7 @@ void
 SDL_JoystickUpdate(void)
 {
     int i;
-    SDL_Joystick *joystick, *next;
+    SDL_Joystick *joystick;
 
     if (!SDL_WasInit(SDL_INIT_JOYSTICK)) {
         return;
@@ -1662,17 +1716,6 @@ SDL_JoystickUpdate(void)
 
     SDL_LockJoysticks();
 
-    if (SDL_updating_joystick) {
-        /* The joysticks are already being updated */
-        SDL_UnlockJoysticks();
-        return;
-    }
-
-    SDL_updating_joystick = SDL_TRUE;
-
-    /* Make sure the list is unlocked while dispatching events to prevent application deadlocks */
-    SDL_UnlockJoysticks();
-
 #ifdef SDL_JOYSTICK_HIDAPI
     /* Special function for HIDAPI devices, as a single device can provide multiple SDL_Joysticks */
     HIDAPI_UpdateDevices();
@@ -1680,11 +1723,6 @@ SDL_JoystickUpdate(void)
 
     for (joystick = SDL_joysticks; joystick; joystick = joystick->next) {
         if (joystick->attached) {
-            /* This driver should always be != NULL, but seeing a crash in the wild...? */
-            if (!joystick->driver) {
-                continue;  /* nothing we can do, and other things use joystick->driver below here. */
-            }
-
             joystick->driver->Update(joystick);
 
             if (joystick->delayed_guide_button) {
@@ -1692,36 +1730,14 @@ SDL_JoystickUpdate(void)
             }
         }
 
-        if (joystick->rumble_expiration) {
-            SDL_LockJoysticks();
-            /* Double check now that the lock is held */
-            if (joystick->rumble_expiration &&
-                SDL_TICKS_PASSED(SDL_GetTicks(), joystick->rumble_expiration)) {
-                SDL_JoystickRumble(joystick, 0, 0, 0);
-            }
-            SDL_UnlockJoysticks();
+        if (joystick->rumble_expiration &&
+            SDL_TICKS_PASSED(SDL_GetTicks(), joystick->rumble_expiration)) {
+            SDL_JoystickRumble(joystick, 0, 0, 0);
         }
 
-        if (joystick->trigger_rumble_expiration) {
-            SDL_LockJoysticks();
-            /* Double check now that the lock is held */
-            if (joystick->trigger_rumble_expiration &&
-                SDL_TICKS_PASSED(SDL_GetTicks(), joystick->trigger_rumble_expiration)) {
-                SDL_JoystickRumbleTriggers(joystick, 0, 0, 0);
-            }
-            SDL_UnlockJoysticks();
-        }
-    }
-
-    SDL_LockJoysticks();
-
-    SDL_updating_joystick = SDL_FALSE;
-
-    /* If any joysticks were closed while updating, free them here */
-    for (joystick = SDL_joysticks; joystick; joystick = next) {
-        next = joystick->next;
-        if (joystick->ref_count <= 0) {
-            SDL_JoystickClose(joystick);
+        if (joystick->trigger_rumble_expiration &&
+            SDL_TICKS_PASSED(SDL_GetTicks(), joystick->trigger_rumble_expiration)) {
+            SDL_JoystickRumbleTriggers(joystick, 0, 0, 0);
         }
     }
 

src/joystick/SDL_joystick_c.h

@@ -39,6 +39,12 @@ struct _SDL_JoystickDriver;
 extern int SDL_JoystickInit(void);
 extern void SDL_JoystickQuit(void);
 
+/* Return whether the joystick system is currently initialized */
+extern SDL_bool SDL_JoysticksInitialized(void);
+
+/* Return whether the joystick system is shutting down */
+extern SDL_bool SDL_JoysticksQuitting(void);
+
 /* Function to get the next available joystick instance ID */
 extern SDL_JoystickID SDL_GetNextJoystickInstanceID(void);
 
@@ -48,9 +54,6 @@ extern void SDL_GameControllerQuitMappings(void);
 extern int SDL_GameControllerInit(void);
 extern void SDL_GameControllerQuit(void);
 
-/* Function to get the joystick driver and device index for an API device index */
-extern SDL_bool SDL_GetDriverAndJoystickIndex(int device_index, struct _SDL_JoystickDriver **driver, int *driver_index);
-
 /* Function to return the device index for a joystick ID, or -1 if not found */
 extern int SDL_JoystickGetDeviceIndexFromInstanceID(SDL_JoystickID instance_id);
 

src/joystick/windows/SDL_rawinputjoystick.c

@@ -36,7 +36,6 @@
 #include "SDL_endian.h"
 #include "SDL_events.h"
 #include "SDL_hints.h"
-#include "SDL_mutex.h"
 #include "SDL_timer.h"
 #include "../usb_ids.h"
 #include "../SDL_sysjoystick.h"
@@ -96,7 +95,6 @@ typedef struct WindowsGamingInputGamepadState WindowsGamingInputGamepadState;
 
 static SDL_bool SDL_RAWINPUT_inited = SDL_FALSE;
 static int SDL_RAWINPUT_numjoysticks = 0;
-static SDL_mutex *SDL_RAWINPUT_mutex = NULL;
 
 static void RAWINPUT_JoystickClose(SDL_Joystick *joystick);
 
@@ -865,7 +863,6 @@ RAWINPUT_JoystickInit(void)
         return -1;
     }
 
-    SDL_RAWINPUT_mutex = SDL_CreateMutex();
     SDL_RAWINPUT_inited = SDL_TRUE;
 
     if ((GetRawInputDeviceList(NULL, &device_count, sizeof(RAWINPUTDEVICELIST)) != -1) && device_count > 0) {
@@ -1927,7 +1924,7 @@ RAWINPUT_WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
     LRESULT result = -1;
 
     if (SDL_RAWINPUT_inited) {
-        SDL_LockMutex(SDL_RAWINPUT_mutex);
+        SDL_LockJoysticks();
 
         switch (msg) {
             case WM_INPUT_DEVICE_CHANGE:
@@ -1973,7 +1970,7 @@ RAWINPUT_WindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
             break;
         }
 
-        SDL_UnlockMutex(SDL_RAWINPUT_mutex);
+        SDL_UnlockJoysticks();
     }
 
     if (result >= 0) {
@@ -1998,9 +1995,6 @@ RAWINPUT_JoystickQuit(void)
     SDL_RAWINPUT_numjoysticks = 0;
 
     SDL_RAWINPUT_inited = SDL_FALSE;
-
-    SDL_DestroyMutex(SDL_RAWINPUT_mutex);
-    SDL_RAWINPUT_mutex = NULL;
 }
 
 static SDL_bool

src/joystick/windows/SDL_windows_gaming_input.c

@@ -253,9 +253,11 @@ static HRESULT STDMETHODCALLTYPE IEventHandler_CRawGameControllerVtbl_InvokeAdde
     HRESULT hr;
     __x_ABI_CWindows_CGaming_CInput_CIRawGameController *controller = NULL;
 
-    /* We can get delayed calls to InvokeAdded() after WGI_JoystickQuit(). Do nothing if WGI is deinitialized.
-     * FIXME: Can we tell if WGI has been quit and reinitialized prior to a delayed callback? */
-    if (wgi.statics == NULL) {
+    SDL_LockJoysticks();
+
+    /* We can get delayed calls to InvokeAdded() after WGI_JoystickQuit() */
+    if (SDL_JoysticksQuitting() || !SDL_JoysticksInitialized()) {
+        SDL_UnlockJoysticks();
         return S_OK;
     }
 
@@ -388,6 +390,9 @@ static HRESULT STDMETHODCALLTYPE IEventHandler_CRawGameControllerVtbl_InvokeAdde
 
         __x_ABI_CWindows_CGaming_CInput_CIRawGameController_Release(controller);
     }
+
+    SDL_UnlockJoysticks();
+
     return S_OK;
 }