[PATCH v9 0/6] MR9843: wtsapi32: Fix session id and desktop handling when using CreateProcessAsUserW with tokens from WTSQueryUserToken
This Merge request resolves [Wine-Bug #58588](https://bugs.winehq.org/show_bug.cgi?id=58588). The underlying problem was that services started on their own, non-interactive winstation, but within the current user's session. This is trivially not the case for system services on Windows: they run, by design, in other user contexts (e.g. NTAUTHORITY\SYSTEM). Whenever a service wants to start a process within another user's graphical session, it needs to set the session id to the correct value. This may be done using the WTS API. This MR implements this functionality in a (hopefully) wine-compatible way: 1. Patch wineserver so tokens can be transferred to other sessions 2. Implement NtSetInformationToken for TokenSessionId 3. Change the session of non-interactive services to session 0 (from the default session 1) in addition to setting their desktop to `__wineservice_winstation\Default` 4. Let CreateProcessAsUserW reset the desktop to the default value `__winsta0\Default` if the session of the to-be-created process is not a service session. This avoids implementing all user and session handling in-depth while still allowing the underlying functionality to work. A real-world user of this mechanism is the Dassault 3DEXPERIENCE launcher, which runs as a system service and wants to open interactive child processes within a user's scope. -- v9: server: Let connect_process_winstation overwrite the desktop if needed services: start services with session id 0 wtsapi32: Let WTSEnumerateSessionsA/W return Console and Service session server: attach winstations to sessions, filter enum_windowstation wtsapi32: Make WTSQueryUserToken return a token with correct session id server: Allow changing of session id for process tokens https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> Add a server protocol message to set session ids for process tokens. Implement NtSetInformationToken for TokenSessionId in ntdll. --- dlls/ntdll/unix/security.c | 10 ++++++++-- server/protocol.def | 5 +++++ server/token.c | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/dlls/ntdll/unix/security.c b/dlls/ntdll/unix/security.c index 949af82f783..aff8763ee75 100644 --- a/dlls/ntdll/unix/security.c +++ b/dlls/ntdll/unix/security.c @@ -702,7 +702,7 @@ NTSTATUS WINAPI NtSetInformationToken( HANDLE token, TOKEN_INFORMATION_CLASS cla break; case TokenSessionId: - if (length < sizeof(DWORD)) + if (length < sizeof(ULONG)) { ret = STATUS_INFO_LENGTH_MISMATCH; break; @@ -712,7 +712,13 @@ NTSTATUS WINAPI NtSetInformationToken( HANDLE token, TOKEN_INFORMATION_CLASS cla ret = STATUS_ACCESS_VIOLATION; break; } - FIXME("TokenSessionId stub!\n"); + SERVER_START_REQ( set_token_session_id ) + { + req->handle = wine_server_obj_handle( token ); + req->session_id = *((ULONG *)info); + ret = wine_server_call(req); + } + SERVER_END_REQ; ret = STATUS_SUCCESS; break; diff --git a/server/protocol.def b/server/protocol.def index 0ac64297026..9f210a8504d 100644 --- a/server/protocol.def +++ b/server/protocol.def @@ -3550,6 +3550,11 @@ enum caret_state VARARG(acl,acl); /* default dacl to set */ @END +@REQ(set_token_session_id) + obj_handle_t handle; /* handle to the token */ + unsigned int session_id; /* session_id the token should become */ +@END + @REQ(set_security_object) obj_handle_t handle; /* handle to the object */ unsigned int security_info; /* which parts of security descriptor to set */ diff --git a/server/token.c b/server/token.c index 5ce7298211e..b8a214fd21e 100644 --- a/server/token.c +++ b/server/token.c @@ -1602,6 +1602,20 @@ DECL_HANDLER(set_token_default_dacl) } } +DECL_HANDLER(set_token_session_id) +{ + struct token *token; + + if((token = (struct token *)get_handle_obj( current->process, req->handle, + TOKEN_ADJUST_DEFAULT, + &token_ops))) + { + token->session_id = req->session_id; + + release_object( token ); + } +} + DECL_HANDLER(create_linked_token) { struct token *token, *linked; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> On Windows, WTSQueryUserToken called with a session_id returns a token that is in that user's current session and winstation. Since WINE lacks handling for multiple users, make WTSQueryUserToken return the duplicated token with the requested session_id set. --- dlls/wtsapi32/wtsapi32.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dlls/wtsapi32/wtsapi32.c b/dlls/wtsapi32/wtsapi32.c index 5a1a87f416e..0a2e25bb9cd 100644 --- a/dlls/wtsapi32/wtsapi32.c +++ b/dlls/wtsapi32/wtsapi32.c @@ -653,9 +653,13 @@ BOOL WINAPI WTSQueryUserToken(ULONG session_id, PHANDLE token) return FALSE; } - return DuplicateHandle(GetCurrentProcess(), GetCurrentProcessToken(), + if(!DuplicateHandle(GetCurrentProcess(), GetCurrentProcessToken(), GetCurrentProcess(), token, - 0, FALSE, DUPLICATE_SAME_ACCESS); + 0, FALSE, DUPLICATE_SAME_ACCESS)) + return FALSE; + + + return SetTokenInformation(*token, TokenSessionId, &session_id, sizeof(ULONG)); } /************************************************************ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> Make enum_windowstation filter for the current processes session; only return windowstations attached to the current session. --- server/user.h | 1 + server/winstation.c | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/server/user.h b/server/user.h index 060e54ec775..527136ae732 100644 --- a/server/user.h +++ b/server/user.h @@ -49,6 +49,7 @@ struct winstation unsigned int monitor_count; /* number of monitors */ struct monitor_info *monitors; /* window station monitors */ unsigned __int64 monitor_serial; /* winstation monitor update counter */ + unsigned int session_id; /* session id this winstation belongs to */ }; struct key_repeat diff --git a/server/winstation.c b/server/winstation.c index 90e55b80d7d..770162d1ad6 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -136,7 +136,7 @@ static const struct object_ops desktop_ops = /* create a winstation object */ static struct winstation *create_winstation( struct object *root, const struct unicode_str *name, - unsigned int attr, unsigned int flags ) + unsigned int attr, unsigned int flags, unsigned int session_id ) { struct winstation *winstation; @@ -152,6 +152,7 @@ static struct winstation *create_winstation( struct object *root, const struct u winstation->monitors = NULL; winstation->monitor_count = 0; winstation->monitor_serial = 1; + winstation->session_id = session_id; list_add_tail( &winstation_list, &winstation->entry ); list_init( &winstation->desktops ); if (!(winstation->desktop_names = create_namespace( 7 ))) @@ -613,7 +614,7 @@ DECL_HANDLER(create_winstation) reply->handle = 0; if (req->rootdir && !(root = get_directory_obj( current->process, req->rootdir ))) return; - if ((winstation = create_winstation( root, &name, req->attributes, req->flags ))) + if ((winstation = create_winstation( root, &name, req->attributes, req->flags, current->process->session_id ))) { reply->handle = alloc_handle( current->process, winstation, req->access, req->attributes ); release_object( winstation ); @@ -987,6 +988,7 @@ DECL_HANDLER(enum_winstation) unsigned int access = WINSTA_ENUMERATE; if (!(name = winstation->obj.name)) continue; if (!check_object_access( NULL, &winstation->obj, &access )) continue; + if (current->process->session_id != winstation->session_id) continue; reply->count++; reply->total += name->len + sizeof(WCHAR); if (reply->total <= size) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> WINE manages two sessions; Services and Console. Since support for more sessions is currently non-existent, hardcode both sessions. --- dlls/wtsapi32/tests/wtsapi.c | 2 +- dlls/wtsapi32/wtsapi32.c | 37 +++++++++++++++++++++++++----------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/dlls/wtsapi32/tests/wtsapi.c b/dlls/wtsapi32/tests/wtsapi.c index 49d74b42afc..60ee705b57b 100644 --- a/dlls/wtsapi32/tests/wtsapi.c +++ b/dlls/wtsapi32/tests/wtsapi.c @@ -430,7 +430,7 @@ static void test_WTSEnumerateSessions(void) } } ok(console_found, "Console session not found.\n"); - todo_wine ok(services_found, "Services session not found.\n"); + ok(services_found, "Services session not found.\n"); WTSFreeMemory(info); WTSFreeMemory(infoA); diff --git a/dlls/wtsapi32/wtsapi32.c b/dlls/wtsapi32/wtsapi32.c index 0a2e25bb9cd..306f43bcc53 100644 --- a/dlls/wtsapi32/wtsapi32.c +++ b/dlls/wtsapi32/wtsapi32.c @@ -348,26 +348,41 @@ BOOL WINAPI WTSEnumerateSessionsA(HANDLE server, DWORD reserved, DWORD version, BOOL WINAPI WTSEnumerateSessionsW(HANDLE server, DWORD reserved, DWORD version, PWTS_SESSION_INFOW *session_info, DWORD *count) { - static const WCHAR session_name[] = L"Console"; + static const WCHAR interactive_session_name[] = L"Console"; + static const WCHAR service_session_name[] = L"Services"; + PWTS_SESSION_INFOW sessions; + size_t next_string_offset = 2 * sizeof(**session_info); FIXME("%p 0x%08lx 0x%08lx %p %p semi-stub.\n", server, reserved, version, session_info, count); if (!session_info || !count) return FALSE; - if (!(*session_info = malloc(sizeof(**session_info) + sizeof(session_name)))) + if (!(*session_info = malloc(2 * sizeof(**session_info) + sizeof(interactive_session_name) + sizeof(service_session_name)))) { SetLastError(ERROR_OUTOFMEMORY); return FALSE; } - if (!ProcessIdToSessionId( GetCurrentProcessId(), &(*session_info)->SessionId)) - { - WTSFreeMemory(*session_info); - return FALSE; - } - *count = 1; - (*session_info)->State = WTSActive; - (*session_info)->pWinStationName = (WCHAR *)((char *)*session_info + sizeof(**session_info)); - memcpy((*session_info)->pWinStationName, session_name, sizeof(session_name)); + + /* As per discussion in IRC, we hardcode the console and server session. + * If WINE ever supports multiple users, this probably needs to query smss.exe. */ + + *count = 2; + + sessions = *session_info; + + /* Services session; has session ID 0 */ + sessions[0].SessionId = 0; + sessions[0].State = WTSDisconnected; + sessions[0].pWinStationName = (WCHAR *)((char *)*session_info + next_string_offset); + memcpy(sessions[0].pWinStationName, service_session_name, sizeof(service_session_name)); + + next_string_offset += sizeof(service_session_name); + + /* Console session; WINE sets this to session ID 1 */ + sessions[1].SessionId = 1; + sessions[1].State = WTSActive; + sessions[1].pWinStationName = (WCHAR*)((char *)*session_info + next_string_offset); + memcpy(sessions[1].pWinStationName, interactive_session_name, sizeof(interactive_session_name)); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> Services run in a non-interactive session by default; this session is also not enumeratable in WTSEnumerateSessions. Previously, the first part was handled by overwriting the winstation to __wineservice_winstation\Default while leaving the session id to the default 1. --- dlls/wtsapi32/wtsapi32.c | 1 + programs/services/services.c | 21 +++++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/dlls/wtsapi32/wtsapi32.c b/dlls/wtsapi32/wtsapi32.c index 306f43bcc53..05811f77cb0 100644 --- a/dlls/wtsapi32/wtsapi32.c +++ b/dlls/wtsapi32/wtsapi32.c @@ -354,6 +354,7 @@ BOOL WINAPI WTSEnumerateSessionsW(HANDLE server, DWORD reserved, DWORD version, size_t next_string_offset = 2 * sizeof(**session_info); FIXME("%p 0x%08lx 0x%08lx %p %p semi-stub.\n", server, reserved, version, session_info, count); + FIXME("Always returning default session id / name\n"); if (!session_info || !count) return FALSE; diff --git a/programs/services/services.c b/programs/services/services.c index dff3cd60040..933e8bb2689 100644 --- a/programs/services/services.c +++ b/programs/services/services.c @@ -918,7 +918,7 @@ static DWORD service_start_process(struct service_entry *service_entry, struct p BOOL is_wow64 = FALSE; HANDLE token; WCHAR *path; - DWORD err; + DWORD err, service_session_id = 0; BOOL r; service_lock(service_entry); @@ -1028,7 +1028,10 @@ found: si.lpDesktop = (WCHAR *)L"__wineservice_winstation\\Default"; } - if (!environment && OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_DUPLICATE, &token)) + r = DuplicateHandle(GetCurrentProcess(), GetCurrentProcessToken(), + GetCurrentProcess(), &token, 0, FALSE, DUPLICATE_SAME_ACCESS); + TRACE("DuplicateHandle %u %lu\n", r, GetLastError()); + if (!environment && r ) { WCHAR val[16]; CreateEnvironmentBlock(&environment, token, FALSE); @@ -1040,7 +1043,6 @@ found: RtlInitUnicodeString( &value, val ); RtlSetEnvironmentVariable( (WCHAR **)&environment, &name, &value ); } - CloseHandle(token); } service_entry->status.dwCurrentState = SERVICE_START_PENDING; @@ -1053,8 +1055,19 @@ found: process->use_count++; service_unlock(service_entry); - r = CreateProcessW(NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS, environment, NULL, &si, &pi); + r = SetTokenInformation(token, TokenSessionId, &service_session_id, sizeof(service_session_id)); + + if(!r) + { + err = GetLastError(); + process_terminate(process); + release_process(process); + return err; + } + + r = CreateProcessAsUserW(token, NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS, environment, NULL, &si, &pi); free(path); + CloseHandle(token); if (!r) { err = GetLastError(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
From: Katharina Bogad <katharina@hacked.xyz> Calling CreateProcessAsUserW with a token that has session_id != 0 set should result in the program being started on the token's assoiciated desktop. Currently, WINE doesn't handle multiple users and multiple winstations, so for the moment, if the new process has a session_id different from it's parent thread session_id, reset the winstations to their default values according to the relevant session_id. This allows serivces started with session_id 0 running non-interactively to get a session_id from wtsapi32 belonging to an interactive session and use CreateProcessAsUserW to open child processes in an interactive environment. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58588 --- server/winstation.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/server/winstation.c b/server/winstation.c index 770162d1ad6..43432dd97e2 100644 --- a/server/winstation.c +++ b/server/winstation.c @@ -501,12 +501,19 @@ void set_process_default_desktop( struct process *process, struct desktop *deskt void connect_process_winstation( struct process *process, struct unicode_str *desktop_path, struct thread *parent_thread, struct process *parent_process ) { - struct unicode_str desktop_name = *desktop_path, winstation_name = {0}; + struct unicode_str desktop_name = *desktop_path, winstation_name = {0}, root_name = {0}, full_winstation_name = {0}; const int attributes = OBJ_CASE_INSENSITIVE | OBJ_OPENIF; struct winstation *winstation = NULL; struct desktop *desktop = NULL; const WCHAR *wch, *end; obj_handle_t handle; + char ascii_root[50]; /* len of root path + len(str(2**32)) + 4 (to be safe) */ + static const WCHAR service_winstationW[] = {'_','_','w','i','n','e','s','e','r','v','i','c','e','_','w','i','n','s','t','a','t','i','o','n'}; + static const WCHAR console_winstationW[] = { 'W','i','n','S','t','a','0'}; + static const WCHAR default_desktopW[] = {'D','e','f','a','u','l','t'}; + static const struct unicode_str service_winstation_str = {service_winstationW, sizeof(service_winstationW)}; + static const struct unicode_str console_winstation_str = {console_winstationW, sizeof(console_winstationW)}; + static const struct unicode_str default_desktop_str = {default_desktopW, sizeof(default_desktopW)}; for (wch = desktop_name.str, end = wch + desktop_name.len / sizeof(WCHAR); wch != end; wch++) { @@ -520,14 +527,41 @@ void connect_process_winstation( struct process *process, struct unicode_str *de } } + if(process->session_id != parent_thread->process->session_id) + { + /* Someone used CreateProcessAsUserW; reset the winstation to the default values. */ + switch(process->session_id) { + case 0: + memcpy(&winstation_name, &service_winstation_str, sizeof(winstation_name)); + break; + default: /* fall through */ + case 1: + memcpy(&winstation_name, &console_winstation_str, sizeof(winstation_name)); + break; + } + + memcpy(&desktop_name, &default_desktop_str, sizeof(desktop_name)); + } + /* check for an inherited winstation handle (don't ask...) */ if ((handle = find_inherited_handle( process, &winstation_ops ))) { winstation = (struct winstation *)get_handle_obj( process, handle, 0, &winstation_ops ); } - else if (winstation_name.len && (winstation = open_named_object( NULL, &winstation_ops, &winstation_name, attributes ))) + else if (winstation_name.len) { - handle = alloc_handle( process, winstation, STANDARD_RIGHTS_REQUIRED | WINSTA_ALL_ACCESS, 0 ); + snprintf(ascii_root, sizeof(ascii_root), "\\Sessions\\%u\\Windows\\WindowStations\\", process->session_id); + ascii_to_unicode_str(ascii_root, &root_name); + + full_winstation_name.len = root_name.len + winstation_name.len; + full_winstation_name.str = malloc(full_winstation_name.len + 2); + memcpy((void*)full_winstation_name.str, root_name.str, root_name.len); + memcpy((void*)(full_winstation_name.str + root_name.len / sizeof(WCHAR)), winstation_name.str, winstation_name.len); + + if((winstation = open_named_object( NULL, &winstation_ops, &full_winstation_name, attributes ))) + { + handle = alloc_handle( process, winstation, STANDARD_RIGHTS_REQUIRED | WINSTA_ALL_ACCESS, 0 ); + } } else if (parent_process->winstation) { @@ -564,6 +598,7 @@ void connect_process_winstation( struct process *process, struct unicode_str *de } if (handle) set_process_default_desktop( process, desktop, handle ); + done: if (desktop) release_object( desktop ); if (winstation) release_object( winstation ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9843
v3 - also rebased against current master. Diff: - removed all changes to dlls/kernelbase; desktop switching now occurs entirely in wineserver. - As discussed in IRC with @zfigura, all relevant places now hardcode session 0 and session 1. - Cleaned up commit history; Changes to WTSEnumerateSessionsW are now in their own commit. Since I now do not touch the startupinfo at all, I hope that the tests pass. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9843#note_127283
1/6: ``` + SERVER_START_REQ( set_token_session_id ) + { + req->handle = wine_server_obj_handle( token ); + req->session_id = *((ULONG *)info); + ret = wine_server_call(req); + } + SERVER_END_REQ; ret = STATUS_SUCCESS; ``` Looks like you forgot to remove the "ret = STATUS_SUCCESS" here. ``` + if((token = (struct token *)get_handle_obj( current->process, req->handle, + TOKEN_ADJUST_DEFAULT, + &token_ops))) + { + token->session_id = req->session_id; + + release_object( token ); + } ``` Shouldn't that be TOKEN_ADJUST_SESSIONID? Also, space after the if. (Also, you could probably make that two lines instead of three, and get rid of the blank line.) 2/6: ``` + return FALSE; + + + return SetTokenInformation(*token, TokenSessionId, &session_id, sizeof(ULONG)); ``` One blank line is enough ;-) 4/6: ``` + /* As per discussion in IRC, we hardcode the console and server session. + * If WINE ever supports multiple users, this probably needs to query smss.exe. */ ``` I don't think we need the "as per IRC" part. 5/6: ``` @@ -354,6 +354,7 @@ BOOL WINAPI WTSEnumerateSessionsW(HANDLE server, DWORD reserved, DWORD version, size_t next_string_offset = 2 * sizeof(**session_info); FIXME("%p 0x%08lx 0x%08lx %p %p semi-stub.\n", server, reserved, version, session_info, count); + FIXME("Always returning default session id / name\n"); if (!session_info || !count) return FALSE; ``` I don't think this belongs here, and I don't think it's necessary anywhere. The "semi-stub" is already there. 5/6: ``` + r = DuplicateHandle(GetCurrentProcess(), GetCurrentProcessToken(), + GetCurrentProcess(), &token, 0, FALSE, DUPLICATE_SAME_ACCESS); + TRACE("DuplicateHandle %u %lu\n", r, GetLastError()); + if (!environment && r ) ``` That's a bit odd. It was already odd, but I'd advocate for improving it. I don't think we ever expect DuplicateHandle() to fail, so I'd just say ``` if (!DuplicateHandle(...)) ERR("Failed to duplicate handle, error %lu.\n", GetLastError()); if (!environment) ``` if not get rid of the trace altogether. Similarly, here and below, no need to assign BOOL returns to a variable; just check them directly in the if (). ``` + r = SetTokenInformation(token, TokenSessionId, &service_session_id, sizeof(service_session_id)); + + if(!r) ``` As above, this can just be one line. And I don't think SetTokenInformation() can ever fail here, so I just wouldn't bother with that anyway. 6/6: ``` @@ -520,14 +527,41 @@ void connect_process_winstation( struct process *process, struct unicode_str *de } } + if(process->session_id != parent_thread->process->session_id) + { + /* Someone used CreateProcessAsUserW; reset the winstation to the default values. */ + switch(process->session_id) { + case 0: + memcpy(&winstation_name, &service_winstation_str, sizeof(winstation_name)); + break; + default: /* fall through */ + case 1: + memcpy(&winstation_name, &console_winstation_str, sizeof(winstation_name)); + break; + } + + memcpy(&desktop_name, &default_desktop_str, sizeof(desktop_name)); + } + /* check for an inherited winstation handle (don't ask...) */ ``` This seems odd. Is it really correct to ignore the winstation/desktop from the startup info? Shouldn't all of this be in the fallback path for when we don't have a window station and would otherwise inherit it from the parent? Or maybe the logic should all remain, but if the winstation we found has a session ID that doesn't match ours, we give up and use the default winstation/desktop for that session. For instance, what happens if you try to create a process with WinSta0 from a service? Or a custom winstation that an interactive process created? Or try to connect to another user's winstation without setting the session ID? (Also, that switch seems excessive...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9843#note_127433
Thank you for looking at the MR! Re: 6/6 - the desktop needs to be in the processes session. Checking for NULL after inheriting the winstation won't change if the parent process is a valid winstation. But I agree that this should probably come as a last fallback; if no winstation is found or nothing could be intherited or the inherited winstation lives in a different session, then we should reset to the default. Maybe it is a wise idea to test what windows does here, but I am still not entirely sure how to code such a test within the WINE conformance tests - WTSQueryUserToken is basically only callable from within a service context, and I still understand winetest way to little to implement this into the wtsapi tests. Regarding the switch, yeah, I thought that this conveys a bit clearer than an if/else that the two cases are session 0 and 1 (with 1 adopting the default). But I can surely change that and explain the case in a comment. Re: 1-5/6: I'll implement this in the next iteration. Sorry for the noise in review; I am still getting acquainted to the WINE code base and "how things are usually done". Thank you for taking the time to point out the little things! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9843#note_127442
Re: 6/6 - the desktop needs to be in the processes session. Checking for NULL after inheriting the winstation won't change if the parent process is a valid winstation, but not in the user's session.
But I agree that forced switching should probably come as a last fallback; if no winstation is found or nothing could be intherited or the inherited winstation lives in a different session, then we should reset to the default. Maybe it is a wise idea to test what windows does here, but I am still not entirely sure how to code such a test within the WINE conformance tests - WTSQueryUserToken is basically only callable from within a service context, and I still understand winetest way to little to implement this into the wtsapi tests.
It might not be possible to do in an automated way, but it's probably worth testing at least manually whether you can create a process with a different session *and* a non-default window station that was previously created that matches that session, and other things of that ilk. I don't know if you can even do that from a service, can you? Maybe it needs RDP or multi-user to properly work, in which case it's definitely not something we can automate, and that's fine.
Regarding the switch, yeah, I thought that this conveys a bit clearer than an if/else that the two cases are session 0 and 1 (with 1 adopting the default). But I can surely change that and explain the case in a comment.
Well, even if we had other sessions, wouldn't they also use WinSta0?
Re: 1-5/6: I'll implement this in the next iteration. Sorry for the noise in review; I am still getting acquainted to the WINE code base and "how things are usually done". Thank you for taking the time to point out the little things!
Thanks. I do hate nitpicking style anyway. One of the odd things with Wine especially is that we don't want people to touch more code than they have to, and generally tell people to match the surrounding style, but then if existing code is stylistically ugly we'd prefer that to be fixed if you're touching it... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9843#note_127475
participants (3)
-
Elizabeth Figura (＠zfigura) -
Katharina Bogad
-
Katharina Bogad (＠hackathi)