[PATCH] wined3d: Decrement reference count and take the lock atomically for cached objects.

[Jan Sikorski]

Samplers, blend states, rasterizer states and depth stencil states can
be retrieved from the cache in struct d3d_device even after the
reference count reaches zero, causing memory corruption.

Signed-off-by: Jan Sikorski <jsikorski(a)codeweavers.com>
---
 dlls/wined3d/sampler.c         |  3 +--
 dlls/wined3d/state.c           |  9 +++------
 dlls/wined3d/wined3d_private.h | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/dlls/wined3d/sampler.c b/dlls/wined3d/sampler.c
index bba3165316e..fa648304d07 100644
--- a/dlls/wined3d/sampler.c
+++ b/dlls/wined3d/sampler.c
@@ -35,13 +35,12 @@ ULONG CDECL wined3d_sampler_incref(struct wined3d_sampler *sampler)
 
 ULONG CDECL wined3d_sampler_decref(struct wined3d_sampler *sampler)
 {
-    ULONG refcount = InterlockedDecrement(&sampler->refcount);
+    ULONG refcount = wined3d_atomic_decrement_mutex_lock(&sampler->refcount);
 
     TRACE("%p decreasing refcount to %u.\n", sampler, refcount);
 
     if (!refcount)
     {
-        wined3d_mutex_lock();
         sampler->parent_ops->wined3d_object_destroyed(sampler->parent);
         sampler->device->adapter->adapter_ops->adapter_destroy_sampler(sampler);
         wined3d_mutex_unlock();
diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c
index c76e0c2b604..87ebefea9d0 100644
--- a/dlls/wined3d/state.c
+++ b/dlls/wined3d/state.c
@@ -55,14 +55,13 @@ static void wined3d_blend_state_destroy_object(void *object)
 
 ULONG CDECL wined3d_blend_state_decref(struct wined3d_blend_state *state)
 {
-    ULONG refcount = InterlockedDecrement(&state->refcount);
+    ULONG refcount = wined3d_atomic_decrement_mutex_lock(&state->refcount);
     struct wined3d_device *device = state->device;
 
     TRACE("%p decreasing refcount to %u.\n", state, refcount);
 
     if (!refcount)
     {
-        wined3d_mutex_lock();
         state->parent_ops->wined3d_object_destroyed(state->parent);
         wined3d_cs_destroy_object(device->cs, wined3d_blend_state_destroy_object, state);
         wined3d_mutex_unlock();
@@ -131,14 +130,13 @@ static void wined3d_depth_stencil_state_destroy_object(void *object)
 
 ULONG CDECL wined3d_depth_stencil_state_decref(struct wined3d_depth_stencil_state *state)
 {
-    ULONG refcount = InterlockedDecrement(&state->refcount);
+    ULONG refcount = wined3d_atomic_decrement_mutex_lock(&state->refcount);
     struct wined3d_device *device = state->device;
 
     TRACE("%p decreasing refcount to %u.\n", state, refcount);
 
     if (!refcount)
     {
-        wined3d_mutex_lock();
         state->parent_ops->wined3d_object_destroyed(state->parent);
         wined3d_cs_destroy_object(device->cs, wined3d_depth_stencil_state_destroy_object, state);
         wined3d_mutex_unlock();
@@ -196,14 +194,13 @@ static void wined3d_rasterizer_state_destroy_object(void *object)
 
 ULONG CDECL wined3d_rasterizer_state_decref(struct wined3d_rasterizer_state *state)
 {
-    ULONG refcount = InterlockedDecrement(&state->refcount);
+    ULONG refcount = wined3d_atomic_decrement_mutex_lock(&state->refcount);
     struct wined3d_device *device = state->device;
 
     TRACE("%p decreasing refcount to %u.\n", state, refcount);
 
     if (!refcount)
     {
-        wined3d_mutex_lock();
         state->parent_ops->wined3d_object_destroyed(state->parent);
         wined3d_cs_destroy_object(device->cs, wined3d_rasterizer_state_destroy_object, state);
         wined3d_mutex_unlock();
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h
index 2fce585c6b1..6047defcc2e 100644
--- a/dlls/wined3d/wined3d_private.h
+++ b/dlls/wined3d/wined3d_private.h
@@ -4155,6 +4155,26 @@ const char *wined3d_debug_view_desc(const struct wined3d_view_desc *d,
         const struct wined3d_resource *resource) DECLSPEC_HIDDEN;
 const char *wined3d_debug_vkresult(VkResult vr) DECLSPEC_HIDDEN;
 
+static inline ULONG wined3d_atomic_decrement_mutex_lock(volatile LONG *refcount)
+{
+    ULONG count, old_count = *refcount;
+    do
+    {
+        if ((count = old_count) == 1)
+        {
+            wined3d_mutex_lock();
+            count = InterlockedDecrement(refcount);
+            if (count) wined3d_mutex_unlock();
+            return count;
+        }
+
+        old_count = InterlockedCompareExchange(refcount, count - 1, count);
+    }
+    while (old_count != count);
+
+    return count - 1;
+}
+
 struct wined3d_client_resource
 {
     /* The resource's persistently mapped address, which we may use to perform
-- 
2.32.0

[Henri Verbeet]

Signed-off-by: Henri Verbeet <hverbeet(a)codeweavers.com>

[Henri Verbeet]

On Wed, 1 Dec 2021 at 13:31, Jan Sikorski <jsikorski(a)codeweavers.com> wrote:
> Samplers, blend states, rasterizer states and depth stencil states can
> be retrieved from the cache in struct d3d_device even after the
> reference count reaches zero, causing memory corruption.

I'm signing off on this because it solves a real problem and we're
close to code-freeze, but I don't think this is great. Ultimately,
d3d11 can't rely on wined3d_mutex_lock()/wined3d_mutex_unlock() to
protect its own internal data structures like the state object caches,
simply because they're going to go away. That means the interaction
between the cache, parent_ops, and locking is going to need some more
thought. It may end up being the case that the caches should be
handled be wined3d, for example.