[PATCH v4 0/2] MR2752: combase: Prevent use-after-free due to a race with proxy_manager_destroy.

[Jinoh Kang (＠iamahuman)]

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:
> - Thread X: A proxy manager's refcount reaches 0 and is about to be released.
> - Thread Y tries to grab it, calls AddRef, notices it returns 1, and drops it.
> - Thread Z tries to grab it, calls AddRef, notices it returns 2, and returns it.
> - Thread X then proceeds to remove it from the list and free the object.

--
  v4: combase: Prevent use-after-free due to a race with proxy_manager_destroy.

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

[PATCH v4 1/2] combase: Compare AddRef() return value against 1 instead of 0 in find_proxy_manager.

[Jinoh Kang]

From: Jinoh Kang

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;

[PATCH v4 2/2] combase: Prevent use-after-free due to a race with proxy_manager_destroy.

[Jinoh Kang]

From: Jinoh Kang

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 | 60 +++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c
index 360c2c3e987..524ed08ffa3 100644
--- a/dlls/combase/marshal.c
+++ b/dlls/combase/marshal.c
@@ -986,10 +986,38 @@ 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)
+        {
+            struct list * cursor;
+
+            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 +1926,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 +1957,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);

[Zebediah Figura (＠zfigura)]

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

There is in fact a proxy manager cs (the "cs" member of struct proxy_manager), but grabbing it doesn't help here—we need to grab the same cs that's held in proxy_manager_destroy() when removing from the list.

We could potentially grab the proxy manager cs in proxy_manager_destroy(), though that'd take some investigation as to whether it's actually safe to do so without causing deadlocks and/or lock inversion.

I agree that grabbing the apartment cs conditionally doesn't feel great.

It's also worth pointing out that the general pattern of addref-if-nonzero, although it's kind of fragile, isn't exactly a new one; I've seen it in other code. I think it's probably the best option here.

[Huw Davies (＠huw)]

> It's also worth pointing out that the general pattern of addref-if-nonzero, although it's kind of fragile, isn't exactly a new one; I've seen it in other code. I think it's probably the best option here.

Ok, let's go with that then.  I just wanted to make sure that other options had at least been thought about, as that wasn't at all clear from the description.