[PATCH 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.

[PATCH 1/1] oleaut32: Prevent use-after-free by not returning a typelib from the cache that is about to be destroyed.

[Connor McAdams]

From: Connor McAdams

Signed-off-by: Connor McAdams 
---
 dlls/oleaut32/typelib.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/dlls/oleaut32/typelib.c b/dlls/oleaut32/typelib.c
index fa512184182..7fa7eeedfdc 100644
--- a/dlls/oleaut32/typelib.c
+++ b/dlls/oleaut32/typelib.c
@@ -3273,6 +3273,32 @@ static HRESULT TLB_Mapping_Open(LPCWSTR path, LPVOID *ppBase, DWORD *pdwTLBLengt
     return TYPE_E_CANTLOADLIBRARY;
 }

+/*
+ * Safely increment the reference count of a typelib obtained from the typelib
+ * cache list.
+ *
+ * This function shall be called inside the typelib cache's critical section.
+ */
+static LONG typelib_addref_if_alive(ITypeLibImpl *This)
+{
+    LONG refs = ReadNoFence(&This->ref);
+    LONG old_refs, new_refs;
+
+    do
+    {
+        if (refs == 0)
+        {
+            /* This typelib is about to be destroyed. */
+            return 0;
+        }
+
+        old_refs = refs;
+        new_refs = refs + 1;
+    } while ((refs = InterlockedCompareExchange(&This->ref, new_refs, old_refs)) != old_refs);
+
+    return new_refs;
+}
+
 /****************************************************************************
  *	TLB_ReadTypeLib
  *
@@ -3337,11 +3363,10 @@ static HRESULT TLB_ReadTypeLib(LPCWSTR pszFileName, LPWSTR pszPath, UINT cchPath
     EnterCriticalSection(&cache_section);
     LIST_FOR_EACH_ENTRY(entry, &tlb_cache, ITypeLibImpl, entry)
     {
-        if (!wcsicmp(entry->path, pszPath) && entry->index == index)
+        if (!wcsicmp(entry->path, pszPath) && entry->index == index && typelib_addref_if_alive(entry))
         {
             TRACE("cache hit\n");
             *ppTypeLib = &entry->ITypeLib2_iface;
-            ITypeLib2_AddRef(*ppTypeLib);
             LeaveCriticalSection(&cache_section);
             return S_OK;
         }
@@ -7955,11 +7980,11 @@ static HRESULT WINAPI ITypeInfo_fnGetRefTypeInfo(
                         && entry->ver_major == ref_type->pImpTLInfo->wVersionMajor
                         && entry->ver_minor == ref_type->pImpTLInfo->wVersionMinor
                         && entry->set_lcid == ref_type->pImpTLInfo->lcid
-                        && entry->syskind == This->pTypeLib->syskind)
+                        && entry->syskind == This->pTypeLib->syskind
+                        && typelib_addref_if_alive(entry))
                     {
                         TRACE("got cached %p\n", entry);
                         pTLib = (ITypeLib*)&entry->ITypeLib2_iface;
-                        ITypeLib_AddRef(pTLib);
                         result = S_OK;
                         break;
                     }

[PATCH 1/1] oleaut32: Prevent use-after-free by not returning a typelib from the cache that is about to be destroyed.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=138190

Your paranoid android.

=== debian11b (64 bit WoW report) ===

mfmediaengine:
Unhandled exception: assertion failed in 64-bit code (0x000001700275f8).

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

[Jinoh Kang (＠iamahuman)]

This merge request was approved by Jinoh Kang.

[Huw Davies (＠huw)]

Couldn't we just enter the cache section before decrementing the ref count in `ITypeLib2_fnRelease()`?

[Connor McAdams (＠cmcadams)]

On Wed Nov  8 12:38:27 2023 +0000, Huw Davies wrote:
> Couldn't we just enter the cache section before decrementing the ref
> count in `ITypeLib2_fnRelease()`?

I guess we could, but that feels a bit clunky IMO. We'd be entering/exiting the typelib cache CS every time we AddRef/Release the typelib interface, which seems to happen often.

It's a simpler solution, but the current solution avoids that potential issue. E.g, I don't think we want to cause all AddRef calls on typelib interfaces to block while waiting on a new typelib to be inserted into the cache for example.

[Huw Davies (＠huw)]

On Wed Nov  8 12:38:27 2023 +0000, Connor McAdams wrote:
> I guess we could, but that feels a bit clunky IMO. We'd be
> entering/exiting the typelib cache CS every time we AddRef/Release the
> typelib interface, which seems to happen often.
> It's a simpler solution, but the current solution avoids that potential
> issue. E.g, I don't think we want to cause all AddRef calls on typelib
> interfaces to block while waiting on a new typelib to be inserted into
> the cache for example.

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.