[PATCH 0/5] MR3162: Chromium sandbox fixes
This series of patches enable Adobe PDF Reader 11 protected mode, which is essentially a Chromium sandbox, to work properly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Chromium creates a suspended sandbox process with a token of limited access. Then it sets a token with normal access for the main thread of the newly created process. Without this change, explorer is started with the process token of limited access and fails to create a desktop window. --- dlls/win32u/winstation.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/dlls/win32u/winstation.c b/dlls/win32u/winstation.c index 9b97a4b4076..f1e16326b96 100644 --- a/dlls/win32u/winstation.c +++ b/dlls/win32u/winstation.c @@ -445,7 +445,8 @@ HWND get_desktop_window(void) static const WCHAR system_dir[] = {'C',':','\\','w','i','n','d','o','w','s','\\', 's','y','s','t','e','m','3','2','\\',0}; RTL_USER_PROCESS_PARAMETERS params = { sizeof(params), sizeof(params) }; - PS_ATTRIBUTE_LIST ps_attr; + ULONG_PTR buffer[offsetof( PS_ATTRIBUTE_LIST, Attributes[2] ) / sizeof(ULONG_PTR)]; + PS_ATTRIBUTE_LIST *ps_attr = (PS_ATTRIBUTE_LIST *)buffer; PS_CREATE_INFO create_info; WCHAR desktop[MAX_PATH]; PEB *peb = NtCurrentTeb()->Peb; @@ -478,24 +479,30 @@ HWND get_desktop_window(void) RtlInitUnicodeString( ¶ms.WindowTitle, appnameW + 4 ); RtlInitUnicodeString( ¶ms.Desktop, desktop ); - ps_attr.TotalLength = sizeof(ps_attr); - ps_attr.Attributes[0].Attribute = PS_ATTRIBUTE_IMAGE_NAME; - ps_attr.Attributes[0].Size = sizeof(appnameW) - sizeof(WCHAR); - ps_attr.Attributes[0].ValuePtr = (WCHAR *)appnameW; - ps_attr.Attributes[0].ReturnLength = NULL; + ps_attr->Attributes[0].Attribute = PS_ATTRIBUTE_IMAGE_NAME; + ps_attr->Attributes[0].Size = sizeof(appnameW) - sizeof(WCHAR); + ps_attr->Attributes[0].ValuePtr = (WCHAR *)appnameW; + ps_attr->Attributes[0].ReturnLength = NULL; + + ps_attr->Attributes[1].Attribute = PS_ATTRIBUTE_TOKEN; + ps_attr->Attributes[1].Size = sizeof(HANDLE); + ps_attr->Attributes[1].ValuePtr = GetCurrentThreadEffectiveToken(); + ps_attr->Attributes[1].ReturnLength = NULL; + + ps_attr->TotalLength = offsetof( PS_ATTRIBUTE_LIST, Attributes[2] ); if (NtCurrentTeb64() && !NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR]) { NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR] = TRUE; status = NtCreateUserProcess( &process, &thread, PROCESS_ALL_ACCESS, THREAD_ALL_ACCESS, NULL, NULL, 0, THREAD_CREATE_FLAGS_CREATE_SUSPENDED, ¶ms, - &create_info, &ps_attr ); + &create_info, ps_attr ); NtCurrentTeb64()->TlsSlots[WOW64_TLS_FILESYSREDIR] = FALSE; } else status = NtCreateUserProcess( &process, &thread, PROCESS_ALL_ACCESS, THREAD_ALL_ACCESS, NULL, NULL, 0, THREAD_CREATE_FLAGS_CREATE_SUSPENDED, ¶ms, - &create_info, &ps_attr ); + &create_info, ps_attr ); if (!status) { NtResumeThread( thread, NULL ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Test that processes running with an invisible window station have physical monitor information instead of the virtual monitor information used for services. --- dlls/user32/tests/winstation.c | 72 ++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/dlls/user32/tests/winstation.c b/dlls/user32/tests/winstation.c index e33a3252494..5902743c6ca 100644 --- a/dlls/user32/tests/winstation.c +++ b/dlls/user32/tests/winstation.c @@ -1026,8 +1026,71 @@ static void test_foregroundwindow(void) ok(timeout == timeout_old, "unexpected timeout %ld\n", timeout); } +static void test_invisible_winstation_child(char *expected_info) +{ + char buffer[MAX_PATH]; + HDESK desktop; + int ret; + + desktop = GetThreadDesktop(GetCurrentThreadId()); + ok(!!desktop, "GetThreadDesktop failed, error %lu.\n", GetLastError()); + + ret = GetUserObjectInformationA(desktop, UOI_NAME, &buffer, sizeof(buffer), NULL); + ok(ret, "GetUserObjectInformationA failed, error %lu.\n", GetLastError()); + ok(!strcmp(buffer, "invisible_winstation_desktop"), "Got unexpected desktop %s.\n", buffer); + + sprintf(buffer, "%d,%d,%d,%d,%d", GetSystemMetrics(SM_XVIRTUALSCREEN), + GetSystemMetrics(SM_YVIRTUALSCREEN), GetSystemMetrics(SM_CXVIRTUALSCREEN), + GetSystemMetrics(SM_CYVIRTUALSCREEN), GetSystemMetrics(SM_CMONITORS)); + todo_wine_if(strcmp(buffer, "0,0,1024,768,1")) + ok(!strcmp(buffer, expected_info), "Expected %s, got %s.\n", expected_info, buffer); +} + +static void test_invisible_winstation(char **argv) +{ + char buffer[MAX_PATH], desktop_name[MAX_PATH]; + HWINSTA old_winstation, winstation; + PROCESS_INFORMATION pi = {0}; + STARTUPINFOA si = {0}; + HDESK desktop; + BOOL ret; + + old_winstation = GetProcessWindowStation(); + ok(!!old_winstation, "GetProcessWindowStation failed, error %lu.\n", GetLastError()); + winstation = CreateWindowStationW(NULL, 0, GENERIC_READ | WINSTA_CREATEDESKTOP, NULL); + ok(!!winstation, "CreateWindowStationW failed, error %lu.\n", GetLastError()); + ret = SetProcessWindowStation(winstation); + ok(ret, "SetProcessWindowStation failed, error %lu.\n", GetLastError()); + desktop = CreateDesktopA("invisible_winstation_desktop", NULL, NULL, 0, DESKTOP_CREATEWINDOW, NULL); + ok(!!desktop, "CreateDesktopA failed, error %lu.\n", GetLastError()); + + ret = GetUserObjectInformationA(winstation, UOI_NAME, buffer, sizeof(buffer), NULL); + ok(ret, "GetUserObjectInformationA failed, error %lu.\n", GetLastError()); + strcpy(desktop_name, buffer); + strcat(desktop_name, "\\invisible_winstation_desktop"); + + si.cb = sizeof(si); + si.lpDesktop = desktop_name; + + sprintf(buffer, "\"%s\" %s invisible %d,%d,%d,%d,%d", argv[0], argv[1], + GetSystemMetrics(SM_XVIRTUALSCREEN), GetSystemMetrics(SM_YVIRTUALSCREEN), + GetSystemMetrics(SM_CXVIRTUALSCREEN), GetSystemMetrics(SM_CYVIRTUALSCREEN), + GetSystemMetrics(SM_CMONITORS)); + ret = CreateProcessA(argv[0], buffer, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); + ok(ret, "CreateProcessA failed, error %lu.\n", GetLastError()); + + wait_child_process(pi.hProcess); + CloseHandle(pi.hThread); + CloseHandle(pi.hProcess); + CloseDesktop(desktop); + CloseWindowStation(winstation); + SetProcessWindowStation(old_winstation); +} + START_TEST(winstation) { + char **argv; + int argc; HMODULE hntdll = GetModuleHandleA("ntdll.dll"); pNtQueryObject = (void *)GetProcAddress(hntdll, "NtQueryObject"); @@ -1041,6 +1104,14 @@ START_TEST(winstation) return; } + argc = winetest_get_mainargs(&argv); + if (argc > 2) + { + if (!lstrcmpA(argv[2], "invisible")) + test_invisible_winstation_child(argv[3]); + + return; + } test_inputdesktop(); test_inputdesktop2(); test_enumstations(); @@ -1048,4 +1119,5 @@ START_TEST(winstation) test_handles(); test_getuserobjectinformation(); test_foregroundwindow(); + test_invisible_winstation(argv); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Having an invisible window station doesn't mean an application is running in a service context. For example, Chromium creates a sandbox process to run in an invisible window station and still expect the physical monitor information to be returned, instead of the virtual monitor information for services. --- dlls/user32/tests/winstation.c | 1 - dlls/win32u/sysparams.c | 8 +++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dlls/user32/tests/winstation.c b/dlls/user32/tests/winstation.c index 5902743c6ca..7829e3364b3 100644 --- a/dlls/user32/tests/winstation.c +++ b/dlls/user32/tests/winstation.c @@ -1042,7 +1042,6 @@ static void test_invisible_winstation_child(char *expected_info) sprintf(buffer, "%d,%d,%d,%d,%d", GetSystemMetrics(SM_XVIRTUALSCREEN), GetSystemMetrics(SM_YVIRTUALSCREEN), GetSystemMetrics(SM_CXVIRTUALSCREEN), GetSystemMetrics(SM_CYVIRTUALSCREEN), GetSystemMetrics(SM_CMONITORS)); - todo_wine_if(strcmp(buffer, "0,0,1024,768,1")) ok(!strcmp(buffer, expected_info), "Expected %s, got %s.\n", expected_info, buffer); } diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index acda600b43c..233e5bb934b 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -1994,14 +1994,16 @@ static BOOL desktop_update_display_devices( BOOL force, struct device_manager_ct BOOL update_display_cache( BOOL force ) { + static const WCHAR wine_service_station_name[] = + {'_','_','w','i','n','e','s','e','r','v','i','c','e','_','w','i','n','s','t','a','t','i','o','n',0}; HWINSTA winstation = NtUserGetProcessWindowStation(); struct device_manager_ctx ctx = {0}; BOOL was_virtual_desktop, ret; - USEROBJECTFLAGS flags; + WCHAR name[MAX_PATH]; /* services do not have any adapters, only a virtual monitor */ - if (NtUserGetObjectInformation( winstation, UOI_FLAGS, &flags, sizeof(flags), NULL ) - && !(flags.dwFlags & WSF_VISIBLE)) + if (NtUserGetObjectInformation( winstation, UOI_NAME, name, sizeof(name), NULL ) + && !wcscmp( name, wine_service_station_name )) { pthread_mutex_lock( &display_lock ); clear_display_devices(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/winstation.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/dlls/user32/tests/winstation.c b/dlls/user32/tests/winstation.c index 7829e3364b3..ab64556a754 100644 --- a/dlls/user32/tests/winstation.c +++ b/dlls/user32/tests/winstation.c @@ -1030,6 +1030,7 @@ static void test_invisible_winstation_child(char *expected_info) { char buffer[MAX_PATH]; HDESK desktop; + HWND hwnd; int ret; desktop = GetThreadDesktop(GetCurrentThreadId()); @@ -1043,6 +1044,13 @@ static void test_invisible_winstation_child(char *expected_info) GetSystemMetrics(SM_YVIRTUALSCREEN), GetSystemMetrics(SM_CXVIRTUALSCREEN), GetSystemMetrics(SM_CYVIRTUALSCREEN), GetSystemMetrics(SM_CMONITORS)); ok(!strcmp(buffer, expected_info), "Expected %s, got %s.\n", expected_info, buffer); + + hwnd = GetDesktopWindow(); + ok(!!hwnd, "GetDesktopWindow failed, error %lu.\n", GetLastError()); + + ret = SendMessageW(hwnd, WM_NCHITTEST, 0, 0); + todo_wine + ok(ret == HTCLIENT, "SendMessageW failed, error %lu.\n", GetLastError()); } static void test_invisible_winstation(char **argv) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
From: Zhiyi Zhang <zzhang(a)codeweavers.com> The graphics driver information is stored as a property in the desktop window. When the server get_desktop_window handler simply returns a window handle when the window station is invisible, the window handle doesn't have the graphics driver property, which is set in desktop_window_proc() when handling WM_NCCREATE. Removing the invisible window station check allows an invisible explorer desktop window to be created and with the required property. --- dlls/user32/tests/winstation.c | 1 - server/window.c | 8 ++------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/dlls/user32/tests/winstation.c b/dlls/user32/tests/winstation.c index ab64556a754..5f0925a7712 100644 --- a/dlls/user32/tests/winstation.c +++ b/dlls/user32/tests/winstation.c @@ -1049,7 +1049,6 @@ static void test_invisible_winstation_child(char *expected_info) ok(!!hwnd, "GetDesktopWindow failed, error %lu.\n", GetLastError()); ret = SendMessageW(hwnd, WM_NCHITTEST, 0, 0); - todo_wine ok(ret == HTCLIENT, "SendMessageW failed, error %lu.\n", GetLastError()); } diff --git a/server/window.c b/server/window.c index 328e3dfc64c..242e93f303a 100644 --- a/server/window.c +++ b/server/window.c @@ -2151,14 +2151,10 @@ DECL_HANDLER(destroy_window) DECL_HANDLER(get_desktop_window) { struct desktop *desktop = get_thread_desktop( current, 0 ); - int force; if (!desktop) return; - /* if winstation is invisible, then avoid roundtrip */ - force = req->force || !(desktop->winstation->flags & WSF_VISIBLE); - - if (!desktop->top_window && force) /* create it */ + if (!desktop->top_window && req->force) /* create it */ { if ((desktop->top_window = create_window( NULL, NULL, DESKTOP_ATOM, 0 ))) { @@ -2167,7 +2163,7 @@ DECL_HANDLER(get_desktop_window) } } - if (!desktop->msg_window && force) /* create it */ + if (!desktop->msg_window && req->force) /* create it */ { static const WCHAR messageW[] = {'M','e','s','s','a','g','e'}; static const struct unicode_str name = { messageW, sizeof(messageW) }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3162
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134167 Your paranoid android. === debian11 (build log) === WineRunWineTest.pl:error: The task timed out === debian11b (build log) === WineRunWineTest.pl:error: The task timed out
On Mon Jun 26 11:49:58 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134167 Your paranoid android. === debian11 (build log) === WineRunWineTest.pl:error: The task timed out === debian11b (build log) === WineRunWineTest.pl:error: The task timed out ``` I will take a look at this. It's caused by 5/5 but it works fine locally.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3162#note_36801
Nitpick: Should be "a real" in the last commit message, not "an real". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3162#note_36820
participants (4)
-
Alex Henrie (＠alexhenrie) -
Marvin -
Zhiyi Zhang
-
Zhiyi Zhang (＠zhiyi)