Skip to content

Commit

Permalink
Disable the VT color quirk for pwsh and modern inbox powershell (#13352)
Browse files Browse the repository at this point in the history
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]: PowerShell/PSReadLine#830 (comment)
  • Loading branch information
DHowett authored Jun 23, 2022
1 parent 72a4e93 commit 8962f88
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 43 deletions.
62 changes: 27 additions & 35 deletions src/server/ConsoleShimPolicy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::wstring>(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:
Expand Down Expand Up @@ -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;
}
11 changes: 4 additions & 7 deletions src/server/ConsoleShimPolicy.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
2 changes: 1 addition & 1 deletion src/server/ProcessHandle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down

0 comments on commit 8962f88

Please sign in to comment.