[PATCH 0/2] MR7880: combase: Fix refcount leak fix in apartment_hostobject

[Kevin Puetz (＠puetzk)]

Demonstrate a refcount leak on the class factory when a class with
ThreadingModel=Apartment is instantiated in the MTA. CoGetClassObject
ends up using apartment_hostobject to launch an STA thread in which to
host the object, but the process of marshaling this class factory back
into the MTA leaks a reference on it.

This leak is usually minor (class factory implementations are usually
singletons, so it's a one-of rather than a cumulative leak), but it will
generally lock that COM server and e.g. prevent CoFreeUnusedLibraries
from being able to clean up properly.

When CoMarshalInterface succeeds, it added an *additional* refcount
represented by the marshaling data written to the stream. The original
refcount returned from apartment_getclassobject needs to be released
regardless of marshaling success or failure.

[PATCH 1/2] combase: Test refcount leak fix in apartment_hostobject

[Kevin Puetz]

From: Kevin Puetz

Demonstrate a refcount leak on the class factory when a class with
ThreadingModel=Apartment is instantiated in the MTA. CoGetClassObject
ends up using apartment_hostobject to launch an STA thread in which to
host the object, but the process of marshaling this class factory back
into the MTA leaks a reference on it.

This leak is usually minor (class factory implementations are usually
singletons, so it's a one-of rather than a cumulative leak), but it will
generally lock that COM server and e.g. prevent CoFreeUnusedLibraries
from being able to clean up properly.

Signed-off-by: Kevin Puetz 
---
 dlls/ole32/tests/compobj.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c
index 94b83024b91..1ebc92553f7 100644
--- a/dlls/ole32/tests/compobj.c
+++ b/dlls/ole32/tests/compobj.c
@@ -750,6 +750,7 @@ static void test_CoGetClassObject(void)
     REFCLSID rclsid = &CLSID_InternetZoneManager;
     HKEY hkey;
     LONG res;
+    ULONG refs;

hr = CoGetClassObject(rclsid, CLSCTX_INPROC_SERVER, NULL, &IID_IUnknown, (void **)&pUnk);
     ok(hr == CO_E_NOTINITIALIZED, "CoGetClassObject should have returned CO_E_NOTINITIALIZED instead of 0x%08lx\n", hr);
@@ -794,7 +795,8 @@ static void test_CoGetClassObject(void)

hr = CoGetClassObject(&CLSID_InProcFreeMarshaler, CLSCTX_INPROC_SERVER, NULL, &IID_IUnknown, (void **)&pUnk);
     ok(hr == S_OK, "got 0x%08lx\n", hr);
-    IUnknown_Release(pUnk);
+    refs = IUnknown_Release(pUnk);
+    todo_wine ok(refs == 0, "Expected 0, got %lu\n", refs);

/* context redefines FreeMarshaler CLSID */
     if ((handle = activate_context(actctx_manifest, &cookie)))

[PATCH 2/2] combase: Fix refcount leak in apartment_hostobject

[Kevin Puetz]

From: Kevin Puetz

When CoMarshalInterface succeeds, it added an *additional* refcount
represented by the marshaling data written to the stream. The original
refcount returned from apartment_getclassobject needs to be released
regardless of marshaling success or failure.

Signed-off-by: Kevin Puetz 
---
 dlls/combase/apartment.c   | 4 ++--
 dlls/ole32/tests/compobj.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/dlls/combase/apartment.c b/dlls/combase/apartment.c
index 85e06a6fd57..62a043d4670 100644
--- a/dlls/combase/apartment.c
+++ b/dlls/combase/apartment.c
@@ -1098,8 +1098,8 @@ static HRESULT apartment_hostobject(struct apartment *apt, const struct host_obj
         return hr;

hr = CoMarshalInterface(params->stream, &params->iid, object, MSHCTX_INPROC, NULL, MSHLFLAGS_NORMAL);
-    if (FAILED(hr))
-        IUnknown_Release(object);
+
+    IUnknown_Release(object);
     IStream_Seek(params->stream, llZero, STREAM_SEEK_SET, NULL);

return hr;
diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c
index 1ebc92553f7..2955d15d2ee 100644
--- a/dlls/ole32/tests/compobj.c
+++ b/dlls/ole32/tests/compobj.c
@@ -796,7 +796,7 @@ static void test_CoGetClassObject(void)
     hr = CoGetClassObject(&CLSID_InProcFreeMarshaler, CLSCTX_INPROC_SERVER, NULL, &IID_IUnknown, (void **)&pUnk);
     ok(hr == S_OK, "got 0x%08lx\n", hr);
     refs = IUnknown_Release(pUnk);
-    todo_wine ok(refs == 0, "Expected 0, got %lu\n", refs);
+    ok(refs == 0, "Expected 0, got %lu\n", refs);

/* context redefines FreeMarshaler CLSID */
     if ((handle = activate_context(actctx_manifest, &cookie)))

[Kevin Puetz (＠puetzk)]

> ole32:compobj compobj.c:799 Test failed: Expected 0, got 1

This failure (only on linux-32, not linux-64) is definitely relevant to the changes we made (which fixed the leak on linux-64). I guess marshaling has enough platform-specific code that perhaps linux-32 could have another unrelated leak...

[Nikolay Sivov (＠nsivov)]

On Tue Apr 22 19:06:59 2025 +0000, Kevin Puetz wrote:
> > ole32:compobj compobj.c:799 Test failed: Expected 0, got 1
>
> This failure (only on linux-32, not linux-64) is definitely relevant to
> the changes we made (which fixed the leak on linux-64). I guess
> marshaling has enough platform-specific code that perhaps linux-32 could
> have another unrelated leak...

Hi, Kevin.

It fails for me locally for both cases, it's not platform specific. We don't get most (or any?) results from linux-64, that's why you don't see it reported.

Have you tried doing ole32 'make test' locally?

[PATCH 0/2] MR7880: Draft: combase: Fix refcount leak fix in apartment_hostobject

[Kevin Puetz (＠puetzk)]

On Tue Apr 22 19:06:59 2025 +0000, Nikolay Sivov wrote:
> Hi, Kevin.
> It fails for me locally for both cases, it's not platform specific. We
> don't get most (or any?) results from linux-64, that's why you don't see
> it reported.
> Have you tried doing ole32 'make test' locally?

Yes, but not as rebased to master (we're quite a bit behind, so I just ran things through testbot.winehq.org before submitting rather than doing a full local build of 10.6). I'll mark as draft for now and do some more testing...