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