From 43a61fec9120ea218bbb48deff84de506756fc03 Mon Sep 17 00:00:00 2001
From: Sam Lantinga <[EMAIL REDACTED]>
Date: Thu, 12 Dec 2024 13:15:05 -0800
Subject: [PATCH] Removed external hashtable locking functions
Read-write locks are not recursive and can't be upgraded from one type to another, so it's not safe to lock the hash table and then call functions that operate on it. If you really want this functionality, we'd need to create unlocked versions of the hashtable functions and you would call those once you've taken a lock on the hashtable, and we'd have to assert that the operations you're doing are compatible with the type of lock you've taken.
All of that complicates working with hashtables, so if you need that type of access, you should probably just use external locking.
---
src/SDL_hashtable.c | 22 ----------------
src/SDL_hashtable.h | 7 ++----
src/camera/SDL_camera.c | 56 +++++++++++++++++++++++++++--------------
src/stdlib/SDL_getenv.c | 42 +++++++++++++++++++++++--------
4 files changed, 71 insertions(+), 56 deletions(-)
diff --git a/src/SDL_hashtable.c b/src/SDL_hashtable.c
index 203607a0865b3..192121a36c45b 100644
--- a/src/SDL_hashtable.c
+++ b/src/SDL_hashtable.c
@@ -493,28 +493,6 @@ bool SDL_HashTableEmpty(SDL_HashTable *table)
return !(table && table->num_occupied_slots);
}
-void SDL_LockHashTable(SDL_HashTable *table, bool for_writing)
-{
- if (!table) {
- return;
- }
-
- if (for_writing) {
- SDL_LockRWLockForWriting(table->lock);
- } else {
- SDL_LockRWLockForReading(table->lock);
- }
-}
-
-void SDL_UnlockHashTable(SDL_HashTable *table)
-{
- if (!table) {
- return;
- }
-
- SDL_UnlockRWLock(table->lock);
-}
-
static void nuke_all(SDL_HashTable *table)
{
void *data = table->data;
diff --git a/src/SDL_hashtable.h b/src/SDL_hashtable.h
index b1998094a6025..5d0f810590019 100644
--- a/src/SDL_hashtable.h
+++ b/src/SDL_hashtable.h
@@ -55,15 +55,12 @@ extern bool SDL_FindInHashTable(const SDL_HashTable *table, const void *key, con
// This function is thread-safe if the hashtable was created with threadsafe = true
extern bool SDL_HashTableEmpty(SDL_HashTable *table);
-extern void SDL_LockHashTable(SDL_HashTable *table, bool for_writing);
-extern void SDL_UnlockHashTable(SDL_HashTable *table);
-
// iterate all values for a specific key. This only makes sense if the hash is stackable. If not-stackable, just use SDL_FindInHashTable().
-// This function is not thread-safe, you should use SDL_LockHashTable() if necessary
+// This function is not thread-safe, you should use external locking if you use this function
extern bool SDL_IterateHashTableKey(const SDL_HashTable *table, const void *key, const void **_value, void **iter);
// iterate all key/value pairs in a hash (stackable hashes can have duplicate keys with multiple values).
-// This function is not thread-safe, you should use SDL_LockHashTable() if necessary
+// This function is not thread-safe, you should use external locking if you use this function
extern bool SDL_IterateHashTable(const SDL_HashTable *table, const void **_key, const void **_value, void **iter);
extern Uint32 SDL_HashPointer(const void *key, void *unused);
diff --git a/src/camera/SDL_camera.c b/src/camera/SDL_camera.c
index 55a0f1a0eb43a..c11f08affc672 100644
--- a/src/camera/SDL_camera.c
+++ b/src/camera/SDL_camera.c
@@ -299,9 +299,11 @@ void UnrefPhysicalCamera(SDL_Camera *device)
{
if (SDL_AtomicDecRef(&device->refcount)) {
// take it out of the device list.
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
if (SDL_RemoveFromHashTable(camera_driver.device_hash, (const void *) (uintptr_t) device->instance_id)) {
SDL_AddAtomicInt(&camera_driver.device_count, -1);
}
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
DestroyPhysicalCamera(device); // ...and nuke it.
}
}
@@ -327,7 +329,9 @@ static SDL_Camera *ObtainPhysicalCamera(SDL_CameraID devid) // !!! FIXME: SDL_A
}
SDL_Camera *device = NULL;
+ SDL_LockRWLockForReading(camera_driver.device_hash_lock);
SDL_FindInHashTable(camera_driver.device_hash, (const void *) (uintptr_t) devid, (const void **) &device);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
if (!device) {
SDL_SetError("Invalid camera device instance ID");
} else {
@@ -420,7 +424,9 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
SDL_assert((specs != NULL) == (num_specs > 0));
SDL_assert(handle != NULL);
+ SDL_LockRWLockForReading(camera_driver.device_hash_lock);
const int shutting_down = SDL_GetAtomicInt(&camera_driver.shutting_down);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
if (shutting_down) {
return NULL; // we're shutting down, don't add any devices that are hotplugged at the last possible moment.
}
@@ -490,6 +496,7 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
SDL_SetAtomicInt(&device->zombie, 0);
RefPhysicalCamera(device);
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
if (SDL_InsertIntoHashTable(camera_driver.device_hash, (const void *) (uintptr_t) device->instance_id, device)) {
SDL_AddAtomicInt(&camera_driver.device_count, 1);
} else {
@@ -507,14 +514,13 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
p->type = SDL_EVENT_CAMERA_DEVICE_ADDED;
p->devid = device->instance_id;
p->next = NULL;
- SDL_LockHashTable(camera_driver.device_hash, true);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = p;
camera_driver.pending_events_tail = p;
- SDL_UnlockHashTable(camera_driver.device_hash);
}
}
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
return device;
}
@@ -568,12 +574,12 @@ void SDL_CameraDisconnected(SDL_Camera *device)
if (first_disconnect) {
if (pending.next) { // NULL if event is disabled or disaster struck.
- SDL_LockHashTable(camera_driver.device_hash, true);
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = pending.next;
camera_driver.pending_events_tail = pending_tail;
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
}
}
}
@@ -606,12 +612,12 @@ void SDL_CameraPermissionOutcome(SDL_Camera *device, bool approved)
ReleaseCamera(device);
if (pending.next) { // NULL if event is disabled or disaster struck.
- SDL_LockHashTable(camera_driver.device_hash, true);
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = pending.next;
camera_driver.pending_events_tail = pending_tail;
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
}
}
@@ -627,15 +633,16 @@ SDL_Camera *SDL_FindPhysicalCameraByCallback(bool (*callback)(SDL_Camera *device
const void *value;
void *iter = NULL;
- SDL_LockHashTable(camera_driver.device_hash, false);
+ SDL_LockRWLockForReading(camera_driver.device_hash_lock);
while (SDL_IterateHashTable(camera_driver.device_hash, &key, &value, &iter)) {
SDL_Camera *device = (SDL_Camera *) value;
if (callback(device, userdata)) { // found it?
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
return device;
}
}
- SDL_UnlockHashTable(camera_driver.device_hash);
+
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
SDL_SetError("Device not found");
return NULL;
@@ -709,7 +716,7 @@ SDL_CameraID *SDL_GetCameras(int *count)
SDL_CameraID *result = NULL;
- SDL_LockHashTable(camera_driver.device_hash, false);
+ SDL_LockRWLockForReading(camera_driver.device_hash_lock);
int num_devices = SDL_GetAtomicInt(&camera_driver.device_count);
result = (SDL_CameraID *) SDL_malloc((num_devices + 1) * sizeof (SDL_CameraID));
if (!result) {
@@ -726,7 +733,7 @@ SDL_CameraID *SDL_GetCameras(int *count)
SDL_assert(devs_seen == num_devices);
result[devs_seen] = 0; // null-terminated.
}
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
*count = num_devices;
@@ -1356,14 +1363,14 @@ void SDL_QuitCamera(void)
return;
}
- SDL_HashTable *device_hash = camera_driver.device_hash;
- SDL_LockHashTable(device_hash, true);
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_SetAtomicInt(&camera_driver.shutting_down, 1);
+ SDL_HashTable *device_hash = camera_driver.device_hash;
camera_driver.device_hash = NULL;
SDL_PendingCameraEvent *pending_events = camera_driver.pending_events.next;
camera_driver.pending_events.next = NULL;
SDL_SetAtomicInt(&camera_driver.device_count, 0);
- SDL_UnlockHashTable(device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
SDL_PendingCameraEvent *pending_next = NULL;
for (SDL_PendingCameraEvent *i = pending_events; i; i = pending_next) {
@@ -1381,6 +1388,7 @@ void SDL_QuitCamera(void)
// Free the driver data
camera_driver.impl.Deinitialize();
+ SDL_DestroyRWLock(camera_driver.device_hash_lock);
SDL_DestroyHashTable(device_hash);
SDL_zero(camera_driver);
@@ -1409,8 +1417,14 @@ bool SDL_CameraInit(const char *driver_name)
SDL_QuitCamera(); // shutdown driver if already running.
}
- SDL_HashTable *device_hash = SDL_CreateHashTable(NULL, 8, HashCameraID, MatchCameraID, NukeCameraHashItem, true, false);
+ SDL_RWLock *device_hash_lock = SDL_CreateRWLock(); // create this early, so if it fails we don't have to tear down the whole camera subsystem.
+ if (!device_hash_lock) {
+ return false;
+ }
+
+ SDL_HashTable *device_hash = SDL_CreateHashTable(NULL, 8, HashCameraID, MatchCameraID, NukeCameraHashItem, false, false);
if (!device_hash) {
+ SDL_DestroyRWLock(device_hash_lock);
return false;
}
@@ -1427,6 +1441,7 @@ bool SDL_CameraInit(const char *driver_name)
const char *driver_attempt = driver_name_copy;
if (!driver_name_copy) {
+ SDL_DestroyRWLock(device_hash_lock);
SDL_DestroyHashTable(device_hash);
return false;
}
@@ -1442,6 +1457,7 @@ bool SDL_CameraInit(const char *driver_name)
tried_to_init = true;
SDL_zero(camera_driver);
camera_driver.pending_events_tail = &camera_driver.pending_events;
+ camera_driver.device_hash_lock = device_hash_lock;
camera_driver.device_hash = device_hash;
if (bootstrap[i]->init(&camera_driver.impl)) {
camera_driver.name = bootstrap[i]->name;
@@ -1465,6 +1481,7 @@ bool SDL_CameraInit(const char *driver_name)
tried_to_init = true;
SDL_zero(camera_driver);
camera_driver.pending_events_tail = &camera_driver.pending_events;
+ camera_driver.device_hash_lock = device_hash_lock;
camera_driver.device_hash = device_hash;
if (bootstrap[i]->init(&camera_driver.impl)) {
camera_driver.name = bootstrap[i]->name;
@@ -1485,6 +1502,7 @@ bool SDL_CameraInit(const char *driver_name)
}
SDL_zero(camera_driver);
+ SDL_DestroyRWLock(device_hash_lock);
SDL_DestroyHashTable(device_hash);
return false; // No driver was available, so fail.
}
@@ -1501,20 +1519,20 @@ bool SDL_CameraInit(const char *driver_name)
// ("UpdateSubsystem" is the same naming that the other things that hook into PumpEvents use.)
void SDL_UpdateCamera(void)
{
- SDL_LockHashTable(camera_driver.device_hash, false);
+ SDL_LockRWLockForReading(camera_driver.device_hash_lock);
SDL_PendingCameraEvent *pending_events = camera_driver.pending_events.next;
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
if (!pending_events) {
return; // nothing to do, check next time.
}
// okay, let's take this whole list of events so we can dump the lock, and new ones can queue up for a later update.
- SDL_LockHashTable(camera_driver.device_hash, true);
+ SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
pending_events = camera_driver.pending_events.next; // in case this changed...
camera_driver.pending_events.next = NULL;
camera_driver.pending_events_tail = &camera_driver.pending_events;
- SDL_UnlockHashTable(camera_driver.device_hash);
+ SDL_UnlockRWLock(camera_driver.device_hash_lock);
SDL_PendingCameraEvent *pending_next = NULL;
for (SDL_PendingCameraEvent *i = pending_events; i; i = pending_next) {
diff --git a/src/stdlib/SDL_getenv.c b/src/stdlib/SDL_getenv.c
index 39af4d6b1af52..81d3bfbe2ab4a 100644
--- a/src/stdlib/SDL_getenv.c
+++ b/src/stdlib/SDL_getenv.c
@@ -53,6 +53,7 @@ static char **environ;
struct SDL_Environment
{
+ SDL_Mutex *lock;
SDL_HashTable *strings;
};
static SDL_Environment *SDL_environment;
@@ -87,12 +88,15 @@ SDL_Environment *SDL_CreateEnvironment(bool populated)
return NULL;
}
- env->strings = SDL_CreateHashTable(NULL, 16, SDL_HashString, SDL_KeyMatchString, SDL_NukeFreeKey, true, false);
+ env->strings = SDL_CreateHashTable(NULL, 16, SDL_HashString, SDL_KeyMatchString, SDL_NukeFreeKey, false, false);
if (!env->strings) {
SDL_free(env);
return NULL;
}
+ // Don't fail if we can't create a mutex (e.g. on a single-thread environment)
+ env->lock = SDL_CreateMutex();
+
if (populated) {
#ifdef SDL_PLATFORM_WINDOWS
LPWCH strings = GetEnvironmentStringsW();
@@ -153,10 +157,15 @@ const char *SDL_GetEnvironmentVariable(SDL_Environment *env, const char *name)
return NULL;
}
- const char *value;
- if (SDL_FindInHashTable(env->strings, name, (const void **)&value)) {
- result = SDL_GetPersistentString(value);
+ SDL_LockMutex(env->lock);
+ {
+ const char *value;
+
+ if (SDL_FindInHashTable(env->strings, name, (const void **)&value)) {
+ result = SDL_GetPersistentString(value);
+ }
}
+ SDL_UnlockMutex(env->lock);
return result;
}
@@ -170,7 +179,7 @@ char **SDL_GetEnvironmentVariables(SDL_Environment *env)
return NULL;
}
- SDL_LockHashTable(env->strings, false);
+ SDL_LockMutex(env->lock);
{
size_t count, length = 0;
void *iter;
@@ -207,7 +216,7 @@ char **SDL_GetEnvironmentVariables(SDL_Environment *env)
}
result[count] = NULL;
}
- SDL_UnlockHashTable(env->strings);
+ SDL_UnlockMutex(env->lock);
return result;
}
@@ -224,7 +233,7 @@ bool SDL_SetEnvironmentVariable(SDL_Environment *env, const char *name, const ch
return SDL_InvalidParamError("value");
}
- SDL_LockHashTable(env->strings, true);
+ SDL_LockMutex(env->lock);
{
const void *existing_value;
bool insert = true;
@@ -249,21 +258,33 @@ bool SDL_SetEnvironmentVariable(SDL_Environment *env, const char *name, const ch
}
}
}
- SDL_UnlockHashTable(env->strings);
+ SDL_UnlockMutex(env->lock);
return result;
}
bool SDL_UnsetEnvironmentVariable(SDL_Environment *env, const char *name)
{
+ bool result = false;
+
if (!env) {
return SDL_InvalidParamError("env");
} else if (!name || *name == '\0' || SDL_strchr(name, '=') != NULL) {
return SDL_InvalidParamError("name");
}
- SDL_RemoveFromHashTable(env->strings, name);
- return true;
+ SDL_LockMutex(env->lock);
+ {
+ const void *value;
+ if (SDL_FindInHashTable(env->strings, name, &value)) {
+ result = SDL_RemoveFromHashTable(env->strings, name);
+ } else {
+ result = true;
+ }
+ }
+ SDL_UnlockMutex(env->lock);
+
+ return result;
}
void SDL_DestroyEnvironment(SDL_Environment *env)
@@ -272,6 +293,7 @@ void SDL_DestroyEnvironment(SDL_Environment *env)
return;
}
+ SDL_DestroyMutex(env->lock);
SDL_DestroyHashTable(env->strings);
SDL_free(env);
}