[PATCH 0/1] MR9180: win32u: Fix deadlock caused by nested locking.
Within the NtUserUpdateLayeredWindow function, there exists the following call sequence: window_surface_lock()->NtGdiAlphaBlend()->window_surface_unlock(). However, the implementation of the NtGdiAlphaBlend() function calls the windrv_GetImage() function, which internally invokes lock_surface() and unlock_surface(). In this situation, window_surface_lock() is called first to acquire a lock. When lock_surface() is subsequently called to acquire the same lock, the initial lock has not yet been released, ultimately resulting in a deadlock. Signed-off-by: Zhao Yi <zhaoyi(a)uniontech.com> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180
From: Zhao Yi <zhaoyi(a)uniontech.com> Within the NtUserUpdateLayeredWindow function, there exists the following call sequence: window_surface_lock()->NtGdiAlphaBlend()->window_surface_unlock(). However, the implementation of the NtGdiAlphaBlend() function calls the windrv_GetImage() function, which internally invokes lock_surface() and unlock_surface(). In this situation, window_surface_lock() is called first to acquire a lock. When lock_surface() is subsequently called to acquire the same lock, the initial lock has not yet been released, ultimately resulting in a deadlock. Signed-off-by: Zhao Yi <zhaoyi(a)uniontech.com> --- dlls/win32u/window.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dlls/win32u/window.c b/dlls/win32u/window.c index 7e7726f52f9..e420ebd1b39 100644 --- a/dlls/win32u/window.c +++ b/dlls/win32u/window.c @@ -2550,7 +2550,7 @@ BOOL WINAPI NtUserUpdateLayeredWindow( HWND hwnd, HDC hdc_dst, const POINT *pts_ intersect_rect( &rect, &rect, &surface_rect ); if (!(hdc = NtGdiCreateCompatibleDC( 0 ))) goto done; - window_surface_lock( surface ); + if (surface != &dummy_surface) pthread_mutex_lock( &surfaces_lock ); NtGdiSelectBitmap( hdc, surface->color_bitmap ); if (dirty) intersect_rect( &rect, &rect, dirty ); @@ -2566,7 +2566,7 @@ BOOL WINAPI NtUserUpdateLayeredWindow( HWND hwnd, HDC hdc_dst, const POINT *pts_ if (ret) add_bounds_rect( &surface->bounds, &rect ); NtGdiDeleteObjectApp( hdc ); - window_surface_unlock( surface ); + if (surface != &dummy_surface) pthread_mutex_unlock( &surfaces_lock ); if (!(flags & ULW_COLORKEY)) key = CLR_INVALID; window_surface_set_layered( surface, key, -1, 0xff000000 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9180
@rbernon Hi Remi Bernon, Can you help review this fix? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_118818
Rémi Bernon (@rbernon) commented about dlls/win32u/window.c:
intersect_rect( &rect, &rect, &surface_rect );
if (!(hdc = NtGdiCreateCompatibleDC( 0 ))) goto done; - window_surface_lock( surface ); + if (surface != &dummy_surface) pthread_mutex_lock( &surfaces_lock ); NtGdiSelectBitmap( hdc, surface->color_bitmap );
This isn't right, surfaces bitmaps should not be used without locking them. What we should do here, is to detect that the src DC is the windows's DC, in which case it should be unnecessary to create a new one, and unnecessary to lock the surface Something like this maybe, but I'm not completely sure about it: ```suggestion:-2+0 if (NtUserWindowFromDC( hdc_src ) == hwnd) hdc = hdc_src; else { if (!(hdc = NtGdiCreateCompatibleDC( 0 ))) goto done; if (surface != &dummy_surface) pthread_mutex_lock( &surfaces_lock ); NtGdiSelectBitmap( hdc, surface->color_bitmap ); } ``` You would need something similar on the unlock side. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_118839
On Fri Oct 17 06:57:35 2025 +0000, Rémi Bernon wrote:
This isn't right, surfaces bitmaps should not be used without locking them. What we should do here, is to detect that the src DC is the windows's DC, in which case it should be unnecessary to create a new one, and unnecessary to lock the surface Something like this maybe, but I'm not completely sure about it: ```suggestion:-2+0 if (NtUserWindowFromDC( hdc_src ) == hwnd) hdc = hdc_src; else { if (!(hdc = NtGdiCreateCompatibleDC( 0 ))) goto done; if (surface != &dummy_surface) pthread_mutex_lock( &surfaces_lock ); NtGdiSelectBitmap( hdc, surface->color_bitmap ); } ``` You would need something similar on the unlock side. Just wrote a test program as follows: Test1:
HDC hdc = GetDC(hWndA);
BOOL ret = UpdateLayeredWindow(hWndA, NULL, NULL, &si, hdc, &pi, 0, &bf, ULW_ALPHA);
ReleaseDC(hWndA, hdc);
Test2: ``` HDC hdc = CreateCompatibleDC(NULL); BOOL ret = UpdateLayeredWindow(hWndA, NULL, NULL, &si, hdc, &pi, 0, &bf, ULW_ALPHA); ReleaseDC(NULL, hdc); ``` The test results confirm what you said: The window DC created in Test1 doesn't unnecessary to lock the surface, and calling window_surface_lock would cause a deadlock. The compatible DC created in Test2 requires locking. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_118858
On Fri Oct 17 08:35:05 2025 +0000, Zhao Yi wrote:
Just wrote a test program as follows: Test1: ``` HDC hdc = GetDC(hWndA); BOOL ret = UpdateLayeredWindow(hWndA, NULL, NULL, &si, hdc, &pi, 0, &bf, ULW_ALPHA); ReleaseDC(hWndA, hdc); ``` Test2: ``` HDC hdc = CreateCompatibleDC(NULL); BOOL ret = UpdateLayeredWindow(hWndA, NULL, NULL, &si, hdc, &pi, 0, &bf, ULW_ALPHA); ReleaseDC(NULL, hdc); ``` The test results confirm what you said: The window DC created in Test1 doesn't unnecessary to lock the surface, and calling window_surface_lock would cause a deadlock.
The compatible DC created in Test2 requires locking. After applying the changes according to the patch file, the call to the UpdateLayeredWindow function failed. I will continue to analyze the cause. [fix-deadlock.patch](/uploads/51708682a6c0d42328d07e5a05bb6468/fix-deadlock.patch)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_118866
I reviewed the previous version of the UpdateLayeredWindow function's implementation and noticed that it did not handle the case where the source DC is a window DC separately. Instead, it applied locking uniformly. ``` BOOL X11DRV_UpdateLayeredWindow( HWND hwnd, const UPDATELAYEREDWINDOWINFO *info, const RECT *window_rect ) { struct window_surface *surface; struct x11drv_win_data *data; BLENDFUNCTION blend = { AC_SRC_OVER, 0, 255, 0 }; COLORREF color_key = (info->dwFlags & ULW_COLORKEY) ? info->crKey : CLR_INVALID; char buffer[FIELD_OFFSET( BITMAPINFO, bmiColors[256] )]; BITMAPINFO *bmi = (BITMAPINFO *)buffer; void *src_bits, *dst_bits; RECT rect, src_rect; HDC hdc = 0; HBITMAP dib; BOOL mapped, ret = FALSE; if (!(data = get_win_data( hwnd ))) return FALSE; data->layered = TRUE; if (!data->embedded && argb_visual.visualid) set_window_visual( data, &argb_visual, TRUE ); rect = *window_rect; OffsetRect( &rect, -window_rect->left, -window_rect->top ); surface = data->surface; if (!surface || !EqualRect( &surface->rect, &rect )) { data->surface = create_surface( data->whole_window, &data->vis, &rect, color_key, data->use_alpha ); if (surface) window_surface_release( surface ); surface = data->surface; surface->is_trayicon = is_trayicon(hwnd); } else set_surface_color_key( surface, color_key ); if (surface) window_surface_add_ref( surface ); mapped = data->mapped; release_win_data( data ); /* layered windows are mapped only once their attributes are set */ if (!mapped) { DWORD style = NtUserGetWindowLongW( hwnd, GWL_STYLE ); if ((style & WS_VISIBLE) && ((style & WS_MINIMIZE) || is_window_rect_mapped( window_rect ))) map_window( hwnd, style, 0 ); } if (!surface) return FALSE; if (!info->hdcSrc) { window_surface_release( surface ); return TRUE; } dst_bits = surface->funcs->get_info( surface, bmi ); if (!(dib = NtGdiCreateDIBSection( info->hdcDst, NULL, 0, bmi, DIB_RGB_COLORS, 0, 0, 0, &src_bits ))) goto done; if (!(hdc = NtGdiCreateCompatibleDC( 0 ))) goto done; NtGdiSelectBitmap( hdc, dib ); surface->funcs->lock( surface ); if (info->prcDirty) { intersect_rect( &rect, &rect, info->prcDirty ); memcpy( src_bits, dst_bits, bmi->bmiHeader.biSizeImage ); NtGdiPatBlt( hdc, rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, BLACKNESS ); } src_rect = rect; if (info->pptSrc) OffsetRect( &src_rect, info->pptSrc->x, info->pptSrc->y ); NtGdiTransformPoints( info->hdcSrc, (POINT *)&src_rect, (POINT *)&src_rect, 2, NtGdiDPtoLP ); if (info->dwFlags & ULW_ALPHA) blend = *info->pblend; ret = NtGdiAlphaBlend( hdc, rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, info->hdcSrc, src_rect.left, src_rect.top, src_rect.right - src_rect.left, src_rect.bottom - src_rect.top, *(DWORD *)&blend, 0 ); if (ret) { memcpy( dst_bits, src_bits, bmi->bmiHeader.biSizeImage ); add_bounds_rect( surface->funcs->get_bounds( surface ), &rect ); } surface->funcs->unlock( surface ); surface->funcs->flush( surface ); done: window_surface_release( surface ); if (hdc) NtGdiDeleteObjectApp( hdc ); if (dib) NtGdiDeleteObjectApp( dib ); return ret; } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_119028
@jacek Hi jacek, I saw in the code records that you previously maintained the UpdateLayeredWindow function. Could you help review this fix? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9180#note_119029
participants (3)
-
Rémi Bernon -
Zhao Yi
-
Zhao Yi (＠Zhaoyi)