[PATCH 0/1] MR7306: winex11.drv: Don't call NtUserGetWindowLongW() from X11DRV_GetWindowStateUpdates() with win_data_mutex held.

[Paul Gofman (＠gofman)]

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

[PATCH 1/1] winex11.drv: Don't call NtUserGetWindowLongW() from X11DRV_GetWindowStateUpdates() with win_data_mutex held.

[Paul Gofman]

From: Paul Gofman <pgofman(a)codeweavers.com>

---
 dlls/winex11.drv/window.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
index b033960fc26..401749c69e9 100644
--- a/dlls/winex11.drv/window.c
+++ b/dlls/winex11.drv/window.c
@@ -1524,9 +1524,9 @@ static void unmap_window( HWND hwnd )
     release_win_data( data );
 }
 
-static UINT window_update_client_state( struct x11drv_win_data *data )
+static UINT window_update_client_state( UINT old_style, struct x11drv_win_data *data )
 {
-    UINT old_style = NtUserGetWindowLongW( data->hwnd, GWL_STYLE ), new_style;
+    UINT new_style;
 
     if (!data->managed) return 0; /* unmanaged windows are managed by the Win32 side */
     if (data->desired_state.wm_state == WithdrawnState) return 0; /* ignore state changes on invisible windows */
@@ -1569,11 +1569,11 @@ static UINT window_update_client_state( struct x11drv_win_data *data )
     return 0;
 }
 
-static UINT window_update_client_config( struct x11drv_win_data *data )
+static UINT window_update_client_config( UINT old_style, struct x11drv_win_data *data )
 {
     static const UINT fullscreen_mask = (1 << NET_WM_STATE_MAXIMIZED) | (1 << NET_WM_STATE_FULLSCREEN);
-    UINT old_style = NtUserGetWindowLongW( data->hwnd, GWL_STYLE ), flags;
     RECT rect, old_rect = data->rects.window, new_rect;
+    UINT flags;
 
     if (!data->managed) return 0; /* unmanaged windows are managed by the Win32 side */
     if (data->desired_state.wm_state != NormalState) return 0; /* ignore config changes on invisible/minimized windows */
@@ -1621,12 +1621,13 @@ static UINT window_update_client_config( struct x11drv_win_data *data )
  */
 BOOL X11DRV_GetWindowStateUpdates( HWND hwnd, UINT *state_cmd, UINT *config_cmd, RECT *rect )
 {
+    UINT old_style = NtUserGetWindowLongW( hwnd, GWL_STYLE );
     struct x11drv_win_data *data;
 
     if (!(data = get_win_data( hwnd ))) return FALSE;
 
-    *state_cmd = window_update_client_state( data );
-    *config_cmd = window_update_client_config( data );
+    *state_cmd = window_update_client_state( old_style, data );
+    *config_cmd = window_update_client_config( old_style, data );
     *rect = window_rect_from_visible( &data->rects, data->current_state.rect );
 
     release_win_data( data );
-- 
GitLab

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

[Paul Gofman (＠gofman)]

NtUserGetWindowLongW() gets win32u user_lock (for getting window data).

I am only reproducing the real deadlock with some tentative patches and not sure if that can happen without those. Yet it seems to me it is best to avoid nested locking between winex11's win data mutex and win32u's user lock, at least with winex11 locks taken first. In this place nesting the locks looks easily avoidable.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94220

[Rémi Bernon]

I don't think win32u should call into the drivers with the user lock held.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94238

[Paul Gofman (＠gofman)]

One place I know of where that currently happens is from win32u.update_surface_region (which holds win_ptr throughout the function) -> window_surface_set_clip() -> surface->funcs->set_clip().

So we should probably be fixing that one instead?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94266

[Rémi Bernon]

Possible, though I rather meant the user_driver functions shouldn't be called with user_lock.

The window surface calls might be different, because I suspect it's more likely to be called while holding a win32u win ptr, but then the rule would rather be the other way around, with window surface functions should perhaps not call into win32u. I'm not sure it's as well respected there though.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94282

[Paul Gofman (＠gofman)]

Window surface functions maybe don't call into win32u, what I tried to do is to use x11 window data from surface functions (in particular, set_clip), that is enough to randomly trigger a deadlock (because of the path concerned in this MR which takes those locks in a different order). Not sure, maybe it is a moot point until a code which is doing that submitted.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_94284

[PATCH 0/1] MR7306: winex11.drv: Don't call NtUserGetWindowLongW() from X11DRV_GetWindowStateUpdates() with win_data_mutex held. - closed

[Rémi Bernon]

This merge request was closed by Rémi Bernon.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306

[Rémi Bernon]

Closing as this isn't the right way, win32u simply shouldn't call into the user driver with user_lock held, it breaks many assumptions a bit everywhere.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7306#note_97081