Use recursive mutexes to fix deadlocks in loader

.github/workflows/build.yml

@@ -202,6 +202,32 @@ jobs:
             - run: ctest --parallel --output-on-failure --test-dir build/
             - run: cmake --install build --prefix /tmp
 
+    linux-threading:
+        needs: codegen
+        runs-on: ubuntu-24.04-arm
+        timeout-minutes: 30
+        steps:
+            - uses: actions/checkout@v6.0.2
+            - uses: actions/setup-python@v6
+              with:
+                python-version: '3.11'
+            - name: Test CMake min
+              uses: lukka/get-cmake@latest
+              with:
+                  cmakeVersion: 3.22.1
+            - run: sudo apt update
+            - run: sudo apt install --yes --no-install-recommends libwayland-dev libxrandr-dev
+            - run: |
+                  cmake -S. -B build \
+                  -D CMAKE_BUILD_TYPE=Debug \
+                  -D BUILD_TESTS=ON \
+                  -D UPDATE_DEPS=ON \
+                  -D LOADER_ENABLE_THREAD_SANITIZER=ON \
+                  -D BUILD_WERROR=ON
+            - run: cmake --build build
+            - run: ctest --parallel --output-on-failure --test-dir build/
+            - run: cmake --install build --prefix /tmp
+
     windows_vs:
         # windows is 2x expensive to run on GitHub machines, so only run if we know something else simple passed as well
         needs: linux-no-asm

loader/CMakeLists.txt

@@ -432,6 +432,11 @@ else()
     if (LOADER_ENABLE_ADDRESS_SANITIZER)
         target_compile_options(vulkan PUBLIC -fsanitize=address,undefined)
         target_link_options(vulkan PUBLIC -fsanitize=address,undefined)
+        # workaround regression in macOS SDK 15 when address sanitizer is enabled in debug mode
+        # https://developer.apple.com/forums/thread/774632
+        if (APPLE)
+            target_compile_options(vulkan PRIVATE -mllvm -asan-globals=0)
+        endif()
     endif()
     if (LOADER_ENABLE_THREAD_SANITIZER)
         target_compile_options(vulkan PUBLIC -fsanitize=thread)

loader/cJSON.c

@@ -823,8 +823,8 @@ loader_cJSON_ParseWithLengthOpts(const VkAllocationCallbacks *pAllocator, const
     cJSON *item = NULL;
 
     /* reset error position */
-    global_error.json = NULL;
-    global_error.position = 0;
+    // global_error.json = NULL;
+    // global_error.position = 0;
 
     if (value == NULL || 0 == buffer_length) {
         goto fail;
@@ -880,7 +880,7 @@ loader_cJSON_ParseWithLengthOpts(const VkAllocationCallbacks *pAllocator, const
             *return_parse_end = (const char *)local_error.json + local_error.position;
         }
 
-        global_error = local_error;
+        // global_error = local_error;
     }
 
     return NULL;

loader/loader.c

@@ -94,7 +94,6 @@ struct activated_layer_info {
 // additionally CreateDevice and DestroyDevice needs to be locked
 loader_platform_thread_mutex loader_lock;
 loader_platform_thread_mutex loader_preload_icd_lock;
-loader_platform_thread_mutex loader_global_instance_list_lock;
 
 // A list of ICDs that gets initialized when the loader does its global initialization. This list should never be used by anything
 // other than EnumerateInstanceExtensionProperties(), vkDestroyInstance, and loader_release(). This list does not change
@@ -1631,7 +1630,7 @@ struct loader_icd_term *loader_get_icd_and_device(const void *device, struct loa
         *found_dev = NULL;
         return NULL;
     }
-    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
+    loader_platform_thread_lock_mutex(&loader_lock);
     *found_dev = NULL;
 
     for (struct loader_instance *inst = loader.instances; inst; inst = inst->next) {
@@ -1641,13 +1640,13 @@ struct loader_icd_term *loader_get_icd_and_device(const void *device, struct loa
                 if (loader_get_dispatch(dev->icd_device) == dispatch_table_device ||
                     (dev->chain_device != VK_NULL_HANDLE && loader_get_dispatch(dev->chain_device) == dispatch_table_device)) {
                     *found_dev = dev;
-                    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
+                    loader_platform_thread_unlock_mutex(&loader_lock);
                     return icd_term;
                 }
             }
         }
     }
-    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
+    loader_platform_thread_unlock_mutex(&loader_lock);
     return NULL;
 }
 
