[PATCH] dxgi: Avoid a double free in d3d11_swapchain_init().

[Zhiyi Zhang]

If IDXGISwapChain1_GetContainingOutput() or dxgi_swapchain_set_fullscreen_state() fails,
wined3d_swapchain_decref() is called to free the private store and d3d11_swapchain, which
are freed again later in d3d11_swapchain_init() and upper functions. In this error path,
there are also use-after-frees for a IDXGIFactory and IDXGIDevice. This error could be
produced by creating a swapchain using a device window on the secondary monitor as of now.

Signed-off-by: Zhiyi Zhang <zzhang(a)codeweavers.com>
---
 dlls/dxgi/swapchain.c     |  4 ++--
 dlls/wined3d/device.c     |  9 +++++++++
 dlls/wined3d/swapchain.c  | 14 ++------------
 dlls/wined3d/wined3d.spec |  1 +
 include/wine/wined3d.h    |  1 +
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/dlls/dxgi/swapchain.c b/dlls/dxgi/swapchain.c
index 004bd1457d..7ac5a881c2 100644
--- a/dlls/dxgi/swapchain.c
+++ b/dlls/dxgi/swapchain.c
@@ -898,7 +898,7 @@ HRESULT d3d11_swapchain_init(struct d3d11_swapchain *swapchain, struct dxgi_devi
                 &swapchain->target)))
         {
             WARN("Failed to get target output for fullscreen swapchain, hr %#x.\n", hr);
-            wined3d_swapchain_decref(swapchain->wined3d_swapchain);
+            wined3d_device_destroy_swapchain(device->wined3d_device, swapchain->wined3d_swapchain);
             goto cleanup;
         }
 
@@ -906,7 +906,7 @@ HRESULT d3d11_swapchain_init(struct d3d11_swapchain *swapchain, struct dxgi_devi
         {
             WARN("Failed to set fullscreen state, hr %#x.\n", hr);
             IDXGIOutput_Release(swapchain->target);
-            wined3d_swapchain_decref(swapchain->wined3d_swapchain);
+            wined3d_device_destroy_swapchain(device->wined3d_device, swapchain->wined3d_swapchain);
             goto cleanup;
         }
 
diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c
index 8405643741..f754a2bd49 100644
--- a/dlls/wined3d/device.c
+++ b/dlls/wined3d/device.c
@@ -888,6 +888,15 @@ void wined3d_device_destroy_default_samplers(struct wined3d_device *device, stru
     device->null_sampler = NULL;
 }
 
+void CDECL wined3d_device_destroy_swapchain(struct wined3d_device *device, struct wined3d_swapchain *swapchain)
+{
+    if (device->swapchain_count && device->swapchains[0] == swapchain)
+        wined3d_device_uninit_3d(device);
+
+    wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_DEFAULT);
+    device->adapter->adapter_ops->adapter_destroy_swapchain(swapchain);
+}
+
 HRESULT CDECL wined3d_device_acquire_focus_window(struct wined3d_device *device, HWND window)
 {
     unsigned int screensaver_active;
diff --git a/dlls/wined3d/swapchain.c b/dlls/wined3d/swapchain.c
index 62bab65fcf..7648b99260 100644
--- a/dlls/wined3d/swapchain.c
+++ b/dlls/wined3d/swapchain.c
@@ -129,18 +129,9 @@ ULONG CDECL wined3d_swapchain_decref(struct wined3d_swapchain *swapchain)
 
     if (!refcount)
     {
-        struct wined3d_device *device;
-
         wined3d_mutex_lock();
-
-        device = swapchain->device;
-        if (device->swapchain_count && device->swapchains[0] == swapchain)
-            wined3d_device_uninit_3d(device);
-        wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_DEFAULT);
-
         swapchain->parent_ops->wined3d_object_destroyed(swapchain->parent);
-        swapchain->device->adapter->adapter_ops->adapter_destroy_swapchain(swapchain);
-
+        wined3d_device_destroy_swapchain(swapchain->device, swapchain);
         wined3d_mutex_unlock();
     }
 
@@ -1081,8 +1072,7 @@ HRESULT CDECL wined3d_swapchain_create(struct wined3d_device *device, struct win
         wined3d_mutex_lock();
         if (FAILED(hr = wined3d_device_set_implicit_swapchain(device, object)))
         {
-            wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_DEFAULT);
-            device->adapter->adapter_ops->adapter_destroy_swapchain(object);
+            wined3d_device_destroy_swapchain(device, object);
             wined3d_mutex_unlock();
             return hr;
         }
diff --git a/dlls/wined3d/wined3d.spec b/dlls/wined3d/wined3d.spec
index c8ef442c72..1c198c121b 100644
--- a/dlls/wined3d/wined3d.spec
+++ b/dlls/wined3d/wined3d.spec
@@ -47,6 +47,7 @@
 @ cdecl wined3d_device_copy_uav_counter(ptr ptr long ptr)
 @ cdecl wined3d_device_create(ptr long long ptr long long ptr long ptr ptr)
 @ cdecl wined3d_device_decref(ptr)
+@ cdecl wined3d_device_destroy_swapchain(ptr ptr)
 @ cdecl wined3d_device_dispatch_compute(ptr long long long)
 @ cdecl wined3d_device_dispatch_compute_indirect(ptr ptr long)
 @ cdecl wined3d_device_draw_indexed_primitive(ptr long long)
diff --git a/include/wine/wined3d.h b/include/wine/wined3d.h
index d313c7aec8..1bcae83d94 100644
--- a/include/wine/wined3d.h
+++ b/include/wine/wined3d.h
@@ -2313,6 +2313,7 @@ HRESULT __cdecl wined3d_device_create(struct wined3d *wined3d, unsigned int adap
         const enum wined3d_feature_level *feature_levels, unsigned int feature_level_count,
         struct wined3d_device_parent *device_parent, struct wined3d_device **device);
 ULONG __cdecl wined3d_device_decref(struct wined3d_device *device);
+void __cdecl wined3d_device_destroy_swapchain(struct wined3d_device *device, struct wined3d_swapchain *swapchain);
 void __cdecl wined3d_device_dispatch_compute(struct wined3d_device *device,
         unsigned int group_count_x, unsigned int group_count_y, unsigned int group_count_z);
 void __cdecl wined3d_device_dispatch_compute_indirect(struct wined3d_device *device,
-- 
2.20.1

[Henri Verbeet]

On Tue, 18 Feb 2020 at 05:57, Zhiyi Zhang <zzhang(a)codeweavers.com> wrote:
> +void CDECL wined3d_device_destroy_swapchain(struct wined3d_device *device, struct wined3d_swapchain *swapchain)
> +{
> +    if (device->swapchain_count && device->swapchains[0] == swapchain)
> +        wined3d_device_uninit_3d(device);
> +
> +    wined3d_cs_finish(device->cs, WINED3D_CS_QUEUE_DEFAULT);
> +    device->adapter->adapter_ops->adapter_destroy_swapchain(swapchain);
> +}
> +
> I think the introduction of wined3d_device_destroy_swapchain in the
> public wined3d API is undesirable, but even more undesirable is the
> fact that it does something different from destroying the swapchain
> through wined3d_swapchain_decref().

I think you should simply avoid the issue by moving the code for
handling fullscreen mode in d3d11_swapchain_init() to
dxgi_swapchain_factory_create_swapchain(). If that were somehow not
possible you could check whether parent initialisation is completely
done in d3d11_swapchain_wined3d_object_released(), but I don't think
that's the case.