From cf9f0ce6a9f6924df1c671d5bbfecc857777aac1 Mon Sep 17 00:00:00 2001 From: Mykhailo Pylyp Date: Tue, 22 Jun 2021 15:24:03 +0300 Subject: [PATCH] [PowerToys Run] Issues with elevation (#11775) * Delegate creation of non elevated process * Error handling * nits * Fix potential race condition * Fix spelling * Fix spelling * Fix build --- .github/actions/spell-check/expect.txt | 8 + src/common/utils/elevation.h | 145 ++++++++++++++++++ .../launcher/Microsoft.Launcher/dllmain.cpp | 99 ++---------- .../launcher/PowerLauncher/App.xaml.cs | 61 +++++--- .../PowerLauncher/Helper/SingleInstance`1.cs | 12 +- .../powerpreview/powerpreview.vcxproj | 3 + 6 files changed, 215 insertions(+), 113 deletions(-) diff --git a/.github/actions/spell-check/expect.txt b/.github/actions/spell-check/expect.txt index 1e1236cb7b..71723885a4 100644 --- a/.github/actions/spell-check/expect.txt +++ b/.github/actions/spell-check/expect.txt @@ -649,6 +649,7 @@ examplepowertoy EXCLUDEFILES EXCLUDEFOLDERS EXCLUDESUBFOLDERS +exdisp exe Executables executionpolicy @@ -1147,6 +1148,7 @@ Lessthan LEVELID LExit lhs +lhwnd LIBID LIGHTBLUE LIGHTGRAY @@ -1395,6 +1397,7 @@ NCRBUTTONDOWN NCRBUTTONUP NDEBUG ndp +NEEDDISPATCH Nefario neq NESW @@ -1959,6 +1962,7 @@ Soref SORTDOWN SOURCECLIENTAREAONLY SOURCEHEADER +spdisp spdlog spdo spdth @@ -1975,6 +1979,7 @@ spsia spsrif spsrm spsrui +spsv sql src SRCCOPY @@ -2052,8 +2057,11 @@ surfacehub sut SVE svg +SVGIO SVGIn svgpreviewhandler +SWC +SWFO Switchbetweenvirtualdesktops SWP swprintf diff --git a/src/common/utils/elevation.h b/src/common/utils/elevation.h index 0fef3dc84c..93e450675d 100644 --- a/src/common/utils/elevation.h +++ b/src/common/utils/elevation.h @@ -4,6 +4,12 @@ #include #include #include +#include +#include +#include +#include +#include +#include #include #include @@ -12,6 +18,119 @@ #include #include +namespace +{ + inline std::wstring GetErrorString(HRESULT handle) + { + _com_error err(handle); + return err.ErrorMessage(); + } + + inline bool FindDesktopFolderView(REFIID riid, void** ppv) + { + CComPtr spShellWindows; + auto result = spShellWindows.CoCreateInstance(CLSID_ShellWindows); + if (result != S_OK) + { + Logger::warn(L"Failed to create instance. {}", GetErrorString(result)); + return false; + } + + CComVariant vtLoc(CSIDL_DESKTOP); + CComVariant vtEmpty; + long lhwnd; + CComPtr spdisp; + result = spShellWindows->FindWindowSW( + &vtLoc, &vtEmpty, SWC_DESKTOP, &lhwnd, SWFO_NEEDDISPATCH, &spdisp); + + if (result != S_OK) + { + Logger::warn(L"Failed to find the window. {}", GetErrorString(result)); + return false; + } + + CComPtr spBrowser; + result = CComQIPtr(spdisp)->QueryService(SID_STopLevelBrowser, + IID_PPV_ARGS(&spBrowser)); + if (result != S_OK) + { + Logger::warn(L"Failed to query service. {}", GetErrorString(result)); + return false; + } + + CComPtr spView; + result = spBrowser->QueryActiveShellView(&spView); + if (result != S_OK) + { + Logger::warn(L"Failed to query active shell window. {}", GetErrorString(result)); + return false; + } + + result = spView->QueryInterface(riid, ppv); + if (result != S_OK) + { + Logger::warn(L"Failed to query interface. {}", GetErrorString(result)); + return false; + } + + return true; + } + + inline bool GetDesktopAutomationObject(REFIID riid, void** ppv) + { + CComPtr spsv; + if (!FindDesktopFolderView(IID_PPV_ARGS(&spsv))) + { + return false; + } + + CComPtr spdispView; + auto result = spsv->GetItemObject(SVGIO_BACKGROUND, IID_PPV_ARGS(&spdispView)); + if (result != S_OK) + { + Logger::warn(L"GetItemObject() failed. {}", GetErrorString(result)); + return false; + } + + result = spdispView->QueryInterface(riid, ppv); + if (result != S_OK) + { + Logger::warn(L"QueryInterface() failed. {}", GetErrorString(result)); + return false; + } + + return true; + } + + inline bool ShellExecuteFromExplorer( + PCWSTR pszFile, + PCWSTR pszParameters = nullptr) + { + CComPtr spFolderView; + if (!GetDesktopAutomationObject(IID_PPV_ARGS(&spFolderView))) + { + return false; + } + + CComPtr spdispShell; + auto result = spFolderView->get_Application(&spdispShell); + if (result != S_OK) + { + Logger::warn(L"get_Application() failed. {}", GetErrorString(result)); + return false; + } + + CComQIPtr(spdispShell) + ->ShellExecute(CComBSTR(pszFile), + CComVariant(pszParameters ? pszParameters : L""), + CComVariant(L""), + CComVariant(L""), + CComVariant(SW_SHOWNORMAL)); + + return true; + } +} + // Returns true if the current process is running with elevated privileges inline bool is_process_elevated(const bool use_cached_value = true) { @@ -186,6 +305,32 @@ inline bool run_non_elevated(const std::wstring& file, const std::wstring& param return succeeded; } +inline bool RunNonElevatedEx(const std::wstring& file, const std::wstring& params) +{ + bool failedToStart = false; + try + { + CoInitialize(nullptr); + if (!ShellExecuteFromExplorer(file.c_str(), params.c_str())) + { + failedToStart = true; + } + } + catch(...) + { + failedToStart = true; + } + + if (failedToStart) + { + Logger::warn(L"Failed to delegate process creation. Try a fallback"); + DWORD returnPid; + return run_non_elevated(file, params, &returnPid); + } + + return true; +} + // Run command with the same elevation, returns true if succeeded inline bool run_same_elevation(const std::wstring& file, const std::wstring& params, DWORD* returnPid) { diff --git a/src/modules/launcher/Microsoft.Launcher/dllmain.cpp b/src/modules/launcher/Microsoft.Launcher/dllmain.cpp index 75f571c355..6f7777d4cd 100644 --- a/src/modules/launcher/Microsoft.Launcher/dllmain.cpp +++ b/src/modules/launcher/Microsoft.Launcher/dllmain.cpp @@ -59,8 +59,7 @@ private: // Load initial settings from the persisted values. void init_settings(); - // Handle to launch and terminate the launcher - HANDLE m_hProcess = nullptr; + bool processStarted = false; //contains the name of the powerToys std::wstring app_name; @@ -122,10 +121,6 @@ public: ~Microsoft_Launcher() { Logger::info("Launcher object is destroying"); - if (m_enabled) - { - terminateProcess(); - } m_enabled = false; } @@ -225,8 +220,8 @@ public: if (ShellExecuteExW(&sei)) { - m_hProcess = sei.hProcess; - Logger::trace("Started PowerToys Run. Handle {}", m_hProcess); + processStarted = true; + Logger::trace("Started PowerToys Run"); } else { @@ -236,55 +231,20 @@ public: else { Logger::trace("Starting PowerToys Run from elevated process"); - std::wstring action_runner_path = get_module_folderpath(); - + std::wstring runExecutablePath = get_module_folderpath(); std::wstring params; - params += L"-run-non-elevated "; - params += L"-target modules\\launcher\\PowerLauncher.exe "; - params += L"-pidFile "; - params += POWER_LAUNCHER_PID_SHARED_FILE; params += L" -powerToysPid " + std::to_wstring(powertoys_pid) + L" "; params += L"--centralized-kb-hook "; - - action_runner_path += L"\\PowerToys.ActionRunner.exe"; - // Set up the shared file from which to retrieve the PID of PowerLauncher - HANDLE hMapFile = CreateFileMappingW(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, 0, sizeof(DWORD), POWER_LAUNCHER_PID_SHARED_FILE); - if (!hMapFile) + runExecutablePath += L"\\modules\\launcher\\PowerLauncher.exe"; + if (RunNonElevatedEx(runExecutablePath, params)) { - auto err = get_last_error_message(GetLastError()); - Logger::error(L"Failed to create FileMapping {}. {}", POWER_LAUNCHER_PID_SHARED_FILE, err.has_value() ? err.value() : L""); - return; + processStarted = true; + Logger::trace(L"The process started successfully"); } - - PDWORD pidBuffer = reinterpret_cast(MapViewOfFile(hMapFile, FILE_MAP_ALL_ACCESS, 0, 0, sizeof(DWORD))); - if (pidBuffer) + else { - *pidBuffer = 0; - m_hProcess = NULL; - - if (run_non_elevated(action_runner_path, params, pidBuffer)) - { - Logger::trace("Started PowerToys Run Process"); - const int maxRetries = 80; - for (int retry = 0; retry < maxRetries; ++retry) - { - Sleep(50); - DWORD pid = *pidBuffer; - if (pid) - { - m_hProcess = OpenProcess(PROCESS_TERMINATE | PROCESS_QUERY_INFORMATION | SYNCHRONIZE, FALSE, pid); - Logger::trace("Opened PowerToys Run Process. Handle {}", m_hProcess); - break; - } - } - } - else - { - // We expect it to fail if the shell window is not available. It can happen on startup - Logger::warn("Failed to start PowerToys Run"); - } + Logger::error(L"Failed to start the process"); } - CloseHandle(hMapFile); } } @@ -294,9 +254,10 @@ public: Logger::info("Launcher is disabling"); if (m_enabled) { + TerminateRunningInstance(); + processStarted = false; ResetEvent(m_hEvent); ResetEvent(send_telemetry_event); - terminateProcess(); } m_enabled = false; @@ -332,17 +293,13 @@ public: // For now, hotkeyId will always be zero if (m_enabled) { - if (m_hProcess == nullptr) + if (!processStarted) { Logger::warn("PowerToys Run hasn't been started. Starting PowerToys Run"); enable(); - } else if (WaitForSingleObject(m_hProcess, 0) == WAIT_OBJECT_0) - { - Logger::warn("PowerToys Run has exited unexpectedly, restarting PowerToys Run."); - enable(); } - Logger::trace("Set POWER_LAUNCHER_SHARED_EVENT. Handle {}", m_hProcess); + Logger::trace("Set POWER_LAUNCHER_SHARED_EVENT"); SetEvent(m_hEvent); return true; } @@ -362,34 +319,6 @@ public: return true; } - // Terminate process by sending WM_CLOSE signal and if it fails, force terminate. - void terminateProcess() - { - Logger::trace(L"Terminating PowerToys Run process. Handle {}.", m_hProcess); - if (WaitForSingleObject(m_hProcess, 0) == WAIT_OBJECT_0) - { - Logger::warn("PowerToys Run has exited unexpectedly, so there is no need to terminate it."); - return; - } - - DWORD processID = GetProcessId(m_hProcess); - if (TerminateProcess(m_hProcess, 1) == 0) - { - auto err = get_last_error_message(GetLastError()); - Logger::error(L"Launcher process was not terminated. {}", err.has_value() ? err.value() : L""); - } - - // Temporarily disable sending a message to close - /* - EnumWindows(&requestMainWindowClose, processID); - const DWORD result = WaitForSingleObject(m_hProcess, MAX_WAIT_MILLISEC); - if (result == WAIT_TIMEOUT || result == WAIT_FAILED) - { - TerminateProcess(m_hProcess, 1); - } - */ - } - virtual void send_settings_telemetry() override { Logger::info("Send settings telemetry"); diff --git a/src/modules/launcher/PowerLauncher/App.xaml.cs b/src/modules/launcher/PowerLauncher/App.xaml.cs index 33e5f0e441..bdeac9a6aa 100644 --- a/src/modules/launcher/PowerLauncher/App.xaml.cs +++ b/src/modules/launcher/PowerLauncher/App.xaml.cs @@ -6,6 +6,7 @@ using System; using System.Diagnostics; using System.Linq; using System.Text; +using System.Threading; using System.Windows; using interop; using ManagedCommon; @@ -43,34 +44,46 @@ namespace PowerLauncher public static void Main() { Log.Info($"Starting PowerToys Run with PID={Process.GetCurrentProcess().Id}", typeof(App)); - if (SingleInstance.InitializeAsFirstInstance()) + int powerToysPid = GetPowerToysPId(); + if (powerToysPid != 0) { - using (var application = new App()) - { - application.InitializeComponent(); - NativeEventWaiter.WaitForEventLoop(Constants.RunExitEvent(), () => - { - Log.Warn("RunExitEvent was signaled. Exiting PowerToys", typeof(App)); - ExitPowerToys(application); - }); - - int powerToysPid = GetPowerToysPId(); - if (powerToysPid != 0) - { - Log.Info($"Runner pid={powerToysPid}", typeof(App)); - RunnerHelper.WaitForPowerToysRunner(powerToysPid, () => - { - Log.Info($"Runner with pid={powerToysPid} exited. Exiting PowerToys Run", typeof(App)); - ExitPowerToys(application); - }); - } - - application.Run(); - } + // The process started from the PT Run module interface. One instance is handled there. + Log.Info($"Runner pid={powerToysPid}", typeof(App)); + SingleInstance.CreateInstanceMutex(); } else { - Log.Error("There is already running PowerToys Run instance. Exiting PowerToys Run", typeof(App)); + // If PT Run is started as standalone application check if there is already running instance + if (!SingleInstance.InitializeAsFirstInstance()) + { + Log.Warn("There is already running PowerToys Run instance. Exiting PowerToys Run", typeof(App)); + return; + } + } + + using (var application = new App()) + { + application.InitializeComponent(); + new Thread(() => + { + var eventHandle = new EventWaitHandle(false, EventResetMode.AutoReset, Constants.RunExitEvent()); + if (eventHandle.WaitOne()) + { + Log.Warn("RunExitEvent was signaled. Exiting PowerToys", typeof(App)); + ExitPowerToys(application); + } + }).Start(); + + if (powerToysPid != 0) + { + RunnerHelper.WaitForPowerToysRunner(powerToysPid, () => + { + Log.Info($"Runner with pid={powerToysPid} exited. Exiting PowerToys Run", typeof(App)); + ExitPowerToys(application); + }); + } + + application.Run(); } } diff --git a/src/modules/launcher/PowerLauncher/Helper/SingleInstance`1.cs b/src/modules/launcher/PowerLauncher/Helper/SingleInstance`1.cs index 2ba43a081a..66dccba85a 100644 --- a/src/modules/launcher/PowerLauncher/Helper/SingleInstance`1.cs +++ b/src/modules/launcher/PowerLauncher/Helper/SingleInstance`1.cs @@ -44,12 +44,18 @@ namespace PowerLauncher.Helper /// Suffix to the channel name. /// private const string ChannelNameSuffix = "SingeInstanceIPCChannel"; + private const string InstanceMutexName = @"Local\PowerToys_Run_InstanceMutex"; /// /// Gets or sets application mutex. /// internal static Mutex SingleInstanceMutex { get; set; } + internal static void CreateInstanceMutex() + { + SingleInstanceMutex = new Mutex(true, InstanceMutexName, out bool firstInstance); + } + /// /// Checks if the instance of the application attempting to start is the first instance. /// If not, activates the first instance. @@ -57,14 +63,12 @@ namespace PowerLauncher.Helper /// True if this is the first instance of the application. internal static bool InitializeAsFirstInstance() { - string mutexName = @"Local\PowerToys_Run_InstanceMutex"; - // Build unique application Id and the IPC channel name. - string applicationIdentifier = mutexName + Environment.UserName; + string applicationIdentifier = InstanceMutexName + Environment.UserName; string channelName = string.Concat(applicationIdentifier, Delimiter, ChannelNameSuffix); - SingleInstanceMutex = new Mutex(true, mutexName, out bool firstInstance); + SingleInstanceMutex = new Mutex(true, InstanceMutexName, out bool firstInstance); if (firstInstance) { _ = CreateRemoteService(channelName); diff --git a/src/modules/previewpane/powerpreview/powerpreview.vcxproj b/src/modules/previewpane/powerpreview/powerpreview.vcxproj index 4b7898d9b8..456a5301b9 100644 --- a/src/modules/previewpane/powerpreview/powerpreview.vcxproj +++ b/src/modules/previewpane/powerpreview/powerpreview.vcxproj @@ -77,6 +77,9 @@ {caba8dfb-823b-4bf2-93ac-3f31984150d9} + + {d9b8fc84-322a-4f9f-bbb9-20915c47ddfd} + {1d5be09d-78c0-4fd7-af00-ae7c1af7c525}