[PATCH 0/2] MR3372: ole32: Leave the RunningObjectTable Critical Section before unmarshalling object stream

[Lorenzo Ferrillo (＠llde)]

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

[PATCH 1/2] ole32: Add debug info to RunningObjectTable critical section

[Lorenzo Ferrillo]

From: Lorenzo Ferrillo

Signed-off-by: Lorenzo Ferrillo 
---
 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,

[PATCH 2/2] ole32: Leave the RunningObjectTable Critical Section before umarshalling object

[Lorenzo Ferrillo]

From: Lorenzo Ferrillo

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 
---
 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;

[Nikolay Sivov (＠nsivov)]

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.

[Lorenzo Ferrillo (＠llde)]

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.

[Torge Matthies (＠tmatthies)]

I don't see a problem with this MR

[PATCH 0/2] MR3372: ole32: Leave the RunningObjectTable Critical Section before unmarshalling object stream - approved

[Torge Matthies (＠tmatthies)]

This merge request was approved by Torge Matthies.