From 78fd7f580d5f9e6d9d582d82e5ea96003844fbdf Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Fri, 27 Feb 2026 08:41:07 +0100 Subject: [PATCH] [client,x11] improve rails window locking * Add unified lock/unlock functions to allow easier tracing * Fix a few locking issues found during debugging * Add an assertion triggering when a lock is locked/unlocked twice --- client/X11/xf_event.c | 3 +- client/X11/xf_graphics.c | 4 +-- client/X11/xf_rail.c | 46 +++++++++++--------------- client/X11/xf_rail.h | 12 +++++-- client/X11/xf_window.c | 71 ++++++++++++++++++++++++++++++++++++---- client/X11/xf_window.h | 13 ++++++-- client/X11/xfreerdp.h | 4 +++ 7 files changed, 111 insertions(+), 42 deletions(-) diff --git a/client/X11/xf_event.c b/client/X11/xf_event.c index 4d6c542f2..3dc7b702f 100644 --- a/client/X11/xf_event.c +++ b/client/X11/xf_event.c @@ -410,9 +410,8 @@ static BOOL xf_event_Expose(xfContext* xfc, const XExposeEvent* event, BOOL app) { xfAppWindow* appWindow = xf_AppWindowFromX11Window(xfc, event->window); if (appWindow) - { xf_UpdateWindowArea(xfc, appWindow, x, y, w, h); - } + xf_rail_return_window(appWindow); } return TRUE; diff --git a/client/X11/xf_graphics.c b/client/X11/xf_graphics.c index 554d68834..7be34d88e 100644 --- a/client/X11/xf_graphics.c +++ b/client/X11/xf_graphics.c @@ -251,12 +251,12 @@ static Window xf_Pointer_get_window(xfContext* xfc) if (xfc->remote_app) { Window w = 0; - HashTable_Lock(xfc->railWindows); + xf_AppWindowsLock(xfc); if (!xfc->appWindow) WLog_WARN(TAG, "xf_Pointer: Invalid appWindow"); else w = xfc->appWindow->handle; - HashTable_Unlock(xfc->railWindows); + xf_AppWindowsUnlock(xfc); return w; } else diff --git a/client/X11/xf_rail.c b/client/X11/xf_rail.c index 82b52e9d1..416b2825c 100644 --- a/client/X11/xf_rail.c +++ b/client/X11/xf_rail.c @@ -159,9 +159,10 @@ BOOL xf_rail_send_activate(xfContext* xfc, Window xwindow, BOOL enabled) WINPR_ASSERT(appWindow->windowId <= UINT32_MAX); activate.windowId = (UINT32)appWindow->windowId; + xf_rail_return_window(appWindow); + activate.enabled = enabled; const UINT rc = xfc->rail->ClientActivate(xfc->rail, &activate); - xf_rail_return_window(appWindow); return rc == CHANNEL_RC_OK; } @@ -381,6 +382,7 @@ static void window_state_log_style_int(wLog* log, const WINDOW_STATE_ORDER* wind static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* orderInfo, const WINDOW_STATE_ORDER* windowState) { + BOOL rc = FALSE; xfContext* xfc = (xfContext*)context; WINPR_ASSERT(xfc); @@ -440,7 +442,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* } if (!appWindow->title) - return FALSE; + goto fail; xf_AppWindowInit(xfc, appWindow); } @@ -518,14 +520,14 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* if (!(title = _strdup(""))) { WLog_ERR(TAG, "failed to duplicate empty window title string"); - return FALSE; + goto fail; } } else if (!(title = ConvertWCharNToUtf8Alloc( cnv.wc, windowState->titleInfo.length / sizeof(WCHAR), nullptr))) { WLog_ERR(TAG, "failed to convert window title"); - return FALSE; + goto fail; } free(appWindow->title); @@ -566,7 +568,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* (RECTANGLE_16*)calloc(appWindow->numWindowRects, sizeof(RECTANGLE_16)); if (!appWindow->windowRects) - return FALSE; + goto fail; CopyMemory(appWindow->windowRects, windowState->windowRects, appWindow->numWindowRects * sizeof(RECTANGLE_16)); @@ -595,7 +597,7 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* (RECTANGLE_16*)calloc(appWindow->numVisibilityRects, sizeof(RECTANGLE_16)); if (!appWindow->visibilityRects) - return FALSE; + goto fail; CopyMemory(appWindow->visibilityRects, windowState->visibilityRects, appWindow->numVisibilityRects * sizeof(RECTANGLE_16)); @@ -675,7 +677,10 @@ static BOOL xf_rail_window_common(rdpContext* context, const WINDOW_ORDER_INFO* { xf_SetWindowRects(xfc, appWindow, appWindow->windowRects, appWindow->numWindowRects); }*/ - return TRUE; + rc = TRUE; +fail: + xf_rail_return_window(appWindow); + return rc; } static BOOL xf_rail_window_delete(rdpContext* context, const WINDOW_ORDER_INFO* orderInfo) @@ -1186,14 +1191,14 @@ static UINT xf_rail_server_local_move_size(RailClientContext* context, x = localMoveSize->posX; y = localMoveSize->posY; /* FIXME: local keyboard moves not working */ - return CHANNEL_RC_OK; + break; case RAIL_WMSZ_KEYSIZE: direction = NET_WM_MOVERESIZE_SIZE_KEYBOARD; x = localMoveSize->posX; y = localMoveSize->posY; /* FIXME: local keyboard moves not working */ - return CHANNEL_RC_OK; + break; default: break; } @@ -1375,13 +1380,16 @@ xfAppWindow* xf_rail_add_window(xfContext* xfc, UINT64 id, INT32 x, INT32 y, UIN appWindow->width = WINPR_ASSERTING_INT_CAST(int, width); appWindow->height = WINPR_ASSERTING_INT_CAST(int, height); + xf_AppWindowsLock(xfc); if (!xf_AppWindowCreate(xfc, appWindow)) goto fail; + if (!HashTable_Insert(xfc->railWindows, &appWindow->windowId, (void*)appWindow)) goto fail; return appWindow; fail: rail_window_free(appWindow); + xf_AppWindowsUnlock(xfc); return nullptr; } @@ -1396,26 +1404,10 @@ BOOL xf_rail_del_window(xfContext* xfc, UINT64 id) return HashTable_Remove(xfc->railWindows, &id); } -xfAppWindow* xf_rail_get_window(xfContext* xfc, UINT64 id) -{ - if (!xfc) - return nullptr; - - if (!xfc->railWindows) - return nullptr; - - HashTable_Lock(xfc->railWindows); - xfAppWindow* window = HashTable_GetItemValue(xfc->railWindows, &id); - if (!window) - HashTable_Unlock(xfc->railWindows); - - return window; -} - -void xf_rail_return_window(xfAppWindow* window) +void xf_rail_return_windowFrom(xfAppWindow* window, const char* file, const char* fkt, size_t line) { if (!window) return; - HashTable_Unlock(window->xfc->railWindows); + xfAppWindowsUnlockFrom(window->xfc, file, fkt, line); } diff --git a/client/X11/xf_rail.h b/client/X11/xf_rail.h index d87bb0cca..1b788bebc 100644 --- a/client/X11/xf_rail.h +++ b/client/X11/xf_rail.h @@ -125,10 +125,16 @@ BOOL xf_rail_disable_remoteapp_mode(xfContext* xfc); xfAppWindow* xf_rail_add_window(xfContext* xfc, UINT64 id, INT32 x, INT32 y, UINT32 width, UINT32 height, UINT32 surfaceId); -void xf_rail_return_window(xfAppWindow* window); +#define xf_rail_return_window(window) \ + xf_rail_return_windowFrom((window), __FILE__, __func__, __LINE__) +void xf_rail_return_windowFrom(xfAppWindow* window, const char* file, const char* fkt, size_t line); -WINPR_ATTR_MALLOC(xf_rail_return_window, 1) -xfAppWindow* xf_rail_get_window(xfContext* xfc, UINT64 id); +#define xf_rail_get_window(xfc, id) \ + xf_rail_get_windowFrom((xfc), (id), __FILE__, __func__, __LINE__) + +WINPR_ATTR_MALLOC(xf_rail_return_windowFrom, 1) +xfAppWindow* xf_rail_get_windowFrom(xfContext* xfc, UINT64 id, const char* file, const char* fkt, + size_t line); BOOL xf_rail_del_window(xfContext* xfc, UINT64 id); diff --git a/client/X11/xf_window.c b/client/X11/xf_window.c index 5c632e7fe..95774943e 100644 --- a/client/X11/xf_window.c +++ b/client/X11/xf_window.c @@ -1435,7 +1435,31 @@ void xf_DestroyWindow(xfContext* xfc, xfAppWindow* appWindow) free(appWindow); } -xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd) +static xfAppWindow* get_windowUnlocked(xfContext* xfc, UINT64 id) +{ + WINPR_ASSERT(xfc); + return HashTable_GetItemValue(xfc->railWindows, &id); +} + +xfAppWindow* xf_rail_get_windowFrom(xfContext* xfc, UINT64 id, const char* file, const char* fkt, + size_t line) +{ + if (!xfc) + return nullptr; + + if (!xfc->railWindows) + return nullptr; + + xfAppWindowsLockFrom(xfc, file, fkt, line); + xfAppWindow* window = get_windowUnlocked(xfc, id); + if (!window) + xfAppWindowsUnlockFrom(xfc, file, fkt, line); + + return window; +} + +xfAppWindow* xf_AppWindowFromX11WindowFrom(xfContext* xfc, Window wnd, const char* file, + const char* fkt, size_t line) { ULONG_PTR* pKeys = nullptr; @@ -1443,29 +1467,28 @@ xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd) if (!xfc->railWindows) return nullptr; - HashTable_Lock(xfc->railWindows); + xfAppWindowsLockFrom(xfc, file, fkt, line); size_t count = HashTable_GetKeys(xfc->railWindows, &pKeys); for (size_t index = 0; index < count; index++) { - xfAppWindow* appWindow = xf_rail_get_window(xfc, *(UINT64*)pKeys[index]); + xfAppWindow* appWindow = get_windowUnlocked(xfc, *(UINT64*)pKeys[index]); if (!appWindow) { - HashTable_Unlock(xfc->railWindows); + xfAppWindowsUnlockFrom(xfc, file, fkt, line); free(pKeys); return nullptr; } if (appWindow->handle == wnd) { - HashTable_Unlock(xfc->railWindows); free(pKeys); return appWindow; } } - HashTable_Unlock(xfc->railWindows); + xfAppWindowsUnlockFrom(xfc, file, fkt, line); free(pKeys); return nullptr; } @@ -1585,3 +1608,39 @@ void xf_XSetTransientForHint(xfContext* xfc, xfAppWindow* window) (void)LogDynAndXSetTransientForHint(xfc->log, xfc->display, window->handle, parent->handle); xf_rail_return_window(parent); } + +void xfAppWindowsLockFrom(xfContext* xfc, WINPR_ATTR_UNUSED const char* file, + WINPR_ATTR_UNUSED const char* fkt, WINPR_ATTR_UNUSED size_t line) +{ + WINPR_ASSERT(xfc); + +#if defined(WITH_VERBOSE_WINPR_ASSERT) + const DWORD level = WLOG_TRACE; + if (WLog_IsLevelActive(xfc->log, level)) + WLog_PrintTextMessage(xfc->log, level, line, file, fkt, "[rails] locking [%s]", fkt); +#endif + + HashTable_Lock(xfc->railWindows); + +#if defined(WITH_VERBOSE_WINPR_ASSERT) + WINPR_ASSERT(!xfc->isRailWindowsLocked); + xfc->isRailWindowsLocked = TRUE; +#endif +} + +void xfAppWindowsUnlockFrom(xfContext* xfc, WINPR_ATTR_UNUSED const char* file, + WINPR_ATTR_UNUSED const char* fkt, WINPR_ATTR_UNUSED size_t line) +{ + WINPR_ASSERT(xfc); + +#if defined(WITH_VERBOSE_WINPR_ASSERT) + const DWORD level = WLOG_TRACE; + if (WLog_IsLevelActive(xfc->log, level)) + WLog_PrintTextMessage(xfc->log, level, line, file, fkt, "[rails] unocking [%s]", fkt); + + WINPR_ASSERT(xfc->isRailWindowsLocked); + xfc->isRailWindowsLocked = FALSE; +#endif + + HashTable_Unlock(xfc->railWindows); +} diff --git a/client/X11/xf_window.h b/client/X11/xf_window.h index 5761170f8..803b23937 100644 --- a/client/X11/xf_window.h +++ b/client/X11/xf_window.h @@ -122,8 +122,17 @@ void xf_SetWindowMinMaxInfo(xfContext* xfc, xfAppWindow* appWindow, int maxWidth void xf_StartLocalMoveSize(xfContext* xfc, xfAppWindow* appWindow, int direction, int x, int y); void xf_EndLocalMoveSize(xfContext* xfc, xfAppWindow* appWindow); -WINPR_ATTR_MALLOC(xf_rail_return_window, 1) -xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd); +#define xf_AppWindowFromX11Window(xfc, wnd) \ + xf_AppWindowFromX11WindowFrom((xfc), (wnd), __FILE__, __func__, __LINE__) +WINPR_ATTR_MALLOC(xf_rail_return_windowFrom, 1) +xfAppWindow* xf_AppWindowFromX11WindowFrom(xfContext* xfc, Window wnd, const char* file, + const char* fkt, size_t line); + +#define xf_AppWindowsLock(xfc) xfAppWindowsLockFrom((xfc), __FILE__, __func__, __LINE__) +void xfAppWindowsLockFrom(xfContext* xfc, const char* file, const char* fkt, size_t line); + +#define xf_AppWindowsUnlock(xfc) xfAppWindowsUnlockFrom((xfc), __FILE__, __func__, __LINE__) +void xfAppWindowsUnlockFrom(xfContext* xfc, const char* file, const char* fkt, size_t line); const char* window_styles_to_string(UINT32 style, char* buffer, size_t length); const char* window_styles_ex_to_string(UINT32 styleEx, char* buffer, size_t length); diff --git a/client/X11/xfreerdp.h b/client/X11/xfreerdp.h index c82da03fb..469ecce87 100644 --- a/client/X11/xfreerdp.h +++ b/client/X11/xfreerdp.h @@ -290,6 +290,10 @@ struct xf_context wHashTable* railWindows; xfRailIconCache* railIconCache; +#if defined(WITH_VERBOSE_WINPR_ASSERT) + BOOL isRailWindowsLocked; +#endif + BOOL xkbAvailable; BOOL xrenderAvailable;