[PATCH 0/2] MR7880: combase: Fix refcount leak fix in apartment_hostobject
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7880
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> 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 <PuetzKevinA(a)JohnDeere.com> --- 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))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7880
From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com> 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 <PuetzKevinA(a)JohnDeere.com> --- 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, ¶ms->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))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7880
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... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7880#note_101639
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7880#note_101640
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...
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7880#note_101641
participants (3)
-
Kevin Puetz -
Kevin Puetz (＠puetzk)
-
Nikolay Sivov (＠nsivov)