[PATCH v2 0/1] MR4002: oleaut32: Fix typelib cache race condition.

[Connor McAdams (＠cmcadams)]

This fixes the following issue:

- Thread X holds the typelib cache list CS, and is iterating through the list.
- Thread Y calls Release on the typelib interface, reference count hits 0, it's waiting for the cache list CS to remove this typelib.
- Thread X finds the typelib in the cache list, returns it, exits the CS.
- Thread Y enters now the CS, removes the typelib from the cache list, and proceeds to free all of the resources associated with the typelib.
- Thread X tries to use the typelib, use after free.

The method used here to prevent incrementing the reference count is borrowed from MR !2752.

An alternative could be to decouple the actual typelib data from the `ITypeLib2` interface itself, e.g what gets stored into the typelib cache is just the data for the typelib, which would have its own private reference count locked behind the cache list CS. When the public `ITypeLib2` interface's reference count hits 0, it'd lock the typelib cache list CS, decrement the private reference count, and if the private reference count is 0 remove it from the list.

The alternative method would require major restructuring of the code, which is why I'd prefer to use the one in this MR.

--
  v2: oleaut32: Lock ITypeLib2 interface reference count behind the typelib cache critical section.

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

[PATCH v2 1/1] oleaut32: Lock ITypeLib2 interface reference count behind the typelib cache critical section.

[Connor McAdams]

From: Connor McAdams

This prevents an ITypeLib2 interface being returned from the typelib
cache that is in the middle of being destroyed.

Signed-off-by: Connor McAdams 
---
 dlls/oleaut32/typelib.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c
index fa512184182..cd79d71ff19 100644
--- a/dlls/oleaut32/typelib.c
+++ b/dlls/oleaut32/typelib.c
@@ -4702,7 +4702,11 @@ static HRESULT WINAPI ITypeLib2_fnQueryInterface(ITypeLib2 *iface, REFIID riid,
 static ULONG WINAPI ITypeLib2_fnAddRef( ITypeLib2 *iface)
 {
     ITypeLibImpl *This = impl_from_ITypeLib2(iface);
-    ULONG ref = InterlockedIncrement(&This->ref);
+    ULONG ref;
+
+    EnterCriticalSection(&cache_section);
+    ref = ++This->ref;
+    LeaveCriticalSection(&cache_section);

TRACE("%p, refcount %lu.\n", iface, ref);

@@ -4712,7 +4716,10 @@ static ULONG WINAPI ITypeLib2_fnAddRef( ITypeLib2 *iface)
 static ULONG WINAPI ITypeLib2_fnRelease( ITypeLib2 *iface)
 {
     ITypeLibImpl *This = impl_from_ITypeLib2(iface);
-    ULONG ref = InterlockedDecrement(&This->ref);
+    ULONG ref;
+
+    EnterCriticalSection(&cache_section);
+    ref = --This->ref;

TRACE("%p, refcount %lu.\n", iface, ref);

@@ -4728,10 +4735,8 @@ static ULONG WINAPI ITypeLib2_fnRelease( ITypeLib2 *iface)
       if(This->path)
       {
           TRACE("removing from cache list\n");
-          EnterCriticalSection(&cache_section);
           if(This->entry.next)
               list_remove(&This->entry);
-          LeaveCriticalSection(&cache_section);
           free(This->path);
       }
       TRACE(" destroying ITypeLib(%p)\n",This);
@@ -4783,9 +4788,9 @@ static ULONG WINAPI ITypeLib2_fnRelease( ITypeLib2 *iface)
       }
       free(This->typeinfos);
       free(This);
-      return 0;
     }

+    LeaveCriticalSection(&cache_section);
     return ref;
 }

[Connor McAdams (＠cmcadams)]

On Wed Nov  8 12:51:32 2023 +0000, Huw Davies wrote:
> I like simple :-)  I doubt that the contention is going to be a big
> issue here - if it turns out to be the case we can revisit this.

I've pushed a change that does what you've suggested, let me know what you think. :)

[Huw Davies (＠huw)]

Much simpler :-)

[PATCH v2 0/1] MR4002: oleaut32: Fix typelib cache race condition. - approved

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.

[Alexandre Julliard (＠julliard)]

Is it really necessary to lock in AddRef?

[Connor McAdams (＠cmcadams)]

On Thu Nov  9 13:11:18 2023 +0000, Alexandre Julliard wrote:
> Is it really necessary to lock in AddRef?

As it is now, yes. Other threads can hold a reference to the `ITypeLib2` object, which would mean they could potentially call `AddRef` while the lock is held by another thread, which would mess up the reference count.

I guess we could move back to atomics on the reference count, which would remove the need to lock in `AddRef`. It'd feel a bit weird to enter the CS in `Release` and _then_ call `InterlockedDecrement`, just because we'd be using two layers of synchronization functions, but I can't come up with a scenario where that would necessarily break things.

Would you prefer using `InterlockedIncrement` in `AddRef` instead of using the CS lock?

[Huw Davies (＠huw)]

On Thu Nov  9 13:11:18 2023 +0000, Connor McAdams wrote:
> As it is now, yes. Other threads can hold a reference to the `ITypeLib2`
> object, which would mean they could potentially call `AddRef` while the
> lock is held by another thread, which would mess up the reference count.
> I guess we could move back to atomics on the reference count, which
> would remove the need to lock in `AddRef`. It'd feel a bit weird to
> enter the CS in `Release` and _then_ call `InterlockedDecrement`, just
> because we'd be using two layers of synchronization functions, but I
> can't come up with a scenario where that would necessarily break things.
> Would you prefer using `InterlockedIncrement` in `AddRef` instead of
> using the CS lock?

Yes, we don't need the lock in AddRef, but the symmetry seemed a bit more pleasing. I'd be happy if we dropped it though.

[Alexandre Julliard (＠julliard)]

> Would you prefer using InterlockedIncrement in AddRef instead of using the CS lock?

Yes, it's good practice to always use interlocked functions for COM refcounts.