[PATCH 0/1] MR750: winecrt0: remove free_delay_imports.

[Kevin Puetz (＠puetzk)]

MSVC's delayimp.lib does not actually free delayload dependencies.
winecrt0's attempt to do so from ELF __attribute__((destructor))
is unnecessary and potentially harmful:

- When triggered naturally via LdrUnloadDll, this leads to recursive calls
  to FreeLibrary, violating free_lib_count and missing DLL_PROCESS_DETACH
- when triggered by glibc's _dl_fini (at process exit), it leads to
  use-after-free of the TEB (GetCurrentThreadID after the main thread is no longer Win32)
  via  FreeLibrary -> LdrLdrUnloadDll -> RtlEnterCriticalSection( &loader_section )
- double-free of the library itself, since the DLL_PROCESS_DETACH has
  already been handled by LdrShutdownProcess
- Race against wineserver sending a SIGKILL from process_killed,
  since all Win32 threads of the process have exited

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53032

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

[PATCH 1/1] winecrt0: remove free_delay_imports.

[Kevin Puetz]

From: Kevin Puetz <PuetzKevinA(a)JohnDeere.com>

MSVC's delayimp.lib does not actually free delayload dependencies.
winecrt0's attempt to do so from ELF __attribute__((destructor))
is unnecessary and potentially harmful:

- When triggered naturally via LdrUnloadDll, this leads to recursive calls
  to FreeLibrary, violating free_lib_count and missing DLL_PROCESS_DETACH
- when triggered by glibc's _dl_fini (at process exit), it leads to
  use-after-free of the TEB (GetCurrentThreadID after the main thread is no longer Win32)
  via  FreeLibrary -> LdrLdrUnloadDll -> RtlEnterCriticalSection( &loader_section )
- double-free of the library itself, since the DLL_PROCESS_DETACH has
  already been handled by LdrShutdownProcess
- Race against wineserver sending a SIGKILL from process_killed,
  since all Win32 threads of the process have exited

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53032
---
 dlls/winecrt0/delay_load.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/dlls/winecrt0/delay_load.c b/dlls/winecrt0/delay_load.c
index 8f28c94d1d5..4ba840479d8 100644
--- a/dlls/winecrt0/delay_load.c
+++ b/dlls/winecrt0/delay_load.c
@@ -69,14 +69,4 @@ FARPROC WINAPI DECLSPEC_HIDDEN __wine_spec_delay_load( unsigned int id )
     return proc;
 }
 
-#if defined(__GNUC__) && !defined(__APPLE__)  /* we can't support destructors properly on Mac OS */
-static void free_delay_imports(void) __attribute__((destructor));
-static void free_delay_imports(void)
-{
-    struct ImgDelayDescr *descr;
-    for (descr = __wine_spec_delay_imports; descr->szName; descr++)
-        if (*descr->phmod) FreeLibrary( *descr->phmod );
-}
-#endif
-
 #endif /* __WINE_PE_BUILD */
-- 
GitLab

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

[Kevin Puetz (＠puetzk)]

See some test code attached in https://bugs.winehq.org/show_bug.cgi?id=53032#c28 that show that FreeLibrary does not unload delayload libraries. But I wasn't really sure how (or if?) to integrate this into winetest; I didn't really see any existing tests for delayload to give me a pattern for unload/exit/"death" tests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/750#note_7343