[PATCH v8 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.

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

[PATCH v8 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 | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/dlls/imm32/imm.c b/dlls/imm32/imm.c
index 780d544c0e9..e7e50eb2016 100644
--- a/dlls/imm32/imm.c
+++ b/dlls/imm32/imm.c
@@ -80,6 +80,9 @@ typedef struct _tagImmHkl{
     DWORD (WINAPI *pImeGetImeMenuItems)(HIMC, DWORD, DWORD, IMEMENUITEMINFOW *, IMEMENUITEMINFOW *, DWORD);
 } ImmHkl;
 
+static HRESULT (WINAPI *pCoRevokeInitializeSpy)(ULARGE_INTEGER cookie);
+static void (WINAPI *pCoUninitialize)(void);
+
 typedef struct tagInputContextData
 {
         HIMC            handle;
@@ -249,7 +252,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 +264,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;
@@ -359,10 +362,19 @@ static const IInitializeSpyVtbl InitializeSpyVtbl =
     InitializeSpy_PostUninitialize,
 };
 
+static BOOL WINAPI init_ole32_funcs( INIT_ONCE *once, void *param, void **context )
+{
+    HMODULE module_ole32 = GetModuleHandleA("ole32");
+    pCoRevokeInitializeSpy = (void*)GetProcAddress(module_ole32, "CoRevokeInitializeSpy");
+    pCoUninitialize = (void*)GetProcAddress(module_ole32, "CoUninitialize");
+    return pCoRevokeInitializeSpy && pCoUninitialize;
+}
+
 static void imm_coinit_thread(void)
 {
     struct coinit_spy *spy;
     HRESULT hr;
+    static INIT_ONCE init_ole32_once = INIT_ONCE_STATIC_INIT;
 
     TRACE("implicit COM initialization\n");
 
@@ -391,6 +403,12 @@ static void imm_coinit_thread(void)
     hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
     if (SUCCEEDED(hr))
         spy->apt_flags |= IMM_APT_CREATED;
+
+    if (!InitOnceExecuteOnce(&init_ole32_once, init_ole32_funcs, NULL, NULL))
+    {
+        ERR("Initialization failed\n");
+        return;
+    }
 }
 
 static BOOL IMM_IsDefaultContext(HIMC imc)
-- 
GitLab

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

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/imm32/imm.c:
> hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED);
>      if (SUCCEEDED(hr))
>          spy->apt_flags |= IMM_APT_CREATED;
> +
> +    if (!InitOnceExecuteOnce(&init_ole32_once, init_ole32_funcs, NULL, NULL))
> +    {
> +        ERR("Initialization failed\n");
> +        return;
> +    }

```suggestion:-4+0
    if (!InitOnceExecuteOnce(&init_ole32_once, init_ole32_funcs, NULL, NULL))
        ERR("Initialization failed\n");
```

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

[Fabian Maurer (＠DarkShadow44)]

On Tue Nov 22 18:45:21 2022 +0000, Piotr Caban wrote:
> ```suggestion:-4+0
>     if (!InitOnceExecuteOnce(&init_ole32_once, init_ole32_funcs, NULL, NULL))
>         ERR("Initialization failed\n");
> ```

Why don't we need to return here? Seems better than a null pointer exception later on.

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

[Piotr Caban (＠piotr)]

On Tue Nov 22 18:47:30 2022 +0000, Fabian Maurer wrote:
> Why don't we need to return here? Seems better than a null pointer
> exception later on.
> It's end of void function, return is not changing anything. Anyway, I think it's safe to assume that GetProcAddress will not fail.

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

[Fabian Maurer (＠DarkShadow44)]

On Tue Nov 22 18:52:39 2022 +0000, Piotr Caban wrote:
> It's end of void function, return is not changing anything. Anyway, I
> think it's safe to assume that GetProcAddress will not fail.
> That's why I initially put it at the start of the function, if spy is NULL then the uninit won't run. But sure, if we can assume GetProcAddress won't ever fail, we can do that.

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