@@ -2299,7 +2298,6 @@ BOOL __stdcall loader_initialize(PINIT_ONCE InitOnce, PVOID Parameter, PVOID *Co
 void loader_initialize(void) {
     loader_platform_thread_create_mutex(&loader_lock);
     loader_platform_thread_create_mutex(&loader_preload_icd_lock);
-    loader_platform_thread_create_mutex(&loader_global_instance_list_lock);
     init_global_loader_settings();
 #endif
 
@@ -2341,7 +2339,6 @@ void loader_release(void) {
     teardown_global_loader_settings();
     loader_platform_thread_delete_mutex(&loader_lock);
     loader_platform_thread_delete_mutex(&loader_preload_icd_lock);
-    loader_platform_thread_delete_mutex(&loader_global_instance_list_lock);
 }
 
 // Preload the ICD libraries that are likely to be needed so we don't repeatedly load/unload them later
@@ -4656,14 +4653,14 @@ struct loader_instance *loader_get_instance(const VkInstance instance) {
         return NULL;
     } else {
         disp = loader_get_instance_layer_dispatch(instance);
-        loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
+        loader_platform_thread_lock_mutex(&loader_lock);
         for (struct loader_instance *inst = loader.instances; inst; inst = inst->next) {
             if (&inst->disp->layer_inst_disp == disp) {
                 ptr_instance = inst;
                 break;
             }
         }
-        loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
+        loader_platform_thread_unlock_mutex(&loader_lock);
     }
     return ptr_instance;
 }
@@ -6153,7 +6150,6 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
 
     // Remove this instance from the list of instances:
     struct loader_instance *prev = NULL;
-    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
     struct loader_instance *next = loader.instances;
     while (next != NULL) {
         if (next == ptr_instance) {
@@ -6167,7 +6163,6 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
         prev = next;
         next = next->next;
     }
-    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
 
     struct loader_icd_term *icd_terms = ptr_instance->icd_terms;
     while (NULL != icd_terms) {

loader/loader.h

@@ -85,7 +85,6 @@ static inline void loader_init_dispatch(void *obj, const void *data) {
 extern struct loader_struct loader;
 extern loader_platform_thread_mutex loader_lock;
 extern loader_platform_thread_mutex loader_preload_icd_lock;
-extern loader_platform_thread_mutex loader_global_instance_list_lock;
 
 bool compare_vk_extension_properties(const VkExtensionProperties *op1, const VkExtensionProperties *op2);
 

loader/loader_windows.c

@@ -102,7 +102,6 @@ BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, LPVOID reserved) {
             // Only initialize necessary sync primitives
             loader_platform_thread_create_mutex(&loader_lock);
             loader_platform_thread_create_mutex(&loader_preload_icd_lock);
-            loader_platform_thread_create_mutex(&loader_global_instance_list_lock);
             init_global_loader_settings();
             break;
         case DLL_PROCESS_DETACH:

loader/trampoline.c

@@ -673,10 +673,8 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateInstance(const VkInstanceCr
     }
     memcpy(&ptr_instance->disp->layer_inst_disp, &instance_disp, sizeof(instance_disp));
 
-    loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
     ptr_instance->next = loader.instances;
     loader.instances = ptr_instance;
-    loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
 
     // Activate any layers on instance chain
     res = loader_enable_instance_layers(ptr_instance, &ici, &ptr_instance->instance_layer_list, &layer_filters);
@@ -714,12 +712,10 @@ LOADER_EXPORT VKAPI_ATTR VkResult VKAPI_CALL vkCreateInstance(const VkInstanceCr
 
     if (NULL != ptr_instance) {
         if (res != VK_SUCCESS) {
-            loader_platform_thread_lock_mutex(&loader_global_instance_list_lock);
             // error path, should clean everything up
             if (loader.instances == ptr_instance) {
                 loader.instances = ptr_instance->next;
             }
-            loader_platform_thread_unlock_mutex(&loader_global_instance_list_lock);
 
             free_loader_settings(ptr_instance, &ptr_instance->settings);
 

loader/vk_loader_platform.h

@@ -441,7 +441,12 @@ static inline const char *loader_platform_get_proc_address_error(const char *nam
 }
 
 // Thread mutex:
-static inline void loader_platform_thread_create_mutex(loader_platform_thread_mutex *pMutex) { pthread_mutex_init(pMutex, NULL); }
+static inline void loader_platform_thread_create_mutex(loader_platform_thread_mutex *pMutex) {
+    pthread_mutexattr_t attr;
+    pthread_mutexattr_init(&attr);
+    pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE);
+    pthread_mutex_init(pMutex, &attr);
+}
 static inline void loader_platform_thread_lock_mutex(loader_platform_thread_mutex *pMutex) { pthread_mutex_lock(pMutex); }
 static inline void loader_platform_thread_unlock_mutex(loader_platform_thread_mutex *pMutex) { pthread_mutex_unlock(pMutex); }
 static inline void loader_platform_thread_delete_mutex(loader_platform_thread_mutex *pMutex) { pthread_mutex_destroy(pMutex); }

tests/CMakeLists.txt

@@ -19,6 +19,8 @@ enable_language(CXX) # Tests use C++
 set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)
+set(CMAKE_CXX_VISIBILITY_PRESET "hidden")
+
 
 # Make sure tests uses the dynamic runtime instead
 set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL")
@@ -137,9 +139,11 @@ endif()
 if(NOT CMAKE_CROSSCOMPILING)
     gtest_discover_tests(test_regression PROPERTIES DISCOVERY_TIMEOUT 100)
     gtest_discover_tests(test_fuzzing PROPERTIES DISCOVERY_TIMEOUT 100)
+    gtest_discover_tests(test_threading PROPERTIES DISCOVERY_TIMEOUT 100)
 else()
     gtest_add_tests(TARGET test_regression)
     gtest_add_tests(TARGET test_fuzzing)
+    gtest_add_tests(TARGET test_threading)
 endif()
 
 # When APPLE_STATIC_LOADER is ON installation is disabled