[PATCH 1/2] win32u: Guard font unused_entry against race condition.
There is a race condition otherwise between release_gdi_font and find_cached_gdi_font, leading to invalid memory access: One thread calling release_gdi_font may decrement refcount to 0, then try to enter font_lock. At the same time, another thread may be calling find_cached_gdi_font through select_font, holding the font_lock. This second thread would find refcount set to 0, and then try to remove unused_entry from its list, although it hasn't been added yet to the unused list. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- The crash can be easily reproduced by running a simple program which only creates a window then exits, in a loop, shutting down the prefix every time. It randomly causes a segmentation fault. For some reason, this race condition is only present after fd675485be8686ec5e3b4667d3f3e9739a4a07ce, possibly because of the regression described in next patch. dlls/win32u/font.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/dlls/win32u/font.c b/dlls/win32u/font.c index d2f61526540..f9a0359e92c 100644 --- a/dlls/win32u/font.c +++ b/dlls/win32u/font.c @@ -2576,22 +2576,24 @@ static struct gdi_font *find_cached_gdi_font( const LOGFONTW *lf, const FMAT2 *m static void release_gdi_font( struct gdi_font *font ) { if (!font) return; - if (--font->refcount) return; TRACE( "font %p\n", font ); /* add it to the unused list */ pthread_mutex_lock( &font_lock ); - list_add_head( &unused_gdi_font_list, &font->unused_entry ); - if (unused_font_count > UNUSED_CACHE_SIZE) + if (!--font->refcount) { - font = LIST_ENTRY( list_tail( &unused_gdi_font_list ), struct gdi_font, unused_entry ); - TRACE( "freeing %p\n", font ); - list_remove( &font->entry ); - list_remove( &font->unused_entry ); - free_gdi_font( font ); + list_add_head( &unused_gdi_font_list, &font->unused_entry ); + if (unused_font_count > UNUSED_CACHE_SIZE) + { + font = LIST_ENTRY( list_tail( &unused_gdi_font_list ), struct gdi_font, unused_entry ); + TRACE( "freeing %p\n", font ); + list_remove( &font->entry ); + list_remove( &font->unused_entry ); + free_gdi_font( font ); + } + else unused_font_count++; } - else unused_font_count++; pthread_mutex_unlock( &font_lock ); } -- 2.33.1
Since fd675485be8686ec5e3b4667d3f3e9739a4a07ce, the graphics driver initialization sequence changed for the desktop process. The desktop thread loads the graphics driver directly, and that makes it fill the user driver callbacks without going through load_driver anymore. This makes register_builtin_classes to not be called anymore in the desktop process, and IME window creation to fail when the systray window is created. It is noticeable as ImeSetActiveContext and ImmReleaseContext are printing a FIXME, and possible side effects aren't completely obvious, but we should probably make sure builtin classes are registered. This restores a call to register_builtin_classes by redirecting graphics driver wine_create_desktop callback through user32. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/driver.c | 21 ++++++++++++++++++++- dlls/user32/user32.spec | 1 + dlls/wineandroid.drv/android.h | 1 + dlls/wineandroid.drv/init.c | 1 + dlls/wineandroid.drv/window.c | 4 ++-- dlls/wineandroid.drv/wineandroid.drv.spec | 3 --- dlls/winex11.drv/desktop.c | 4 ++-- dlls/winex11.drv/init.c | 1 + dlls/winex11.drv/winex11.drv.spec | 3 --- dlls/winex11.drv/x11drv.h | 1 + include/wine/gdi_driver.h | 3 +++ programs/explorer/desktop.c | 17 ++--------------- 12 files changed, 34 insertions(+), 26 deletions(-) diff --git a/dlls/user32/driver.c b/dlls/user32/driver.c index 06b02adeab5..31443643d5c 100644 --- a/dlls/user32/driver.c +++ b/dlls/user32/driver.c @@ -216,6 +216,11 @@ static void CDECL nulldrv_ThreadDetach( void ) { } +static BOOL CDECL nulldrv_CreateDesktop( UINT width, UINT heigth ) +{ + return FALSE; +} + /********************************************************************** * Lazy loading user driver @@ -374,7 +379,9 @@ static struct user_driver_funcs lazy_load_driver = /* system parameters */ nulldrv_SystemParametersInfo, /* thread management */ - nulldrv_ThreadDetach + nulldrv_ThreadDetach, + /* desktop creation */ + nulldrv_CreateDesktop, }; void CDECL __wine_set_user_driver( const struct user_driver_funcs *funcs, UINT version ) @@ -408,3 +415,15 @@ void CDECL __wine_set_user_driver( const struct user_driver_funcs *funcs, UINT v __wine_set_display_driver( driver, version ); } + +BOOL CDECL __wine_set_desktop( HMODULE module, const WCHAR *name, UINT width, UINT height, HDESK desktop ) +{ + /* we're in the desktop thread already but we need to set the ready + * flag so that later user32 calls won't trigger a SendMessageW while + * holding the user lock when registering classes. */ + wait_graphics_driver_ready(); + + register_builtin_classes(); + if (!desktop) return FALSE; + return USER_Driver->pCreateDesktop( width, height ); +} diff --git a/dlls/user32/user32.spec b/dlls/user32/user32.spec index 78946be86b3..77952d84489 100644 --- a/dlls/user32/user32.spec +++ b/dlls/user32/user32.spec @@ -839,3 +839,4 @@ @ cdecl __wine_send_input(long ptr ptr) @ cdecl __wine_set_pixel_format(long long) @ cdecl __wine_set_user_driver(ptr long) +@ cdecl __wine_set_desktop(ptr ptr long long ptr) diff --git a/dlls/wineandroid.drv/android.h b/dlls/wineandroid.drv/android.h index 73ac0e702cc..e0f3c760c83 100644 --- a/dlls/wineandroid.drv/android.h +++ b/dlls/wineandroid.drv/android.h @@ -106,6 +106,7 @@ extern void CDECL ANDROID_WindowPosChanged( HWND hwnd, HWND insert_after, UINT s const RECT *window_rect, const RECT *client_rect, const RECT *visible_rect, const RECT *valid_rects, struct window_surface *surface ) DECLSPEC_HIDDEN; +extern BOOL CDECL ANDROID_CreateDesktop( UINT width, UINT height ) DECLSPEC_HIDDEN; extern unsigned int screen_width DECLSPEC_HIDDEN; extern unsigned int screen_height DECLSPEC_HIDDEN; diff --git a/dlls/wineandroid.drv/init.c b/dlls/wineandroid.drv/init.c index 180a6650ffd..bdfbec60c0e 100644 --- a/dlls/wineandroid.drv/init.c +++ b/dlls/wineandroid.drv/init.c @@ -310,6 +310,7 @@ static const struct user_driver_funcs android_drv_funcs = .pWindowMessage = ANDROID_WindowMessage, .pWindowPosChanging = ANDROID_WindowPosChanging, .pWindowPosChanged = ANDROID_WindowPosChanged, + .pCreateDesktop = ANDROID_CreateDesktop, }; diff --git a/dlls/wineandroid.drv/window.c b/dlls/wineandroid.drv/window.c index 997bce15964..a5ae628d8a0 100644 --- a/dlls/wineandroid.drv/window.c +++ b/dlls/wineandroid.drv/window.c @@ -1651,9 +1651,9 @@ LRESULT CDECL ANDROID_WindowMessage( HWND hwnd, UINT msg, WPARAM wp, LPARAM lp ) /*********************************************************************** - * ANDROID_create_desktop + * ANDROID_CreateDesktop */ -BOOL CDECL ANDROID_create_desktop( UINT width, UINT height ) +BOOL CDECL ANDROID_CreateDesktop( UINT width, UINT height ) { desktop_orig_wndproc = (WNDPROC)SetWindowLongPtrW( GetDesktopWindow(), GWLP_WNDPROC, (LONG_PTR)desktop_wndproc_wrapper ); diff --git a/dlls/wineandroid.drv/wineandroid.drv.spec b/dlls/wineandroid.drv/wineandroid.drv.spec index 2d7e76a04e8..612bf4634be 100644 --- a/dlls/wineandroid.drv/wineandroid.drv.spec +++ b/dlls/wineandroid.drv/wineandroid.drv.spec @@ -1,6 +1,3 @@ -# Desktop -@ cdecl wine_create_desktop(long long) ANDROID_create_desktop - # MMDevAPI driver functions @ stdcall -private GetPriority() AUDDRV_GetPriority @ stdcall -private GetEndpointIDs(long ptr ptr ptr ptr) AUDDRV_GetEndpointIDs diff --git a/dlls/winex11.drv/desktop.c b/dlls/winex11.drv/desktop.c index 71b3a0a5a27..617efb1a7a3 100644 --- a/dlls/winex11.drv/desktop.c +++ b/dlls/winex11.drv/desktop.c @@ -322,11 +322,11 @@ void X11DRV_init_desktop( Window win, unsigned int width, unsigned int height ) /*********************************************************************** - * X11DRV_create_desktop + * X11DRV_CreateDesktop * * Create the X11 desktop window for the desktop mode. */ -BOOL CDECL X11DRV_create_desktop( UINT width, UINT height ) +BOOL CDECL X11DRV_CreateDesktop( UINT width, UINT height ) { static const WCHAR rootW[] = {'r','o','o','t',0}; XSetWindowAttributes win_attr; diff --git a/dlls/winex11.drv/init.c b/dlls/winex11.drv/init.c index 854221bf948..32047e7aeac 100644 --- a/dlls/winex11.drv/init.c +++ b/dlls/winex11.drv/init.c @@ -426,6 +426,7 @@ static const struct user_driver_funcs x11drv_funcs = .pWindowPosChanged = X11DRV_WindowPosChanged, .pSystemParametersInfo = X11DRV_SystemParametersInfo, .pThreadDetach = X11DRV_ThreadDetach, + .pCreateDesktop = X11DRV_CreateDesktop, }; diff --git a/dlls/winex11.drv/winex11.drv.spec b/dlls/winex11.drv/winex11.drv.spec index 6e0ccfab4b0..d08e2e71e76 100644 --- a/dlls/winex11.drv/winex11.drv.spec +++ b/dlls/winex11.drv/winex11.drv.spec @@ -4,9 +4,6 @@ @ cdecl LoadTabletInfo(long) X11DRV_LoadTabletInfo @ cdecl WTInfoW(long long ptr) X11DRV_WTInfoW -# Desktop -@ cdecl wine_create_desktop(long long) X11DRV_create_desktop - # System tray @ cdecl wine_notify_icon(long ptr) diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 4b4ee8daaeb..43c02450806 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -241,6 +241,7 @@ extern void CDECL X11DRV_WindowPosChanged( HWND hwnd, HWND insert_after, UINT sw extern BOOL CDECL X11DRV_SystemParametersInfo( UINT action, UINT int_param, void *ptr_param, UINT flags ) DECLSPEC_HIDDEN; extern void CDECL X11DRV_ThreadDetach(void) DECLSPEC_HIDDEN; +extern BOOL CDECL X11DRV_CreateDesktop( UINT width, UINT height ) DECLSPEC_HIDDEN; /* X11 driver internal functions */ diff --git a/include/wine/gdi_driver.h b/include/wine/gdi_driver.h index 994b082d5b4..16337eb6235 100644 --- a/include/wine/gdi_driver.h +++ b/include/wine/gdi_driver.h @@ -287,9 +287,12 @@ struct user_driver_funcs BOOL (CDECL *pSystemParametersInfo)(UINT,UINT,void*,UINT); /* thread management */ void (CDECL *pThreadDetach)(void); + /* desktop creation */ + BOOL (CDECL *pCreateDesktop)(UINT,UINT); }; extern void CDECL __wine_set_user_driver( const struct user_driver_funcs *funcs, UINT version ); +extern BOOL CDECL __wine_set_desktop( HMODULE module, const WCHAR *name, UINT width, UINT height, HDESK desktop ); /* the DC hook support is only exported on Win16, the 32-bit version is a Wine extension */ diff --git a/programs/explorer/desktop.c b/programs/explorer/desktop.c index 6d577dd0fd1..5ace15c73b0 100644 --- a/programs/explorer/desktop.c +++ b/programs/explorer/desktop.c @@ -32,6 +32,7 @@ #include "exdisp.h" #include "wine/debug.h" +#include "wine/gdi_driver.h" #include "explorer_private.h" WINE_DEFAULT_DEBUG_CHANNEL(explorer); @@ -684,20 +685,6 @@ static LRESULT WINAPI desktop_wnd_proc( HWND hwnd, UINT message, WPARAM wp, LPAR return desktop_orig_wndproc( hwnd, message, wp, lp ); } -/* create the desktop and the associated driver window, and make it the current desktop */ -static BOOL create_desktop( HMODULE driver, const WCHAR *name, unsigned int width, unsigned int height ) -{ - BOOL ret = FALSE; - BOOL (CDECL *create_desktop_func)(unsigned int, unsigned int); - - if (driver) - { - create_desktop_func = (void *)GetProcAddress( driver, "wine_create_desktop" ); - if (create_desktop_func) ret = create_desktop_func( width, height ); - } - return ret; -} - /* parse the desktop size specification */ static BOOL parse_size( const WCHAR *size, unsigned int *width, unsigned int *height ) { @@ -1019,7 +1006,7 @@ void manage_desktop( WCHAR *arg ) desktop_orig_wndproc = (WNDPROC)SetWindowLongPtrW( hwnd, GWLP_WNDPROC, (LONG_PTR)desktop_wnd_proc ); - using_root = !desktop || !create_desktop( graphics_driver, name, width, height ); + using_root = !__wine_set_desktop( graphics_driver, name, width, height, desktop ); SendMessageW( hwnd, WM_SETICON, ICON_BIG, (LPARAM)LoadIconW( 0, MAKEINTRESOURCEW(OIC_WINLOGO))); if (name) set_desktop_window_title( hwnd, name ); SetWindowPos( hwnd, 0, GetSystemMetrics(SM_XVIRTUALSCREEN), GetSystemMetrics(SM_YVIRTUALSCREEN), -- 2.33.1
Hi Rémi, On 11/19/21 12:23 PM, Rémi Bernon wrote:
This makes register_builtin_classes to not be called anymore in the desktop process, and IME window creation to fail when the systray window is created.
Thanks for looking at it. I was planning to send the attached patch after a bit more testing, I'd expect it to solve the problem that you noticed. Jacek
On 11/19/21 13:17, Jacek Caban wrote:
Hi Rémi,
On 11/19/21 12:23 PM, Rémi Bernon wrote:
This makes register_builtin_classes to not be called anymore in the desktop process, and IME window creation to fail when the systray window is created.
Thanks for looking at it. I was planning to send the attached patch after a bit more testing, I'd expect it to solve the problem that you noticed.
Jacek
Yes, looks like it works as well, but I'm a bit afraid that doing the builtin class registration in GetDesktopWindow is a bet that it'll be called by the desktop process and called soon enough, which I'm not sure we can have any guarantee about. I suppose that the wine_create_desktop call will have to be changed at some point anyway, so it doesn't look completely wrong to use it to notify of desktop initialization completion. This can probably be done later too. The change to CreateCursor then makes calling wait_graphics_driver_ready also unnecessary, but there too I'm a bit afraid that we could get some convoluted call chain to it somehow too, and calling it explicitly when desktop is initialized also seems safer. Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>
On 11/19/21 1:46 PM, Rémi Bernon wrote:
On 11/19/21 13:17, Jacek Caban wrote:
Hi Rémi,
On 11/19/21 12:23 PM, Rémi Bernon wrote:
This makes register_builtin_classes to not be called anymore in the desktop process, and IME window creation to fail when the systray window is created.
Thanks for looking at it. I was planning to send the attached patch after a bit more testing, I'd expect it to solve the problem that you noticed.
Jacek
Yes, looks like it works as well, but I'm a bit afraid that doing the builtin class registration in GetDesktopWindow is a bet that it'll be called by the desktop process and called soon enough, which I'm not sure we can have any guarantee about.
I sent a bit different version of the patch that adds another call that will make sure that it's called soon enough in explorer process and removed the call from load_driver() entirely. Note that there are other, not desktop related, cases that we should handle as well. For example wineandroid.drv is also a sounds driver, so it may be loaded without user32/win32u involvement. Thanks, Jacek
participants (3)
-
Huw Davies -
Jacek Caban -
Rémi Bernon