[PATCH v2 0/2] MR2752: combase: Prevent use-after-free due to a race with proxy_manager_destroy.
Today, find_proxy_manager() may return a proxy manager that is about to be freed. This happens when the proxy manager's reference count reaches zero, but proxy_manager_destroy() has not removed the proxy manager from the apartment proxy list. Fix this by only incrementing the reference count if it was nonzero. If the reference count is zero, proxy_manager_destroy() will proceed to destroy the proxy manager after the current thread releases the apartment lock (apt->cs). Granted, this is fragile and far from elegant, but it seems like a safe approach for the time being. Also, the critical section and "safe" refcount increment will prevent the following race condition scenario:
1. Thread X: A proxy manager's refcount reaches 0 and is about to be released. 2. Thread Y tries to grab it, calls AddRef, notices it returns 1, and drops it. 3. Thread Z tries to grab it, calls AddRef, notices it returns 2, and returns it. 4. Thread X then proceeds to remove it from the list and free the object.
-- v2: combase: Prevent use-after-free due to a race with proxy_manager_destroy. https://gitlab.winehq.org/wine/wine/-/merge_requests/2752
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, find_proxy_manager() tests if AddRef() returns 0 in an attempt to protect against a race condition bug. Note that AddRef does not return zero in normal circumstances, since AddRef returns the reference count after the increment, not before. Fix this by comparing the return value of AddRef() against 1, not 0. --- dlls/combase/marshal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c index a3da851b139..360c2c3e987 100644 --- a/dlls/combase/marshal.c +++ b/dlls/combase/marshal.c @@ -1949,7 +1949,7 @@ static BOOL find_proxy_manager(struct apartment * apt, OXID oxid, OID oid, struc /* be careful of a race with ClientIdentity_Release, which would * cause us to return a proxy which is in the process of being * destroyed */ - if (IMultiQI_AddRef(&proxy->IMultiQI_iface) != 0) + if (IMultiQI_AddRef(&proxy->IMultiQI_iface) != 1) { *proxy_found = proxy; found = TRUE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2752
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> Today, find_proxy_manager() may return a proxy manager that is about to be freed. This happens when the proxy manager's reference count reaches zero, but proxy_manager_destroy() has not removed the proxy manager from the apartment proxy list. Fix this by protecting reference count decrement with the apartment lock (apt->cs). --- dlls/combase/marshal.c | 58 ++++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c index 360c2c3e987..b5bcd504ec1 100644 --- a/dlls/combase/marshal.c +++ b/dlls/combase/marshal.c @@ -986,10 +986,36 @@ static ULONG WINAPI ClientIdentity_AddRef(IMultiQI *iface) static ULONG WINAPI ClientIdentity_Release(IMultiQI *iface) { struct proxy_manager *This = impl_from_IMultiQI(iface); - ULONG refs = InterlockedDecrement(&This->refs); + ULONG refs; + + if (This->parent) + { + EnterCriticalSection(&This->parent->cs); + } + + refs = InterlockedDecrement(&This->refs); TRACE("%p - after %ld\n", iface, refs); + + if (This->parent) + { + if (!refs) + { + LIST_FOR_EACH(cursor, &This->parent->proxies) + { + if (cursor == &This->entry) + { + list_remove(&This->entry); + break; + } + } + } + + LeaveCriticalSection(&This->parent->cs); + } + if (!refs) proxy_manager_destroy(This); + return refs; } @@ -1898,23 +1924,6 @@ static void proxy_manager_destroy(struct proxy_manager * This) TRACE("oxid = %s, oid = %s\n", wine_dbgstr_longlong(This->oxid), wine_dbgstr_longlong(This->oid)); - if (This->parent) - { - EnterCriticalSection(&This->parent->cs); - - /* remove ourself from the list of proxy objects in the apartment */ - LIST_FOR_EACH(cursor, &This->parent->proxies) - { - if (cursor == &This->entry) - { - list_remove(&This->entry); - break; - } - } - - LeaveCriticalSection(&This->parent->cs); - } - /* destroy all of the interface proxies */ while ((cursor = list_head(&This->interfaces))) { @@ -1946,15 +1955,10 @@ static BOOL find_proxy_manager(struct apartment * apt, OXID oxid, OID oid, struc { if ((oxid == proxy->oxid) && (oid == proxy->oid)) { - /* be careful of a race with ClientIdentity_Release, which would - * cause us to return a proxy which is in the process of being - * destroyed */ - if (IMultiQI_AddRef(&proxy->IMultiQI_iface) != 1) - { - *proxy_found = proxy; - found = TRUE; - break; - } + IMultiQI_AddRef(&proxy->IMultiQI_iface); + *proxy_found = proxy; + found = TRUE; + break; } } LeaveCriticalSection(&apt->cs); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2752
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=132724 Your paranoid android. === debian11 (build log) === ../wine/dlls/combase/marshal.c:1003:27: error: ���cursor��� undeclared (first use in this function) Task: The win32 Wine build failed === debian11b (build log) === ../wine/dlls/combase/marshal.c:1003:27: error: ���cursor��� undeclared (first use in this function) Task: The wow64 Wine build failed
Have we thought about using the manager's cs to guard `refs` (and then remove the manager from the apt's list while in that cs?
I've done this, but I'm not sure if this is an improvement: 1. It's important to note that there is no such thing as the manager's cs. There is the *apartment*'s cs, which protects the manager list. The manager may or may not have a apartment, so we should guard against `This->parent == NULL` case. This inevitably makes the code more branchy, with more if statements. 2. This makes the manager's `Release()` unconditionally blocking, and introduces contention to the apartment's cs. Let me know if there are any alternatives that looks more attractive. I have a few options, but it's not clear to me if any of these are better, either: 1. Make it opportunistic: use CAS to only decrement when it's greater than 1. This way, we can only acquire the lock on the "last release" case. 2. Always decrement, then backoff: after entering the critical section, check if the reference count has been increased in the meanwhile. If it has, then cancel the decrement and bailout. This has the disadvantage of introducing "off-by-one" nondeterminism to the return value of `AddRef()` and `Release()`. 3. Introduce a "fake" apartment that orphaned proxy managers end up, and use its address in lieu of `NULL`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2752#note_32888
participants (3)
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Marvin