[PATCH 0/1] MR4002: oleaut32: Fix typelib cache race condition.
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4002
From: Connor McAdams <cmcadams(a)codeweavers.com> Signed-off-by: Connor McAdams <cmcadams(a)codeweavers.com> --- 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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4002
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).
This merge request was approved by Jinoh Kang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4002
Couldn't we just enter the cache section before decrementing the ref count in `ITypeLib2_fnRelease()`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4002#note_51461
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4002#note_51481
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4002#note_51482
participants (5)
-
Connor McAdams -
Connor McAdams (＠cmcadams)
-
Huw Davies (＠huw)
-
Jinoh Kang (＠iamahuman)
-
Marvin