[PATCH 0/2] MR3372: ole32: Leave the RunningObjectTable Critical Section before unmarshalling object stream
In certain circumstances unmarshalling an object stream from the RunningObjectTable can cause the unmarshalling routines to interrogate the same table (maybe to resolve a dependant object?) in a different thread causing a deadlock while getting the critical section lock. Leaving the Critical Section before unmarshalling the object stream solve this problem. Visual Studio 2019 deadlock on start without this. I'm not sure how to test this properly. Signed-off-by: Lorenzo Ferrillo <lorenzofersteam(a)live.it> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372
From: Lorenzo Ferrillo <lorenzofersteam(a)live.it> Signed-off-by: Lorenzo Ferrillo <lorenzofersteam(a)live.it> --- dlls/ole32/moniker.c | 48 ++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/dlls/ole32/moniker.c b/dlls/ole32/moniker.c index ec026758b2f..23465829c07 100644 --- a/dlls/ole32/moniker.c +++ b/dlls/ole32/moniker.c @@ -66,7 +66,7 @@ typedef struct RunningObjectTableImpl { IRunningObjectTable IRunningObjectTable_iface; struct list rot; /* list of ROT entries */ - CRITICAL_SECTION lock; + CRITICAL_SECTION* lock; } RunningObjectTableImpl; /* define the EnumMonikerImpl structure */ @@ -417,9 +417,9 @@ RunningObjectTableImpl_Register(IRunningObjectTable* iface, DWORD flags, /* gives a registration identifier to the registered object*/ *pdwRegister = rot_entry->cookie; - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); list_add_tail(&This->rot, &rot_entry->entry); - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); return hr; } @@ -438,19 +438,19 @@ RunningObjectTableImpl_Revoke( IRunningObjectTable* iface, DWORD dwRegister) TRACE("%p, %ld.\n", iface, dwRegister); - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); LIST_FOR_EACH_ENTRY(rot_entry, &This->rot, struct rot_entry, entry) { if (rot_entry->cookie == dwRegister) { list_remove(&rot_entry->entry); - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); rot_entry_delete(rot_entry); return S_OK; } } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); return E_INVALIDARG; } @@ -480,7 +480,7 @@ RunningObjectTableImpl_IsRunning( IRunningObjectTable* iface, IMoniker *pmkObjec return hr; hr = S_FALSE; - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); LIST_FOR_EACH_ENTRY(rot_entry, &This->rot, const struct rot_entry, entry) { if ((rot_entry->moniker_data->ulCntData == moniker_data->ulCntData) && @@ -490,7 +490,7 @@ RunningObjectTableImpl_IsRunning( IRunningObjectTable* iface, IMoniker *pmkObjec break; } } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); if (hr == S_FALSE) hr = InternalIrotIsRunning(moniker_data); @@ -533,7 +533,7 @@ RunningObjectTableImpl_GetObject( IRunningObjectTable* iface, if (hr != S_OK) return hr; - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); LIST_FOR_EACH_ENTRY(rot_entry, &This->rot, struct rot_entry, entry) { if ((rot_entry->moniker_data->ulCntData == moniker_data->ulCntData) && @@ -547,13 +547,13 @@ RunningObjectTableImpl_GetObject( IRunningObjectTable* iface, IStream_Release(pStream); } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); HeapFree(GetProcessHeap(), 0, moniker_data); return hr; } } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); TRACE("moniker unavailable locally, calling SCM\n"); @@ -593,20 +593,20 @@ RunningObjectTableImpl_NoteChangeTime(IRunningObjectTable* iface, TRACE("%p, %ld, %p.\n", iface, dwRegister, pfiletime); - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); LIST_FOR_EACH_ENTRY(rot_entry, &This->rot, struct rot_entry, entry) { if (rot_entry->cookie == dwRegister) { rot_entry->last_modified = *pfiletime; - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); hr = InternalIrotNoteChangeTime(dwRegister, pfiletime); goto done; } } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); done: TRACE("-- %#lx\n", hr); @@ -644,7 +644,7 @@ RunningObjectTableImpl_GetTimeOfLastChange(IRunningObjectTable* iface, hr = MK_E_UNAVAILABLE; - EnterCriticalSection(&This->lock); + EnterCriticalSection(This->lock); LIST_FOR_EACH_ENTRY(rot_entry, &This->rot, const struct rot_entry, entry) { if ((rot_entry->moniker_data->ulCntData == moniker_data->ulCntData) && @@ -655,7 +655,7 @@ RunningObjectTableImpl_GetTimeOfLastChange(IRunningObjectTable* iface, break; } } - LeaveCriticalSection(&This->lock); + LeaveCriticalSection(This->lock); if (hr != S_OK) hr = InternalIrotGetTimeOfLastChange(moniker_data, pfiletime); @@ -706,10 +706,20 @@ static const IRunningObjectTableVtbl VT_RunningObjectTableImpl = RunningObjectTableImpl_EnumRunning }; +static RTL_CRITICAL_SECTION IROT_lock; + +static RTL_CRITICAL_SECTION_DEBUG critsect_debug = +{ + 0, 0, &IROT_lock, + { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": RunningObjectTable_section") } +}; +static RTL_CRITICAL_SECTION IROT_lock = { &critsect_debug, -1, 0, 0, 0, 0 }; + static RunningObjectTableImpl rot = { .IRunningObjectTable_iface.lpVtbl = &VT_RunningObjectTableImpl, - .lock.LockCount = -1, + .lock = &IROT_lock, .rot = LIST_INIT(rot.rot), }; @@ -741,13 +751,13 @@ void WINAPI DestroyRunningObjectTable(void) TRACE("\n"); - EnterCriticalSection(&rot.lock); + EnterCriticalSection(rot.lock); LIST_FOR_EACH_ENTRY_SAFE(rot_entry, cursor2, &rot.rot, struct rot_entry, entry) { list_remove(&rot_entry->entry); rot_entry_delete(rot_entry); } - LeaveCriticalSection(&rot.lock); + LeaveCriticalSection(rot.lock); } static HRESULT get_moniker_for_progid_display_name(LPBC pbc, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3372
From: Lorenzo Ferrillo <lorenzofersteam(a)live.it> Sometimes umarshalling an object from the RunningObjectTable can cause the routine to access the table back inside another thread, causing a deadlock. Visual Studio 2019 is an example of this behaviour. Signed-off-by: Lorenzo Ferrillo <lorenzofersteam(a)live.it> --- dlls/ole32/moniker.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ole32/moniker.c b/dlls/ole32/moniker.c index 23465829c07..3a37180eaa9 100644 --- a/dlls/ole32/moniker.c +++ b/dlls/ole32/moniker.c @@ -541,13 +541,13 @@ RunningObjectTableImpl_GetObject( IRunningObjectTable* iface, { IStream *pStream; hr = create_stream_on_mip_ro(rot_entry->object, &pStream); + LeaveCriticalSection(This->lock); if (hr == S_OK) { hr = CoUnmarshalInterface(pStream, &IID_IUnknown, (void **)ppunkObject); IStream_Release(pStream); } - LeaveCriticalSection(This->lock); HeapFree(GetProcessHeap(), 0, moniker_data); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3372
This looks fine to me, except for the first commit. Even if our ole32 is unlikely to run on Windows, using this customization is not great. There might be other opinions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_39780
Hi @nsivov. Doesn't most of wine Critical section use debug infos, even other ole32 CS? Why would this one be different? Or you meant something else (the struct change maybe)? Also before commit 48bd5461ed0d5ba0e7c7e0c5ae5fb782764b2003 ole32: Use single static instance for ROT. this CS did have debug information. If the issue is about the structure change, maybe I can remove it. Understanding the flow that caused this deadlock for VS2019 was quite hard without this debug info, in fact I wasn't sure this was actually IROT related before adding this info for testing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_39805
I don't see a problem with this MR -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_44846
This merge request was approved by Torge Matthies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372
participants (4)
-
Lorenzo Ferrillo -
Lorenzo Ferrillo (＠llde)
-
Nikolay Sivov (＠nsivov)
-
Torge Matthies (＠tmatthies)