[PATCH v3 0/2] MR10271: Fixed thread safety issues with NtUserRegistClassExWOW and NtUserUnregisterClass.
Kingsoft Typing Pass failed to start probabilistically due to a deadlock between two threads registering and unregistering the same class. The replication scenario is as follows: |A thread (registration class) | B thread (deregistration class)| |-------------- |---------------| | NtUserRegisterClassExWOW | NtUserUnregisterClass | | create_class | -- | | -- | destroy_class | | find_shared_session_object | -- | | find failed, destroy_class | -- | | free class | -- | | -- | user_lock | | -- | remove class from list | | -- | throw exception(class has freeed by thread A) | Thread B threw an exception and will not call user_unlock. The upper layer application handled the memory access exception thrown by thread B, but due to If user_unlock is not called, the lock will remain occupied, and other threads will continue to wait when calling user_rock. -- v3: Win32u: Fixed thread safety issues with NtUserRegistClassExWOW and NtUserUnregisterClass. user32/tests: Test the thread safety of NtUserRegisterClassExWOW and NtUserUnregisterClass. https://gitlab.winehq.org/wine/wine/-/merge_requests/10271
From: yaoyongjie <yaoyongjie@uniontech.com> Change-Id: Ie01c29ec0c5d984b48a19f930dd03e6ddfc9288f --- dlls/user32/tests/class.c | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/dlls/user32/tests/class.c b/dlls/user32/tests/class.c index 66dee810c45..6dc6c8871c2 100644 --- a/dlls/user32/tests/class.c +++ b/dlls/user32/tests/class.c @@ -2435,6 +2435,55 @@ static void test_class_name(void) UnregisterClassW(wcex.lpszClassName, hinst); } +struct class_register_info +{ + const WCHAR *class_name; + int total_test_count; +}; + +static DWORD WINAPI thread_register_class_proc(void *param) +{ + struct class_register_info *info = param; + for (int i = 0; i < info->total_test_count; i++) + { + WNDCLASSEXW wcex = {0}; + wcex.cbSize = sizeof wcex; + wcex.lpfnWndProc = ClassTest_WndProc; + wcex.hIcon = LoadIconW(0, (LPCWSTR)IDI_APPLICATION); + wcex.lpszClassName = info->class_name; + RegisterClassExW(&wcex); + } + + return 0; +} + +static DWORD WINAPI thread_unregister_class_proc(void *param) +{ + struct class_register_info *info = param; + for (int i = 0; i < info->total_test_count; i++) + { + UnregisterClassW(info->class_name, 0); + } + + return 0; +} + +void test_class_multithread(void) +{ + struct class_register_info info_register = {L"test_class_multithread", 100}; + struct class_register_info info_unregister = {L"test_class_multithread", 100}; + + HANDLE thread_register = CreateThread(NULL, 0, thread_register_class_proc, &info_register, 0, NULL); + HANDLE thread_unregister = CreateThread(NULL, 0, thread_unregister_class_proc, &info_unregister, 0, NULL); + + ok(thread_register != NULL, "CreateThread failed, error %ld\n", GetLastError()); + ok(WaitForSingleObject(thread_register, INFINITE) == WAIT_OBJECT_0, "WaitForSingleObject failed\n"); + CloseHandle(thread_register); + ok(thread_unregister != NULL, "CreateThread failed, error %ld\n", GetLastError()); + ok(WaitForSingleObject(thread_unregister, INFINITE) == WAIT_OBJECT_0, "WaitForSingleObject failed\n"); + CloseHandle(thread_unregister); +} + START_TEST(class) { char **argv; @@ -2471,6 +2520,7 @@ START_TEST(class) test_comctl32_classes(); test_actctx_classes(); test_class_name(); + test_class_multithread(); /* this test unregisters the Button class so it should be executed at the end */ test_instances(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10271
From: yaoyongjie <yaoyongjie@uniontech.com> Kingsoft Typing Pass failed to start probabilistically due to a deadlock between two threads registering and logging out of the same class. The replication scenario is as follows: |A thread (registration class) | B thread (deregistration class)| |-------------- |---------------| | NtUserRegisterClassExWOW | NtUserUnregisterClass | | create_class | -- | | -- | destroy_class | | find_shared_session_object | -- | | find failed, destroy_class | -- | | free class | -- | | -- | user_lock | | -- | remove class from list | | -- | throw exception(class has freeed by thread A) | Thread B threw an exception and will not call user_unlock. The upper layer application handled the memory access exception thrown by thread B, but due to If user_unlock is not called, the lock will remain occupied, and other threads will continue to wait when calling user_rock. Change-Id: I7352f3a7cdfb4699628e85d976f5f87dc11f4325 --- dlls/win32u/class.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/dlls/win32u/class.c b/dlls/win32u/class.c index 5478088a4f4..1587b6b6795 100644 --- a/dlls/win32u/class.c +++ b/dlls/win32u/class.c @@ -547,6 +547,14 @@ ATOM WINAPI NtUserRegisterClassExWOW( const WNDCLASSEXW *wc, UNICODE_STRING *nam class->local = !is_builtin && !(wc->style & CS_GLOBALCLASS); + /* Other non-null values must be set by caller */ + if (wc->hIcon && !wc->hIconSm) + sm_icon = CopyImage( wc->hIcon, IMAGE_ICON, + get_system_metrics( SM_CXSMICON ), + get_system_metrics( SM_CYSMICON ), + LR_COPYFROMRESOURCE ); + + user_lock(); SERVER_START_REQ( create_class ) { req->local = class->local; @@ -562,11 +570,7 @@ ATOM WINAPI NtUserRegisterClassExWOW( const WNDCLASSEXW *wc, UNICODE_STRING *nam atom = reply->atom; } SERVER_END_REQ; - if (!ret) - { - free( class ); - return 0; - } + if (!ret) goto failed; if (!(shared = find_shared_session_object( locator.id, locator.offset ))) { @@ -578,18 +582,10 @@ ATOM WINAPI NtUserRegisterClassExWOW( const WNDCLASSEXW *wc, UNICODE_STRING *nam wine_server_call( req ); } SERVER_END_REQ; - free( class ); - return 0; - } - /* Other non-null values must be set by caller */ - if (wc->hIcon && !wc->hIconSm) - sm_icon = CopyImage( wc->hIcon, IMAGE_ICON, - get_system_metrics( SM_CXSMICON ), - get_system_metrics( SM_CYSMICON ), - LR_COPYFROMRESOURCE ); + goto failed; + } - user_lock(); if (class->local) list_add_head( &class_list, &class->entry ); else list_add_tail( &class_list, &class->entry ); @@ -607,6 +603,13 @@ ATOM WINAPI NtUserRegisterClassExWOW( const WNDCLASSEXW *wc, UNICODE_STRING *nam class->shared = shared; release_class_ptr( class ); return atom; + +failed: + user_unlock(); + NtUserDestroyCursor( sm_icon, 0 ); + free( class ); + return 0; + } /*********************************************************************** @@ -621,6 +624,8 @@ BOOL WINAPI NtUserUnregisterClass( UNICODE_STRING *name, HINSTANCE instance, /* create the desktop window to trigger builtin class registration */ get_desktop_window(); + user_lock(); + SERVER_START_REQ( destroy_class ) { req->instance = wine_server_client_ptr( instance ); @@ -628,11 +633,14 @@ BOOL WINAPI NtUserUnregisterClass( UNICODE_STRING *name, HINSTANCE instance, if (!wine_server_call_err( req )) class = wine_server_get_ptr( reply->client_ptr ); } SERVER_END_REQ; - if (!class) return FALSE; + if (!class) + { + user_unlock(); + return FALSE; + } TRACE( "%p\n", class ); - user_lock(); if (class->dce) free_dce( class->dce, 0, &drawables ); list_remove( &class->entry ); if (class->hbrBackground > (HBRUSH)(COLOR_GRADIENTINACTIVECAPTION + 1)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/10271
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/10271
participants (3)
-
Rémi Bernon -
yaoyongjie
-
Yongjie Yao (＠yaoyongjie)