Skip to content

Launch games under the correct user account on Windows - #600

Merged
ReenigneArcher merged 2 commits into
LizardByte:nightlyfrom
cgutman:unprivileged_launch
Dec 27, 2022
Merged

ReenigneArcher merged 2 commits into
LizardByte:nightlyfrom
cgutman:unprivileged_launch

Conversation

@cgutman

@cgutman cgutman commented Dec 21, 2022 •

Copy link
Copy Markdown
Collaborator

Description

This implements a new platf::run_unprivileged() function that can be used (on Windows, currently) to launch a process as the active console user (including their environment variables).

In order to make this work, I had to fix issues with the way we derive the working directory if one is not provided. That required adding a dependency on libboost-program-options for boost::program_options::split_unix() to perform proper command-line parsing to retrieve the actual binary name.

This could definitely use some testing since there are a ton of use cases to cover.

Screenshot

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@psyke83

psyke83 commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

I believe that something may be wrong with path handling (but it applies to this PR and not #599). I tested with and without the complementary PR in case it helps to troubleshoot.

Test case (3 created test applications)

  • Add Notepad1, with command as notepad
  • Add Notepad2, with command as C:\Windows\System32\notepad.exe
  • Add Notepad3, with command as C:\Windows\System32\notepad.exe and working directory as C:\Windows\System32\

Results:

In summary, this PR makes notepad available (as it is in $Path), but C:\Windows\System32\notepad.exe fails to launch unless you manually add the working directory (which should not be necessary according to the Web UI's description).

Comment thread src/process.cpp Outdated
@ReenigneArcher

Copy link
Copy Markdown
Member

@cgutman
cgutman force-pushed the unprivileged_launch branch 2 times, most recently from 74886f8 to 83f1b99 Compare December 21, 2022 17:46
@cgutman

cgutman commented Dec 21, 2022

Copy link
Copy Markdown
Collaborator Author

OK, I think I've taken care of all the issues.

  • I've changed the behavior for detached commands to use the given working directory as discussed with @ReenigneArcher on Discord (previously working directory was unset for those commands)
  • Fixed working directory lookup issues caused by incorrect command parsing (Notepad1, 2, and 3 from @psyke83 's test should be working now)
  • Switched from ANSI to Unicode functions for process and environment creation. I verified Unicode support is working with non-ASCII working directories, commands, and environment variables (both in the user environment block and from apps.json in env).
  • Fixed Flatpak build by adding missing program_options library)
  • Fixed clang-format issue (seems to disagree with my local clang-format but I'll defer to the bot)

@cgutman
cgutman marked this pull request as ready for review December 21, 2022 17:53
@ReenigneArcher

Copy link
Copy Markdown
Member

Fixed clang-format issue (seems to disagree with my local clang-format but I'll defer to the bot)

If you have any suggestions for changes to the .clang-format file, I'm happy to discuss. That file was provided to me early after I took over sunshine. One thing I personally don't like is the alignment of variable values, but I guess that's normal in c++.

@ReenigneArcher
ReenigneArcher marked this pull request as draft December 22, 2022 04:24
@Nonary

This comment was marked as spam.

@ReenigneArcher

Copy link
Copy Markdown
Member

Several users on the discord had tested this build and I don't recall any issues. We would likely get more testing if merged into nightly.

This PR is not ready to merge.

@cgutman
cgutman force-pushed the unprivileged_launch branch from 83f1b99 to 7b5af0e Compare December 27, 2022 19:39
@cgutman
cgutman force-pushed the unprivileged_launch branch from 7b5af0e to 128c9c9 Compare December 27, 2022 19:40
@cgutman
cgutman marked this pull request as ready for review December 27, 2022 19:40
@cgutman

cgutman commented Dec 27, 2022

Copy link
Copy Markdown
Collaborator Author

platf::run_unprivileged() changes:

  • Added the ImpersonateLoggedOnUser() call to ensure that access checks are done against the user token rather than our SYSTEM token. This also allows launching apps on a network share or network drive.
  • Addressed handle inheritance issues in a cleaner way (using PROC_THREAD_ATTRIBUTE_HANDLE_LIST rather than manually disabling inheritance on Sunshine's own std handles)
  • Added CREATE_BREAKAWAY_FROM_JOB to handle changes in Spawn Sunshine.exe in a job object, so it is terminated if SunshineSvc.exe dies #602 that would otherwise terminate the child process if SunshineSvc died
  • Added CREATE_NEW_CONSOLE to handle the case where log output is not redirected and incorrectly ends up in Sunshine's own log file
  • Removed bp::child() fallback if CreateProcessAsUser() fails. I left that fallback in place just in case CreateProcessAsUser() failed when running as non-admin, but that case seems to work just fine (CreateProcessAsUser() and ImpersonateLoggedOnUser() using your own user token requires no additional permissions).

Log file fixes:

  • Fixed Unicode log file path handling on Windows (previously, attempts to use non-ANSI log file paths would silently fail)
  • Added _SH_DENYNO to ensure reopening log files for applications that are still running will not fail with a sharing violation.

These changes also fix the crash that we saw with UWP notepad.exe with the previous version of this PR.

@ReenigneArcher
ReenigneArcher merged commit 05f5370 into LizardByte:nightly Dec 27, 2022
@Michoko92

Copy link
Copy Markdown

Awesome job! 👍

KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Dec 28, 2022
@cgutman cgutman mentioned this pull request Jan 1, 2023
2 tasks done
@exalented

exalented commented Jan 9, 2023 •

Copy link
Copy Markdown
Contributor

With the 17 release I can no longer launch an app on linux without sunshine core dumping:
Platform: Arch / Sway / Nvidia, running AUR package.

Warning: run_unprivileged() is not yet implemented for this platform. The new process will run with Sunshine's permissions.
terminate called after throwing an instance of 'boost::process::process_error'
  what():  killpg(2) failed in terminate: Invalid argument
Aborted (core dumped)

@FrogTheFrog FrogTheFrog mentioned this pull request Jan 14, 2023
4 of 11 tasks
@ReenigneArcher ReenigneArcher mentioned this pull request Apr 1, 2023
7 of 11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants