From 47462287fcb055c1ae2313e4060f8bd1e8a4d2bb Mon Sep 17 00:00:00 2001 From: BlackMark Date: Wed, 24 Jun 2026 01:22:09 +0200 Subject: [PATCH] Synchronize and bound the Vulkan swapchain tracking (g_swaps) g_swaps (vk_hook.cpp and the Vulkan layer) is pushed from the create-swapchain detour and iterated by the present detour, which can run on different game threads (Vulkan external sync is per-object, not global), and cleared on removal from another thread -- all with no mutex. A push_back realloc could dangle the SwapInfo* a concurrent find_swap/present is using. It was also never pruned, so a game that recreates its swapchain each resize grew it without bound and could match a recycled handle's stale images. Add g_swaps_mutex around every access; the present detour now copies the matched swapchain's fields out under the lock and captures without holding it (no GPU submit under the lock, no dangling pointer). Create de-dups by handle and an LRU cap (8) bounds growth -- the active swapchain is the newest, so it's never evicted. Deliberately NOT hooking vkDestroySwapchainKHR: forwarding a destroy incorrectly could break the game, and the de-dup + cap already bound growth and defeat handle recycling. Verified real by inspection (a concurrent-create+present Vulkan race isn't deterministically reproducible in a test); validated by the full mock_game_test Vulkan paths (capture + layer + too-late) staying green. Co-Authored-By: Claude Opus 4.8 --- README.md | 3 -- hook/src/vk_hook.cpp | 59 ++++++++++++++++++++++++++++++++++---- vk_layer/coop_vk_layer.cpp | 51 ++++++++++++++++++++++++++++---- 3 files changed, 98 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 187ee9f..ef90e31 100644 --- a/README.md +++ b/README.md @@ -114,9 +114,6 @@ From an in-depth review pass. Each item is fixed test-first (a failing test, the as its own commit; "verify" items are confirmed real before any change, and dropped if not. Correctness (verify, then fix if real): -- **`vk_hook` / `vk_layer` `g_swaps`** — no synchronization on push/iterate and never pruned on - swapchain destroy (unbounded growth + stale-handle match). Add a guard + a `vkDestroySwapchainKHR` - prune. - **`mkb_hook` raw-input slot reuse** — a 64-slot ring can overwrite an event before the game reads the `WM_INPUT`. Verify; fix if real. - **Non-atomic cross-process diagnostic counters** — `present_calls`, `frames_dropped`, diff --git a/hook/src/vk_hook.cpp b/hook/src/vk_hook.cpp index a11b99f..e60d01c 100644 --- a/hook/src/vk_hook.cpp +++ b/hook/src/vk_hook.cpp @@ -1,7 +1,9 @@ #include "vk_hook.hpp" +#include #include #include +#include #include #include @@ -72,6 +74,16 @@ struct SwapInfo std::vector images; }; std::vector g_swaps; +// g_swaps is pushed from the create-swapchain detour and read from the present detour, which can run +// on different game threads (Vulkan external-sync is per-object, not global), and cleared from the +// worker thread on removal. This mutex serializes all of that -- a push_back realloc must not race a +// concurrent find_swap iteration. We don't hook vkDestroySwapchainKHR (failing to forward a destroy +// could break the game); instead create de-dups by handle and an LRU cap bounds growth, so a game +// that recreates its swapchain on every resize can't grow g_swaps without bound or match a recycled +// handle's stale images. The active swapchain is always the most-recently created, so it's never +// evicted. +std::mutex g_swaps_mutex; +constexpr std::size_t kMaxTrackedSwaps = 8; // engines use 1-3; headroom for transient resize overlap // --- the real vkGetInstanceProcAddr, via the inline hook's trampoline -------- PFN_vkVoidFunction real_gipa(VkInstance inst, const char* name) @@ -79,6 +91,8 @@ PFN_vkVoidFunction real_gipa(VkInstance inst, const char* name) return g_hk_gipa.stdcall(inst, name); } +// Caller must hold g_swaps_mutex; the returned pointer is only valid until the lock is released +// (copy out what you need before unlocking, since another thread can push_back and reallocate). const SwapInfo* find_swap(VkSwapchainKHR sc) { for (const SwapInfo& s : g_swaps) @@ -107,12 +121,30 @@ VKAPI_ATTR VkResult VKAPI_CALL hk_vkQueuePresentKHR(VkQueue queue, const VkPrese if (g_capture_enabled.load(std::memory_order_acquire) && g_device != VK_NULL_HANDLE && pPresentInfo != nullptr && pPresentInfo->swapchainCount == 1) { - const SwapInfo* s = find_swap(pPresentInfo->pSwapchains[0]); - if (s != nullptr && pPresentInfo->pImageIndices[0] < s->images.size()) + // Copy the matched swapchain's fields out under the lock, then capture without holding it (so + // the GPU submit can't block a concurrent create, and the SwapInfo* can't dangle on a realloc). + VkImage image = VK_NULL_HANDLE; + VkFormat fmt = VK_FORMAT_UNDEFINED; + std::uint32_t w = 0, h = 0; + bool matched = false; + { + std::scoped_lock lock(g_swaps_mutex); + const SwapInfo* s = find_swap(pPresentInfo->pSwapchains[0]); + const std::uint32_t idx = pPresentInfo->pImageIndices[0]; + if (s != nullptr && idx < s->images.size()) + { + image = s->images[idx]; + fmt = s->fmt; + w = s->w; + h = s->h; + matched = true; + } + } + if (matched) { VkSemaphore chained = VK_NULL_HANDLE; - if (g_cap.present(s->images[pPresentInfo->pImageIndices[0]], s->fmt, s->w, s->h, - pPresentInfo->pWaitSemaphores, pPresentInfo->waitSemaphoreCount, chained)) + if (g_cap.present(image, fmt, w, h, pPresentInfo->pWaitSemaphores, + pPresentInfo->waitSemaphoreCount, chained)) { // Replace the present's wait with our chained semaphore (our submit consumed the // originals and signals this one), so the present still orders after rendering. @@ -142,7 +174,19 @@ VKAPI_ATTR VkResult VKAPI_CALL hk_vkCreateSwapchainKHR(VkDevice device, const Vk g_get_swapchain_images(device, *out, &n, nullptr); info.images.resize(n); g_get_swapchain_images(device, *out, &n, info.images.data()); - g_swaps.push_back(std::move(info)); + { + std::scoped_lock lock(g_swaps_mutex); + // De-dup a recycled handle value, then bound growth (drop the oldest; the just-created + // active swapchain is newest and stays). + g_swaps.erase(std::remove_if(g_swaps.begin(), g_swaps.end(), + [&](const SwapInfo& e) { return e.sc == info.sc; }), + g_swaps.end()); + g_swaps.push_back(std::move(info)); + if (g_swaps.size() > kMaxTrackedSwaps) + { + g_swaps.erase(g_swaps.begin()); + } + } // (Don't log the VkSwapchainKHR handle: it's a uint64_t on x86, not a pointer.) logf("vk: swapchain %ux%u fmt=%d images=%u", ci->imageExtent.width, ci->imageExtent.height, static_cast(ci->imageFormat), n); @@ -345,7 +389,10 @@ void remove_vk_hooks() g_cap.shutdown(); // joins the reaper, drains the device, frees the read-back resources hook_set_installed(g_id_present, false); - g_swaps.clear(); + { + std::scoped_lock lock(g_swaps_mutex); // drained above, but keep all g_swaps access serialized + g_swaps.clear(); + } g_get_swapchain_images = nullptr; g_device = VK_NULL_HANDLE; g_instance = VK_NULL_HANDLE; diff --git a/vk_layer/coop_vk_layer.cpp b/vk_layer/coop_vk_layer.cpp index 6b9b9bf..92efd15 100644 --- a/vk_layer/coop_vk_layer.cpp +++ b/vk_layer/coop_vk_layer.cpp @@ -15,10 +15,12 @@ // The loader/layer interface structs (VkLayer*CreateInfo, VkNegotiateLayerInterface) live in // vk_layer.h, which Vulkan-Headers doesn't ship, so they're declared here to the stable // loader-interface-version-2 ABI. +#include #include #include #include #include +#include #include #include @@ -132,6 +134,12 @@ struct SwapInfo std::vector images; }; std::vector g_swaps; +// Serialize all g_swaps access: create (push) and present (find) can run on different game threads, +// and DestroyDevice clears it. We don't track swapchain destruction (forwarding a destroy wrong could +// break the game); create de-dups by handle and an LRU cap bounds growth so repeated resize/recreate +// can't grow g_swaps unbounded or match a recycled handle's stale images. +std::mutex g_swaps_mutex; +constexpr std::size_t kMaxTrackedSwaps = 8; bool eq(const char* a, const char* b) { @@ -209,6 +217,8 @@ bool decide_active() return _stricmp(self8, want) == 0; } +// Caller must hold g_swaps_mutex; copy out what you need before unlocking (a concurrent push_back +// can reallocate and dangle the returned pointer). const SwapInfo* find_swap(VkSwapchainKHR sc) { for (const SwapInfo& s : g_swaps) @@ -268,12 +278,28 @@ VKAPI_ATTR VkResult VKAPI_CALL layer_QueuePresentKHR(VkQueue queue, const VkPres } if (g_active && pi != nullptr && pi->swapchainCount == 1) { - const SwapInfo* s = find_swap(pi->pSwapchains[0]); - if (s != nullptr && pi->pImageIndices[0] < s->images.size()) + // Copy the matched swapchain out under the lock, then capture without holding it. + VkImage image = VK_NULL_HANDLE; + VkFormat fmt = VK_FORMAT_UNDEFINED; + std::uint32_t w = 0, h = 0; + bool matched = false; + { + std::scoped_lock lock(g_swaps_mutex); + const SwapInfo* s = find_swap(pi->pSwapchains[0]); + const std::uint32_t idx = pi->pImageIndices[0]; + if (s != nullptr && idx < s->images.size()) + { + image = s->images[idx]; + fmt = s->fmt; + w = s->w; + h = s->h; + matched = true; + } + } + if (matched) { VkSemaphore chained = VK_NULL_HANDLE; - if (g_cap.present(s->images[pi->pImageIndices[0]], s->fmt, s->w, s->h, pi->pWaitSemaphores, - pi->waitSemaphoreCount, chained)) + if (g_cap.present(image, fmt, w, h, pi->pWaitSemaphores, pi->waitSemaphoreCount, chained)) { VkPresentInfoKHR p = *pi; p.waitSemaphoreCount = 1; @@ -305,7 +331,17 @@ VKAPI_ATTR VkResult VKAPI_CALL layer_CreateSwapchainKHR(VkDevice device, const V g_get_swapchain_images(device, *out, &n, nullptr); info.images.resize(n); g_get_swapchain_images(device, *out, &n, info.images.data()); - g_swaps.push_back(std::move(info)); + { + std::scoped_lock lock(g_swaps_mutex); + g_swaps.erase(std::remove_if(g_swaps.begin(), g_swaps.end(), + [&](const SwapInfo& e) { return e.sc == info.sc; }), + g_swaps.end()); + g_swaps.push_back(std::move(info)); + if (g_swaps.size() > kMaxTrackedSwaps) + { + g_swaps.erase(g_swaps.begin()); + } + } } return r; } @@ -435,7 +471,10 @@ VKAPI_ATTR void VKAPI_CALL layer_DestroyDevice(VkDevice device, const VkAllocati if (g_active && device == g_device) { g_cap.shutdown(); // joins the reaper, drains the device, frees the read-back resources - g_swaps.clear(); + { + std::scoped_lock lock(g_swaps_mutex); + g_swaps.clear(); + } g_device = VK_NULL_HANDLE; } destroy(device, a);