From 8962f88f907d86fd8684b66f7f3e32a2709e3237 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Thu, 23 Jun 2022 10:40:27 -0500 Subject: [PATCH] Disable the VT color quirk for pwsh and modern inbox powershell (#13352) In #6810, we introduced a "quirk" for all known versions of PowerShell that suppressed their requests for black background/gray foreground. This was done to avoid an [issue in PSReadline] where it would paint black bars all over the screen if the default background color wasn't the same as the ANSI black color. Years have passed since that quirk was introduced. The underlying bug was fixed, and the fix was released broadly long ago. It's time for us to remove the quirk... almost. Terminal still runs on versions of Windows that ship a broken version of PSReadline. We must maintain the quirk there -- the user can't do anything about it, and we would make their experience worse if we removed the quirk entirely. PowerShell 7.0 also ships a broken version of PSReadline. It is still in support for another 6 months, but updates have been available for some time. We can encourage users to update. Therefore, we only need the quirk for Windows PowerShell, and then only for specific versions of Windows. _Inside Windows_, we don't even need that: we're guaranteed to be built alongside a fixed version of PowerShell! Closes #6807 [issue in PSReadline]: https://github.com/PowerShell/PSReadLine/issues/830#issuecomment-650508857 --- src/server/ConsoleShimPolicy.cpp | 62 ++++++++++++++------------------ src/server/ConsoleShimPolicy.h | 11 +++--- src/server/ProcessHandle.cpp | 2 +- 3 files changed, 32 insertions(+), 43 deletions(-) diff --git a/src/server/ConsoleShimPolicy.cpp b/src/server/ConsoleShimPolicy.cpp index b4f0870ccf1..7f6cc56d690 100644 --- a/src/server/ConsoleShimPolicy.cpp +++ b/src/server/ConsoleShimPolicy.cpp @@ -9,51 +9,44 @@ // - Constructs a new instance of the shim policy class. // Arguments: // - All arguments specify a true/false status to a policy that could be applied to a console client app. -ConsoleShimPolicy::ConsoleShimPolicy(const bool isCmd, - const bool isPowershell) : - _isCmd{ isCmd }, - _isPowershell{ isPowershell } -{ -} - -// Routine Description: -// - Opens the process token for the given handle and resolves the process name. -// We'll initialize the new ConsoleShimPolicy based on whether the client -// process is "cmd.exe" or "powershell.exe". -// - For more info, see GH#3126 -// Arguments: -// - hProcess - Handle to a connected process -// Return Value: -// - ConsoleShimPolicy object containing resolved shim policy data. -ConsoleShimPolicy ConsoleShimPolicy::s_CreateInstance(const HANDLE hProcess) +ConsoleShimPolicy::ConsoleShimPolicy(const HANDLE hProcess) { // If we cannot determine the exe name, then we're probably not cmd or powershell. - auto isCmd = false; - auto isPowershell = false; - try { const std::filesystem::path processName = wil::GetModuleFileNameExW(hProcess, nullptr); - auto clientName = processName.filename().wstring(); - // For whatever reason, wil::GetModuleFileNameExW leaves trailing nulls, so get rid of them. - clientName.erase(std::find(clientName.begin(), clientName.end(), '\0'), clientName.end()); - - // Convert to lower case, just in case - std::transform(clientName.begin(), clientName.end(), clientName.begin(), std::towlower); + const auto clientName = processName.filename().native(); - isCmd = clientName.compare(L"cmd.exe") == 0; + _isCmd = til::equals_insensitive_ascii(clientName, L"cmd.exe"); - // For powershell, we need both Windows PowersShell (powershell.exe) and - // PowerShell Core (pwsh.exe). If PowerShell Core is ever updated to use + // For powershell, we need both Windows Powershell (powershell.exe) and + // Powershell Core (pwsh.exe). If Powershell Core is ever updated to use // ^[[3J for Clear-Host, then it won't ever hit the shim code path, but // we're keeping this for the long tail of pwsh versions that still // _don't_ use that sequence. - isPowershell = (clientName.compare(L"powershell.exe") == 0) || - (clientName.compare(L"pwsh.exe") == 0); + const auto isInboxPowershell = til::equals_insensitive_ascii(clientName, L"powershell.exe"); + const auto isPwsh = til::equals_insensitive_ascii(clientName, L"pwsh.exe"); + _isPowershell = isInboxPowershell || isPwsh; + + // Inside Windows, we are guaranteed that we're building alongside a new (good) inbox Powershell. + // Therefore, we can default _requiresVtColorQuirk to false. +#ifndef __INSIDE_WINDOWS + // Outside of Windows, we need to check the OS version: Powershell was fixed in early Iron builds. + static auto doesInboxPowershellVersionRequireQuirk = [] { + OSVERSIONINFOEXW osver{}; + osver.dwOSVersionInfoSize = sizeof(osver); + osver.dwBuildNumber = 20348; // Windows Server 2022 RTM + + DWORDLONG dwlConditionMask = 0; + VER_SET_CONDITION(dwlConditionMask, VER_BUILDNUMBER, VER_LESS); + + return VerifyVersionInfoW(&osver, VER_BUILDNUMBER, dwlConditionMask) != FALSE; + }(); + _requiresVtColorQuirk = isInboxPowershell && doesInboxPowershellVersionRequireQuirk; + // All modern versions of pwsh.exe have been fixed, and we can direct users to update. +#endif } CATCH_LOG(); - - return ConsoleShimPolicy(isCmd, isPowershell); } // Method Description: @@ -90,6 +83,5 @@ bool ConsoleShimPolicy::IsPowershellExe() const noexcept // - True as laid out above. bool ConsoleShimPolicy::IsVtColorQuirkRequired() const noexcept { - // Right now, the only client we're shimming is powershell. - return IsPowershellExe(); + return _requiresVtColorQuirk; } diff --git a/src/server/ConsoleShimPolicy.h b/src/server/ConsoleShimPolicy.h index e3bd57ad1b8..38c42217100 100644 --- a/src/server/ConsoleShimPolicy.h +++ b/src/server/ConsoleShimPolicy.h @@ -21,16 +21,13 @@ Module Name: class ConsoleShimPolicy { public: - static ConsoleShimPolicy s_CreateInstance(const HANDLE hProcess); - + ConsoleShimPolicy(const HANDLE hProcess); bool IsCmdExe() const noexcept; bool IsPowershellExe() const noexcept; bool IsVtColorQuirkRequired() const noexcept; private: - ConsoleShimPolicy(const bool isCmd, - const bool isPowershell); - - const bool _isCmd; - const bool _isPowershell; + bool _isCmd{ false }; + bool _isPowershell{ false }; + bool _requiresVtColorQuirk{ false }; }; diff --git a/src/server/ProcessHandle.cpp b/src/server/ProcessHandle.cpp index 7c2adfc6417..b9cf2f1b359 100644 --- a/src/server/ProcessHandle.cpp +++ b/src/server/ProcessHandle.cpp @@ -27,7 +27,7 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId, FALSE, dwProcessId))), _policy(ConsoleProcessPolicy::s_CreateInstance(_hProcess.get())), - _shimPolicy(ConsoleShimPolicy::s_CreateInstance(_hProcess.get())), + _shimPolicy(_hProcess.get()), _processCreationTime(0) { if (nullptr != _hProcess.get())