[PATCH 0/1] MR8949: dxcore: Hold a critical section when interacting with the global factory.

[Tim Clem (＠tclem)]

If multiple threads are creating and releasing the factory, these
accesses are racy and can result in callers of
DXCoreCreateAdapterFactory getting a null pointer or a pointer to
freed memory.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949

[PATCH 1/1] dxcore: Hold a critical section when interacting with the global factory.

[Tim Clem]

From: Tim Clem <tclem(a)codeweavers.com>

If multiple threads are creating and releasing the factory, these
accesses are racy and can result in callers of
DXCoreCreateAdapterFactory getting a null pointer or a pointer to
freed memory.
---
 dlls/dxcore/dxcore.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/dlls/dxcore/dxcore.c b/dlls/dxcore/dxcore.c
index 71cdca9412a..3de9ce99220 100644
--- a/dlls/dxcore/dxcore.c
+++ b/dlls/dxcore/dxcore.c
@@ -29,6 +29,15 @@ WINE_DEFAULT_DEBUG_CHANNEL(dxcore);
 
 static struct dxcore_adapter_factory *dxcore_adapter_factory;
 
+static CRITICAL_SECTION factory_section;
+static CRITICAL_SECTION_DEBUG critsect_debug =
+{
+    0, 0, &factory_section,
+    { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
+    0, 0, { (DWORD_PTR)(__FILE__ ": factory_section") }
+};
+static CRITICAL_SECTION factory_section = { &critsect_debug, -1, 0, 0, 0, 0 };
+
 struct dxcore_adapter
 {
     IDXCoreAdapter IDXCoreAdapter_iface;
@@ -428,18 +437,26 @@ static HRESULT STDMETHODCALLTYPE dxcore_adapter_factory_QueryInterface(IDXCoreAd
 static ULONG STDMETHODCALLTYPE dxcore_adapter_factory_AddRef(IDXCoreAdapterFactory *iface)
 {
     struct dxcore_adapter_factory *factory = impl_from_IDXCoreAdapterFactory(iface);
-    ULONG refcount = InterlockedIncrement(&factory->refcount);
+    ULONG refcount;
+
+    EnterCriticalSection(&factory_section);
 
+    refcount = InterlockedIncrement(&factory->refcount);
     TRACE("%p increasing refcount to %lu.\n", iface, refcount);
 
+    LeaveCriticalSection(&factory_section);
+
     return refcount;
 }
 
 static ULONG STDMETHODCALLTYPE dxcore_adapter_factory_Release(IDXCoreAdapterFactory *iface)
 {
     struct dxcore_adapter_factory *factory = impl_from_IDXCoreAdapterFactory(iface);
-    ULONG refcount = InterlockedDecrement(&factory->refcount);
+    ULONG refcount;
+
+    EnterCriticalSection(&factory_section);
 
+    refcount = InterlockedDecrement(&factory->refcount);
     TRACE("%p decreasing refcount to %lu.\n", iface, refcount);
 
     if (!refcount)
@@ -448,6 +465,8 @@ static ULONG STDMETHODCALLTYPE dxcore_adapter_factory_Release(IDXCoreAdapterFact
         dxcore_adapter_factory = NULL;
     }
 
+    LeaveCriticalSection(&factory_section);
+
     return refcount;
 }
 
@@ -614,15 +633,20 @@ static const struct IDXCoreAdapterFactoryVtbl dxcore_adapter_factory_vtbl =
 
 HRESULT STDMETHODCALLTYPE DXCoreCreateAdapterFactory(REFIID riid, void **out)
 {
+    HRESULT hr;
+
     TRACE("riid %s, out %p\n", debugstr_guid(riid), out);
 
     if (!out)
         return E_POINTER;
 
+    EnterCriticalSection(&factory_section);
+
     if (!dxcore_adapter_factory)
     {
         if (!(dxcore_adapter_factory = calloc(1, sizeof(*dxcore_adapter_factory))))
         {
+            LeaveCriticalSection(&factory_section);
             *out = NULL;
             return E_OUTOFMEMORY;
         }
@@ -631,6 +655,10 @@ HRESULT STDMETHODCALLTYPE DXCoreCreateAdapterFactory(REFIID riid, void **out)
         dxcore_adapter_factory->refcount = 0;
     }
 
+    hr = IDXCoreAdapterFactory_QueryInterface(&dxcore_adapter_factory->IDXCoreAdapterFactory_iface, riid, out);
+
+    LeaveCriticalSection(&factory_section);
+
     TRACE("created IDXCoreAdapterFactory %p.\n", *out);
-    return IDXCoreAdapterFactory_QueryInterface(&dxcore_adapter_factory->IDXCoreAdapterFactory_iface, riid, out);
+    return hr;
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8949

[Nikolay Sivov (＠nsivov)]

It's not caused by your change, but one awkward thing is that calling with wrong IID will create a factory with 0 refcount and function will return leaving it like that. I think it's better to set global pointer only on success.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949#note_115581

[Tim Clem (＠tclem)]

On Wed Sep 10 18:01:43 2025 +0000, Nikolay Sivov wrote:
> It's not caused by your change, but one awkward thing is that calling
> with wrong IID will create a factory with 0 refcount and function will
> return leaving it like that. I think it's better to set global pointer
> only on success. It will get bumped to 1 on first successful call, but
> still having a live object with refcount == 0 is not intuitive.
> Looks like native just returns E_NOINTERFACE for the wrong IID. May as well just check that up front and bail early, right?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949#note_115582

[Elizabeth Figura (＠zfigura)]

The synchronization doesn't really look right. We should probably just make the factory static, and then there should be no need for a mutex.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949#note_115587

[Tim Clem (＠tclem)]

On Wed Sep 10 18:20:43 2025 +0000, Elizabeth Figura wrote:
> The synchronization doesn't really look right. We should probably just
> make the factory static, and then there should be no need for a mutex.
> Huh, ok. What should we do with the refcount? Just make AddRef and Release no-ops?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949#note_115589

[Elizabeth Figura (＠zfigura)]

On Wed Sep 10 18:20:43 2025 +0000, Tim Clem wrote:
> Huh, ok. What should we do with the refcount? Just make AddRef and
> Release no-ops?
> The refcount should stay I think; it just won't free anything on release.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8949#note_115598