From 40bd4feedcca779a4600743c27a786b436edc8f7 Mon Sep 17 00:00:00 2001 From: Sam Lantinga 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 d8573c4e8..7c0463517 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 c855f36d9..da8773739 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 d77f8d007..51112a68b 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 42490ea8b..26d753cff 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 86f733a8d..acb7ae747 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; }