[PATCH 0/1] MR8406: wined3d: Make sure swapchain can't be used after being released.
In wined3d_swapchain_cleanup, before decrementing the reference counts of each of the back/front buffers, we first set their swapchain to NULL, presumably to stop the current under-destruction swapchain from being used through them. But there is an oversight. When we call wined3d_texture_decref on the front_buffer, the back_buffers still have their swapchains pointing to the swapchain being destroyed. In texture_resource_unload, we call context_acquire(device, NULL, 0), note the NULL texture parameter here. When the texture parameter is NULL, wined3d_context_gl_acquire (in turn called by context_acquire) will default to using the first back buffer from the implicit swapchain of "device", which, as previously stated, has not had their swapchain set to NULL yet. From here, we reach wined3d_context_gl_activate with a texture whose swapchain is currently being destroyed. This swapchain is then assigned to "context_gl" here before being freed, leaving a dangling pointer. When this context_gl is acquired again later, we will try to access context_gl->c.swapchain->win_handle, thus complete the use-after-free cycle. This commit makes sure the swapchain pointer of ALL front and back buffers are set to NULL before decrementing their reference counts. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58325 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406
From: Yuxuan Shui <yshui(a)codeweavers.com> In wined3d_swapchain_cleanup, before decrementing the reference counts of each of the back/front buffers, we first set their swapchain to NULL, presumably to stop the current under-destruction swapchain from being used through them. But there is an oversight. When we call wined3d_texture_decref on the front_buffer, the back_buffers still have their swapchains pointing to the swapchain being destroyed. In texture_resource_unload, we call context_acquire(device, NULL, 0), note the NULL texture parameter here. When the texture parameter is NULL, wined3d_context_gl_acquire (in turn called by context_acquire) will default to using the first back buffer from the implicit swapchain of "device", which, as previously stated, has not had their swapchain set to NULL yet. From here, we reach wined3d_context_gl_activate with a texture whose swapchain is currently being destroyed. This swapchain is then assigned to "context_gl" here before being freed, leaving a dangling pointer. When this context_gl is acquired again later, we will try to access context_gl->c.swapchain->win_handle, thus complete the use-after-free cycle. This commit makes sure the swapchain pointer of ALL front and back buffers are set to NULL before decrementing their reference counts. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58325 --- dlls/wined3d/swapchain.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c index 507d6e64bd8..2a852e40771 100644 --- a/dlls/wined3d/swapchain.c +++ b/dlls/wined3d/swapchain.c @@ -34,14 +34,28 @@ void wined3d_swapchain_cleanup(struct wined3d_swapchain *swapchain) TRACE("Destroying swapchain %p.\n", swapchain); + for (i = 0; i < swapchain->device->context_count; i++) + if (swapchain->device->contexts[i]->swapchain == swapchain) + swapchain->device->contexts[i]->swapchain = NULL; + wined3d_swapchain_state_cleanup(&swapchain->state); wined3d_swapchain_set_gamma_ramp(swapchain, 0, &swapchain->orig_gamma); /* Release the swapchain's draw buffers. Make sure swapchain->back_buffers[0] * is the last buffer to be destroyed, FindContext() depends on that. */ if (swapchain->front_buffer) - { wined3d_texture_set_swapchain(swapchain->front_buffer, NULL); + + if (swapchain->back_buffers) + { + i = swapchain->state.desc.backbuffer_count; + + while (i--) + wined3d_texture_set_swapchain(swapchain->back_buffers[i], NULL); + } + + if (swapchain->front_buffer) + { if (wined3d_texture_decref(swapchain->front_buffer)) WARN("Something's still holding the front buffer (%p).\n", swapchain->front_buffer); swapchain->front_buffer = NULL; @@ -53,7 +67,6 @@ void wined3d_swapchain_cleanup(struct wined3d_swapchain *swapchain) while (i--) { - wined3d_texture_set_swapchain(swapchain->back_buffers[i], NULL); if (wined3d_texture_decref(swapchain->back_buffers[i])) WARN("Something's still holding back buffer %u (%p).\n", i, swapchain->back_buffers[i]); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8406
I don't think this explanation makes sense. Before the implicit swapchain destroys its own buffers, it should call wined3d_device_uninit_3d(), which in turn sets device->d3d_initialized to FALSE. At that point texture_resource_unload() will not try to acquire a context. Why isn't this mechanism working? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406#note_108766
On Wed Aug 27 12:01:03 2025 +0000, Elizabeth Figura wrote:
I don't think this explanation makes sense. Before the implicit swapchain destroys its own buffers, it should call wined3d_device_uninit_3d(), which in turn sets device->d3d_initialized to FALSE. At that point texture_resource_unload() will not try to acquire a context. Why isn't this mechanism working? good point, i didn't know about this. i haven't confirmed it yet but there could be a race condition because texture resource unload is done asynchronously. i.e. implicit swapchain destroyed -> texture unload queued -> new swapchain created -> texture unload called.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406#note_113979
On Wed Aug 27 12:01:03 2025 +0000, Yuxuan Shui wrote:
good point, i didn't know about this. i haven't confirmed it yet but there could be a race condition because texture resource unload is done asynchronously. i.e. implicit swapchain destroyed -> texture unload queued -> new swapchain created -> texture unload called. We call wined3d_cs_finish() in adapter_gl_init_3d() before setting d3d_initialized to TRUE, though. [It's missing from the Vulkan adapter, which is concerning, but you did mention GL in the initial description]. I don't think that specific race should be possible.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406#note_114611
On Wed Sep 3 00:21:42 2025 +0000, Elizabeth Figura wrote:
We call wined3d_cs_finish() in adapter_gl_init_3d() before setting d3d_initialized to TRUE, though. [It's missing from the Vulkan adapter, which is concerning, but you did mention GL in the initial description]. I don't think that specific race should be possible. @_@, i think you are right, but then the stack traces we are seeing should not be possible. there must be something i missed, which i'll keep digging.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406#note_117317
This particular report has disappeared from my CI and local testing. Maybe it has been fixed, I'll close for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406#note_128603
This merge request was closed by Yuxuan Shui. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8406
participants (4)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui) -
Yuxuan Shui (＠yshui)