take loader_lock in terminator_DestroySurfaceKHR

[aizu-m]

ThreadSanitizer, test_threading built with -D LOADER_ENABLE_THREAD_SANITIZER=ON, a few threads each creating and destroying their own VkSurfaceKHR:

WARNING: ThreadSanitizer: data race
  Read of size 4 by thread T2 (mutexes: write M0):
    #0 loader_get_next_available_entry loader.c:1119
    #1 allocate_icd_surface_struct wsi.c:698
    #2 terminator_CreateMetalSurfaceEXT wsi.c:1703

  Previous write of size 4 by thread T1:
    #0 loader_release_object_from_list loader.c:1152
    #1 terminator_DestroySurfaceKHR wsi.c:362
    #2 vkDestroySurfaceKHR wsi.c:311

M0 is loader_lock. The reader holds it, the writer holds nothing. Found reading back through the surface terminators after #1946 and #1956.

Every terminator_Create*Surface* holds loader_lock across allocate_icd_surface_struct, which reserves a slot in loader_inst->surfaces_list and initialises or doubles each driver's surface_list. Both of those can reallocate. terminator_DestroySurfaceKHR walks and clears the same two lists with no lock, and the vkDestroySurfaceKHR trampoline does not take one either. Only the surface needs external synchronisation, so destroying one surface while another thread creates a different one is legal.

The reported race is on the slot status. The worse case is a resize landing between the destroy path's load of surface_list.list and its store through that pointer, which is a write into freed memory.

loader_release_object_from_list has one call site and this was it. All three loader_get_next_available_entry call sites already hold the lock.

Locking in the terminator rather than the trampoline keeps a layer calling down the chain covered, and puts the lock where the creation side already takes it. loader_lock is recursive, so a caller that already holds it is unaffected. It is taken after the android/ios early-outs, so no return path escapes it.

Threading.SurfaceCreateDestroyLoop reports the trace above on an unpatched loader and is clean after.

[ci-tester-lunarg]

Author aizu-m not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

Author aizu-m not on autobuild list. Waiting for curator authorization before starting CI build.

[charles-lunarg]

This is a very good find. Clear explanation of the problem and the fix. The test also reproduces when testing without the fix, making this a very easy approval.

[ci-tester-lunarg]

CI Vulkan-Loader build queued with queue ID 140757.

[ci-tester-lunarg]

CI Vulkan-Loader build # 3795 running.

[ci-tester-lunarg]

CI Vulkan-Loader build # 3795 passed.