Re: [PATCH v2] ole32: Store proxy/stub CLSIDs per process, not per apartment.
On Tue, Aug 01, 2017 at 04:54:45PM -0500, Zebediah Figura wrote:
v2: use proper synchronization; add an MTA test (thanks Sebastian) Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ole32/compobj.c | 48 ++++++++++++++++++++++++++++++------------ dlls/ole32/compobj_private.h | 1 - dlls/ole32/tests/compobj.c | 50 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 16 deletions(-)
diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index 60244485249..ee904adbc38 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -170,8 +170,20 @@ struct registered_psclsid struct list entry; IID iid; CLSID clsid; + OXID apartment_id; };
+static struct list RegisteredPSCLSIDList = LIST_INIT(RegisteredPSCLSIDList);
Please don't use CamelCase.
+ +static CRITICAL_SECTION csRegisteredPSCLSIDList; +static CRITICAL_SECTION_DEBUG psclsid_cs_debug = +{ + 0, 0, &csRegisteredPSCLSIDList, + { &psclsid_cs_debug.ProcessLocksList, &psclsid_cs_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": csRegisteredPSCLSIDList") } +}; +static CRITICAL_SECTION csRegisteredPSCLSIDList = { &psclsid_cs_debug, -1, 0, 0, 0, 0 }; + /* * This is a marshallable object exposing registered local servers. * IServiceProvider is used only because it happens meet requirements @@ -622,7 +634,6 @@ static APARTMENT *apartment_construct(DWORD model)
list_init(&apt->proxies); list_init(&apt->stubmgrs); - list_init(&apt->psclsids); list_init(&apt->loaded_dlls); apt->ipidc = 0; apt->refs = 1; @@ -1173,15 +1184,22 @@ DWORD apartment_release(struct apartment *apt) stub_manager_int_release(stubmgr); }
- LIST_FOR_EACH_SAFE(cursor, cursor2, &apt->psclsids) + EnterCriticalSection(&csRegisteredPSCLSIDList); + + LIST_FOR_EACH_SAFE(cursor, cursor2, &RegisteredPSCLSIDList) { struct registered_psclsid *registered_psclsid = LIST_ENTRY(cursor, struct registered_psclsid, entry);
- list_remove(®istered_psclsid->entry); - HeapFree(GetProcessHeap(), 0, registered_psclsid); + if (registered_psclsid->apartment_id == apt->oxid) + { + list_remove(®istered_psclsid->entry); + HeapFree(GetProcessHeap(), 0, registered_psclsid); + } }
+ LeaveCriticalSection(&csRegisteredPSCLSIDList); + /* if this assert fires, then another thread took a reference to a * stub manager without taking a reference to the containing * apartment, which it must do. */
It seems it's more complicated than this. Consider two already existing STAs, if the PSClsid is registered in STA1 which then leaves the apt, a call in STA2 to CoGetPSCLSID() still succeeds. You'll want to expand the tests to cover things like this to really figure out what's going on. Also note that the MTA -> MTA test isn't very convincing, since there is only one MTA and hence both threads are in the same apt. Huw.
On 08/02/2017 04:32 AM, Huw Davies wrote:
On Tue, Aug 01, 2017 at 04:54:45PM -0500, Zebediah Figura wrote:
v2: use proper synchronization; add an MTA test (thanks Sebastian) Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- dlls/ole32/compobj.c | 48 ++++++++++++++++++++++++++++++------------ dlls/ole32/compobj_private.h | 1 - dlls/ole32/tests/compobj.c | 50 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 83 insertions(+), 16 deletions(-)
diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index 60244485249..ee904adbc38 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -170,8 +170,20 @@ struct registered_psclsid struct list entry; IID iid; CLSID clsid; + OXID apartment_id; };
+static struct list RegisteredPSCLSIDList = LIST_INIT(RegisteredPSCLSIDList);
Please don't use CamelCase.
+ +static CRITICAL_SECTION csRegisteredPSCLSIDList; +static CRITICAL_SECTION_DEBUG psclsid_cs_debug = +{ + 0, 0, &csRegisteredPSCLSIDList, + { &psclsid_cs_debug.ProcessLocksList, &psclsid_cs_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": csRegisteredPSCLSIDList") } +}; +static CRITICAL_SECTION csRegisteredPSCLSIDList = { &psclsid_cs_debug, -1, 0, 0, 0, 0 }; + /* * This is a marshallable object exposing registered local servers. * IServiceProvider is used only because it happens meet requirements @@ -622,7 +634,6 @@ static APARTMENT *apartment_construct(DWORD model)
list_init(&apt->proxies); list_init(&apt->stubmgrs); - list_init(&apt->psclsids); list_init(&apt->loaded_dlls); apt->ipidc = 0; apt->refs = 1; @@ -1173,15 +1184,22 @@ DWORD apartment_release(struct apartment *apt) stub_manager_int_release(stubmgr); }
- LIST_FOR_EACH_SAFE(cursor, cursor2, &apt->psclsids) + EnterCriticalSection(&csRegisteredPSCLSIDList); + + LIST_FOR_EACH_SAFE(cursor, cursor2, &RegisteredPSCLSIDList) { struct registered_psclsid *registered_psclsid = LIST_ENTRY(cursor, struct registered_psclsid, entry);
- list_remove(®istered_psclsid->entry); - HeapFree(GetProcessHeap(), 0, registered_psclsid); + if (registered_psclsid->apartment_id == apt->oxid) + { + list_remove(®istered_psclsid->entry); + HeapFree(GetProcessHeap(), 0, registered_psclsid); + } }
+ LeaveCriticalSection(&csRegisteredPSCLSIDList); + /* if this assert fires, then another thread took a reference to a * stub manager without taking a reference to the containing * apartment, which it must do. */
It seems it's more complicated than this. Consider two already existing STAs, if the PSClsid is registered in STA1 which then leaves the apt, a call in STA2 to CoGetPSCLSID() still succeeds.
You'll want to expand the tests to cover things like this to really figure out what's going on.
Also note that the MTA -> MTA test isn't very convincing, since there is only one MTA and hence both threads are in the same apt.
Huw.
I see; I'll send an updated patch.
participants (2)
-
Huw Davies -
Zebediah Figura