[PATCH v3 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. Also add debug info for this critical section so it show a significant name in the log. Signed-off-by: Lorenzo Ferrillo <lorenzofersteam(a)live.it> -- v3: ole32: Add debug info to RunningObjectTable critical section 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 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/ole32/moniker.c b/dlls/ole32/moniker.c index ec026758b2f..06a2ef8de9c 100644 --- a/dlls/ole32/moniker.c +++ b/dlls/ole32/moniker.c @@ -541,13 +541,14 @@ 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
From: Lorenzo Ferrillo <lorenzofersteam(a)live.it> So it's possible to easily debug deadlocks involving the Internal ROT object Signed-off-by: Lorenzo Ferrillo <lorenzofersteam(a)live.it> --- dlls/ole32/moniker.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dlls/ole32/moniker.c b/dlls/ole32/moniker.c index 06a2ef8de9c..2e55516b6f0 100644 --- a/dlls/ole32/moniker.c +++ b/dlls/ole32/moniker.c @@ -707,10 +707,19 @@ static const IRunningObjectTableVtbl VT_RunningObjectTableImpl = RunningObjectTableImpl_EnumRunning }; +static RunningObjectTableImpl rot; + +static RTL_CRITICAL_SECTION_DEBUG critsect_debug = +{ + 0, 0, &rot.lock, + { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList }, + 0, 0, { (DWORD_PTR)(__FILE__ ": RunningObjectTable_section") } +}; + static RunningObjectTableImpl rot = { .IRunningObjectTable_iface.lpVtbl = &VT_RunningObjectTableImpl, - .lock.LockCount = -1, + .lock = { &critsect_debug, -1, 0, 0, 0, 0 }, .rot = LIST_INIT(rot.rot), }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3372
Hi @tmatthies I rebased with last master and was able to add the debug section without changing the struct to a pointer. Tell me if you prefers this version -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_44850
On Sun Sep 10 16:36:33 2023 +0000, Lorenzo Ferrillo wrote:
Hi @tmatthies I rebased with last master and was able to add the debug section without changing the struct to a pointer. Tell me if you prefers this version Yeah that's a much smaller change, I prefer that
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_44851
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.
It's done elsewhere, e.g. in wined3d, which does consistently work on Windows. Which is not to offer an argument, but at least an observation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3372#note_44925
participants (4)
-
Lorenzo Ferrillo -
Lorenzo Ferrillo (＠llde)
-
Torge Matthies (＠tmatthies)
-
Zebediah Figura (＠zfigura)