[PATCH v4 0/1] MR1456: imm32: Prevent deadlock on unload

[Fabian Maurer (＠DarkShadow44)]

We need to make sure all (important) cleanup is finished when we exit DllMain,
otherwise we might already unload krnl386 and deadlock
Since we can't have a synchronous DestroyWindow, use an extra message

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52511
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>


Code flow is like follows:

- DOSVM_Exit -> RtlExitUserThread -> LdrShutdownThread
- acquire loader lock
- send DLL_THREAD_DETACH to imm32
- - calls DestroyWindow on its window
- - WM_DESTROY is handled asynchonously!
- send DLL_THREAD_DETACH to krnl386.exe 
- - TASK_ExitTask
- -  try get win16 lock, is blocked by ime32

meanwhile, at ime32

- handles WM_DESTROY
- already have win16 lock from user32
- __wine_ime_wnd_proc (WM_DESTROY) -> imm_couninit_thread -> CoRevokeInitializeSpy
- LdrGetProcedureAddress (want CoRevokeInitializeSpy)
- try get loader_lock, is blocked

Solution is to wait in ime32 DllMain until the cleanup as happened, aka CoRevokeInitializeSpy has finished.

--
  v4: imm32: Prevent deadlock on unload
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1456

[PATCH v4 1/1] imm32: Prevent deadlock on unload

[Fabian Maurer]

From: Fabian Maurer <dark.shadow4(a)web.de>

ole32 is delay loaded, which can cause race conditions against
krnl386.exe when unloading.
This is because imm32 tries to get the loader lock while holding the win16 mutex,
and kernel386 does the opposite.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52511
Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>
---
 dlls/imm32/imm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/dlls/imm32/imm.c b/dlls/imm32/imm.c
index 780d544c0e9..9b3ccdaadea 100644
--- a/dlls/imm32/imm.c
+++ b/dlls/imm32/imm.c
@@ -80,6 +80,10 @@ typedef struct _tagImmHkl{
     DWORD (WINAPI *pImeGetImeMenuItems)(HIMC, DWORD, DWORD, IMEMENUITEMINFOW *, IMEMENUITEMINFOW *, DWORD);
 } ImmHkl;
 
+static HMODULE module_ole32;
+static HRESULT WINAPI (*pCoRevokeInitializeSpy)(ULARGE_INTEGER cookie);
+static void WINAPI (*pCoUninitialize)(void);
+
 typedef struct tagInputContextData
 {
         HIMC            handle;
@@ -249,7 +253,7 @@ static void imm_couninit_thread(BOOL cleanup)
 
     if (cleanup && spy->cookie.QuadPart)
     {
-        CoRevokeInitializeSpy(spy->cookie);
+        pCoRevokeInitializeSpy(spy->cookie);
         spy->cookie.QuadPart = 0;
     }
 
@@ -261,7 +265,7 @@ static void imm_couninit_thread(BOOL cleanup)
     {
         spy->apt_flags &= ~IMM_APT_CREATED;
         if (spy->apt_flags & IMM_APT_CAN_FREE)
-            CoUninitialize();
+            pCoUninitialize();
     }
     if (cleanup)
         spy->apt_flags = 0;
@@ -565,6 +569,9 @@ BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID lpReserved)
     switch (fdwReason)
     {
         case DLL_PROCESS_ATTACH:
+            module_ole32 = LoadLibraryA("ole32");
+            pCoRevokeInitializeSpy = (void*)GetProcAddress(module_ole32, "CoRevokeInitializeSpy");
+            pCoUninitialize = (void*)GetProcAddress(module_ole32, "CoUninitialize");
             if (!User32InitializeImmEntryTable(IMM_INIT_MAGIC))
             {
                 return FALSE;
@@ -576,6 +583,7 @@ BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID lpReserved)
             IMM_FreeThreadData();
             break;
         case DLL_PROCESS_DETACH:
+            FreeLibrary(module_ole32);
             if (lpReserved) break;
             IMM_FreeThreadData();
             IMM_FreeAllImmHkl();
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/1456

[PATCH v4 1/1] imm32: Prevent deadlock on unload

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=126517

Your paranoid android.


=== debian11 (32 bit report) ===

kernel32:
debugger.c:1027: Test failed: ole32.dll was not reported

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/imm32/imm.c:
> switch (fdwReason)
>      {
>          case DLL_PROCESS_ATTACH:
> +            module_ole32 = LoadLibraryA("ole32");
> +            pCoRevokeInitializeSpy = (void*)GetProcAddress(module_ole32, "CoRevokeInitializeSpy");
> +            pCoUninitialize = (void*)GetProcAddress(module_ole32, "CoUninitialize");

The initialization should be done in imm_coinit_thread function. You may also consider using GetModuleHandle instead of LoadLibraryA so you don't have to explicitly free the module.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/1456#note_16703