[PATCH v6 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 incrementing the reference count only if it is already nonzero. If the reference count is zero, any reference to the proxy manager will become invalid after the current thread leaves the critical section (apt->cs). This is because proxy_manager_destroy() will proceed to destroy the proxy manager after the apartment lock (apt->cs) is released. An alternative solution would be to prevent find_proxy_manager from observing the zero reference count in the first place. Multiple approaches have been considered for implementing this solution, but were eventually dropped due to several disadvantages when applied to the current Wine codebase: 1. Always acquire the apartment lock from the proxy manager's Release() method, and hold the lock until the proxy manager is completely removed from the list if the reference count drops to zero. This requires handling the special case when the proxy manager's parent apartment has been destroyed. When an apartment is destroyed, it sets the `parent` field of each proxy manager that was previously owned by the apartment to NULL. This means that each access to `This->parent->cs` has to be guarded by a NULL check for `This->parent`. Even if `parent` were never NULL, unconditionally acquiring a lock may unnecessarily block the subroutine and introduce contention. 2. Opportunistically decrement the reference count without holding the lock, but only if the count is greater than 1. This approach is still not free from the NULL parent apartment problem. 3. Check for any concurrent reference count change from proxy_manager_destroy(), and bail out if such change has occurred. This makes it possible for the proxy manager's AddRef() method to return 1, which is unusual. 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.
-- v6: 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 incrementing the reference count only if it is already nonzero. If the reference count is zero, any reference to the proxy manager will become invalid after the current thread leaves the critical section (apt->cs). This is because proxy_manager_destroy() will proceed to destroy the proxy manager after the apartment lock (apt->cs) is released. An alternative solution would be to prevent find_proxy_manager from observing the zero reference count in the first place. Multiple approaches have been considered for implementing this solution, but were eventually dropped due to several disadvantages when applied to the current Wine codebase: 1. Always acquire the apartment lock from the proxy manager's Release() method, and hold the lock until the proxy manager is completely removed from the list if the reference count drops to zero. This requires handling the special case when the proxy manager's parent apartment has been destroyed. When an apartment is destroyed, it sets the `parent` field of each proxy manager that was previously owned by the apartment to NULL. This means that each access to `This->parent->cs` has to be guarded by a NULL check for `This->parent`. Even if `parent` were never NULL, unconditionally acquiring a lock may unnecessarily block the subroutine and introduce contention. 2. Opportunistically decrement the reference count without holding the lock, but only if the count is greater than 1. This approach is still not free from the NULL parent apartment problem. 3. Check for any concurrent reference count change from proxy_manager_destroy(), and bail out if such change has occurred. This makes it possible for the proxy manager's AddRef() method to return 1, which is unusual. --- dlls/combase/marshal.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c index 360c2c3e987..e2f6a57d8e1 100644 --- a/dlls/combase/marshal.c +++ b/dlls/combase/marshal.c @@ -75,7 +75,7 @@ struct proxy_manager OXID_INFO oxid_info; /* string binding, ipid of rem unknown and other information (RO) */ OID oid; /* object ID (RO) */ struct list interfaces; /* imported interfaces (CS cs) */ - LONG refs; /* proxy reference count (LOCK) */ + LONG refs; /* proxy reference count (LOCK); 0 if about to be removed from list */ CRITICAL_SECTION cs; /* thread safety for this object and children */ ULONG sorflags; /* STDOBJREF flags (RO) */ IRemUnknown *remunk; /* proxy to IRemUnknown used for lifecycle management (CS cs) */ @@ -1887,6 +1887,32 @@ static HRESULT proxy_manager_get_remunknown(struct proxy_manager * This, IRemUnk return hr; } +/* + * Safely increment the reference count of a proxy manager obtained from an + * apartment proxy list. + * + * This function shall be called inside the apartment's critical section. + */ +static LONG proxy_manager_addref_if_alive(struct proxy_manager * This) +{ + LONG refs = ReadNoFence(&This->refs); + LONG old_refs, new_refs; + + do + { + if (refs == 0) + { + /* This proxy manager is about to be destroyed */ + return 0; + } + + old_refs = refs; + new_refs = refs + 1; + } while ((refs = InterlockedCompareExchange(&This->refs, new_refs, old_refs)) != old_refs); + + return new_refs; +} + /* destroys a proxy manager, freeing the memory it used. * Note: this function should not be called from a list iteration in the * apartment, due to the fact that it removes itself from the apartment and @@ -1949,7 +1975,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) != 1) + if (proxy_manager_addref_if_alive(proxy) != 0) { *proxy_found = proxy; found = TRUE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2752
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2752
participants (3)
-
Huw Davies (＠huw) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)