[PATCH 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 is 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.

[PATCH 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 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 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).
---
 dlls/combase/marshal.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/dlls/combase/marshal.c b/dlls/combase/marshal.c
index 360c2c3e987..607ac93a569 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) */
@@ -958,6 +958,7 @@ HRESULT marshal_object(struct apartment *apt, STDOBJREF *stdobjref, REFIID riid,
 /* Client-side identity of the server object */

static HRESULT proxy_manager_get_remunknown(struct proxy_manager * This, IRemUnknown **remunk);
+static LONG proxy_manager_addref_if_alive(struct proxy_manager * This);
 static void proxy_manager_destroy(struct proxy_manager * This);
 static HRESULT proxy_manager_find_ifproxy(struct proxy_manager * This, REFIID riid, struct ifproxy ** ifproxy_found);
 static HRESULT proxy_manager_query_local_interface(struct proxy_manager * This, REFIID riid, void ** ppv);
@@ -1887,6 +1888,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 +1976,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;

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

[Zebediah Figura (＠zfigura)]

This merge request was approved by Zebediah Figura.

[Zebediah Figura (＠zfigura)]

This looks good to me, if perhaps excessively verbose.

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/combase/marshal.c:
> /* Client-side identity of the server object */
>
> static HRESULT proxy_manager_get_remunknown(struct proxy_manager * This, IRemUnknown **remunk);
> +static LONG proxy_manager_addref_if_alive(struct proxy_manager * This);

This seems unnecessary as the its definition comes before its use.

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/combase/marshal.c:
> -
> - 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;
>
> +}
> -

This isn't particularly pretty.

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?  We'd need to be careful to not deadlock with the apt's cs of course.