[PATCH v3 2/2] sombase: 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 | 62 ++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c
index 360c2c3e987..23386f27d69 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;
 }

@@ -1893,28 +1921,9 @@ static HRESULT proxy_manager_get_remunknown(struct proxy_manager * This, IRemUnk
  * it could add a proxy to IRemUnknown into the apartment. */
 static void proxy_manager_destroy(struct proxy_manager * This)
 {
-    struct list * cursor;
-
     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);