From 40bd4feedcca779a4600743c27a786b436edc8f7 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Tue, 30 Aug 2022 11:42:13 -0700
Subject: [PATCH] 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 | 5 +
src/joystick/SDL_joystick.c | 218 ++++++++++--------
src/joystick/SDL_joystick_c.h | 9 +-
src/joystick/windows/SDL_rawinputjoystick.c | 10 +-
.../windows/SDL_windows_gaming_input.c | 11 +-
5 files changed, 138 insertions(+), 115 deletions(-)
diff --git a/include/SDL_joystick.h b/include/SDL_joystick.h
index d8573c4e8f5..7c0463517a7 100644
--- a/include/SDL_joystick.h
+++ b/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);
diff --git a/src/joystick/SDL_joystick.c b/src/joystick/SDL_joystick.c
index c855f36d9e3..da8773739e5 100644
--- a/src/joystick/SDL_joystick.c
+++ b/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);
}
}
diff --git a/src/joystick/SDL_joystick_c.h b/src/joystick/SDL_joystick_c.h
index d77f8d007f2..51112a68b8b 100644
--- a/src/joystick/SDL_joystick_c.h
+++ b/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);
diff --git a/src/joystick/windows/SDL_rawinputjoystick.c b/src/joystick/windows/SDL_rawinputjoystick.c
index 42490ea8b30..26d753cff70 100644
--- a/src/joystick/windows/SDL_rawinputjoystick.c
+++ b/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
diff --git a/src/joystick/windows/SDL_windows_gaming_input.c b/src/joystick/windows/SDL_windows_gaming_input.c
index 86f733a8d70..acb7ae747de 100644
--- a/src/joystick/windows/SDL_windows_gaming_input.c
+++ b/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;
}