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

@@ -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;

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);

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) {

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);
 }