Skip to content

Commit

Permalink
Implement a pair of shims for cls, Clear-Host in conpty mode (#5627)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

This PR implements a pair of shims for `cmd` and `powershell`, so that their `cls` and `Clear-Host` functions will clear the entire terminal buffer (like they do in conhost), instead of just the viewport. With the conpty viewport and buffer being the same size, there's effectively no way to know if an application is calling these API's in this way with the intention of clearing the buffer or the viewport. We absolutely have to guess. 

Each of these shims checks to see if the way that the API is being called exactly matches the way `cmd` or `powershell` would call these APIs. If it does, we manually write a `^[[3J` to the connected terminal, to get he Terminal to clear it's own scrollback.

~~_⚠️ If another application were trying to clear the **viewport** with an exactly similar API call, this would also cause the terminal scrollback to get cleared ⚠️_~~

* [x] Should these shims be restricted to when the process that's calling them is actually `cmd.exe` or `powershell.exe`? Can I even do this? I think we've done such a good job of isolating the client process information from the rest of the host code that I can't figure out how to do this.
  - YES, this can be done, and I did it.
* [ ] **TODO**: _While I'm here_, should I have `DoSrvPrivateEraseAll` (the implementation for `^[[2J`, in `getset.cpp`) also manually trigger a EraseAll in the terminal in conpty mode?

## PR Checklist
* [x] Closes #3126
* [x] Actually closes #1305 too, which is really the same thing, but probably deserves a callout
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
* checked `cls` in the Terminal
* checked `Clear-Host` in the Terminal
* Checked running `powershell clear-host` from `cmd.exe`
  • Loading branch information
zadjii-msft committed Apr 30, 2020
1 parent b6a21de commit 7612044
Show file tree
Hide file tree
Showing 24 changed files with 475 additions and 72 deletions.
184 changes: 147 additions & 37 deletions src/cascadia/UnitTests_TerminalCore/ConptyRoundtripTests.cpp

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions src/host/ApiRoutines.h
Expand Up @@ -129,7 +129,8 @@ class ApiRoutines : public IApiRoutines
const wchar_t character,
const size_t lengthToWrite,
const COORD startingCoordinate,
size_t& cellsModified) noexcept override;
size_t& cellsModified,
const bool enablePowershellShim = false) noexcept override;

//// Process based. Restrict in protocol side?
//HRESULT GenerateConsoleCtrlEventImpl(const ULONG ProcessGroupFilter,
Expand Down Expand Up @@ -179,7 +180,8 @@ class ApiRoutines : public IApiRoutines
const COORD target,
std::optional<SMALL_RECT> clip,
const wchar_t fillCharacter,
const WORD fillAttribute) noexcept override;
const WORD fillAttribute,
const bool enableCmdShim = false) noexcept override;

[[nodiscard]] HRESULT SetConsoleTextAttributeImpl(SCREEN_INFORMATION& context,
const WORD attribute) noexcept override;
Expand Down
24 changes: 22 additions & 2 deletions src/host/VtIo.cpp
Expand Up @@ -440,12 +440,13 @@ void VtIo::EndResize()
// true to `IsUsingVt`, which will cause the console host to act in conpty
// mode.
// Arguments:
// - <none>
// - vtRenderEngine: a VT renderer that our VtIo should use as the vt engine during these tests
// Return Value:
// - <none>
void VtIo::EnableConptyModeForTests()
void VtIo::EnableConptyModeForTests(std::unique_ptr<Microsoft::Console::Render::VtEngine> vtRenderEngine)
{
_objectsCreated = true;
_pVtRenderEngine = std::move(vtRenderEngine);
}
#endif

Expand All @@ -464,3 +465,22 @@ bool VtIo::IsResizeQuirkEnabled() const
{
return _resizeQuirk;
}

// Method Description:
// - Manually tell the renderer that it should emit a "Erase Scrollback"
// sequence to the connected terminal. We need to do this in certain cases
// that we've identified where we believe the client wanted the entire
// terminal buffer cleared, not just the viewport. For more information, see
// GH#3126.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we wrote the sequences successfully, otherwise an appropriate HRESULT
[[nodiscard]] HRESULT VtIo::ManuallyClearScrollback() const noexcept
{
if (_pVtRenderEngine)
{
return _pVtRenderEngine->ManuallyClearScrollback();
}
return S_OK;
}
4 changes: 3 additions & 1 deletion src/host/VtIo.hpp
Expand Up @@ -40,11 +40,13 @@ namespace Microsoft::Console::VirtualTerminal
void EndResize();

#ifdef UNIT_TESTING
void EnableConptyModeForTests();
void EnableConptyModeForTests(std::unique_ptr<Microsoft::Console::Render::VtEngine> vtRenderEngine);
#endif

bool IsResizeQuirkEnabled() const;

[[nodiscard]] HRESULT ManuallyClearScrollback() const noexcept;

private:
// After CreateIoHandlers is called, these will be invalid.
wil::unique_hfile _hInput;
Expand Down
31 changes: 29 additions & 2 deletions src/host/_output.cpp
Expand Up @@ -242,13 +242,17 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
// - lengthToWrite - the number of elements to write
// - startingCoordinate - Screen buffer coordinate to begin writing to.
// - cellsModified - the number of elements written
// - enablePowershellShim - true iff the client process that's calling this
// method is "powershell.exe". Used to enable certain compatibility shims for
// conpty mode. See GH#3126.
// Return Value:
// - S_OK or suitable HRESULT code from failure to write (memory issues, invalid arg, etc.)
[[nodiscard]] HRESULT ApiRoutines::FillConsoleOutputCharacterWImpl(IConsoleOutputObject& OutContext,
const wchar_t character,
const size_t lengthToWrite,
const COORD startingCoordinate,
size_t& cellsModified) noexcept
size_t& cellsModified,
const bool enablePowershellShim) noexcept
{
// Set modified cells to 0 from the beginning.
cellsModified = 0;
Expand All @@ -269,6 +273,7 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
return S_OK;
}

HRESULT hr = S_OK;
try
{
const OutputCellIterator it(character, lengthToWrite);
Expand All @@ -282,10 +287,32 @@ void WriteToScreen(SCREEN_INFORMATION& screenInfo, const Viewport& region)
auto endingCoordinate = startingCoordinate;
bufferSize.MoveInBounds(cellsModified, endingCoordinate);
screenInfo.NotifyAccessibilityEventing(startingCoordinate.X, startingCoordinate.Y, endingCoordinate.X, endingCoordinate.Y);

// GH#3126 - This is a shim for powershell's `Clear-Host` function. In
// the vintage console, `Clear-Host` is supposed to clear the entire
// buffer. In conpty however, there's no difference between the viewport
// and the entirety of the buffer. We're going to see if this API call
// exactly matched the way we expect powershell to call it. If it does,
// then let's manually emit a ^[[3J to the connected terminal, so that
// their entire buffer will be cleared as well.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
if (enablePowershellShim && gci.IsInVtIoMode())
{
const til::size currentBufferDimensions{ screenInfo.GetBufferSize().Dimensions() };

const bool wroteWholeBuffer = lengthToWrite == (currentBufferDimensions.area<size_t>());
const bool startedAtOrigin = startingCoordinate == COORD{ 0, 0 };
const bool wroteSpaces = character == UNICODE_SPACE;

if (wroteWholeBuffer && startedAtOrigin && wroteSpaces)
{
hr = gci.GetVtIo()->ManuallyClearScrollback();
}
}
}
CATCH_RETURN();

return S_OK;
return hr;
}

// Routine Description:
Expand Down
45 changes: 42 additions & 3 deletions src/host/getset.cpp
Expand Up @@ -127,7 +127,13 @@ void ApiRoutines::GetConsoleScreenBufferInfoExImpl(const SCREEN_INFORMATION& con
&data.dwMaximumWindowSize,
&data.wPopupAttributes,
data.ColorTable);
// Callers of this function expect to receive an exclusive rect, not an inclusive one.

// Callers of this function expect to receive an exclusive rect, not an
// inclusive one. The driver will mangle this value for us
// - For GetConsoleScreenBufferInfoEx, it will re-decrement these values
// to return an inclusive rect.
// - For GetConsoleScreenBufferInfo, it will leave these values
// untouched, returning an exclusive rect.
data.srWindow.Right += 1;
data.srWindow.Bottom += 1;
}
Expand Down Expand Up @@ -828,14 +834,18 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
// - clip - The rectangle inside which all operations should be bounded (or no bounds if not given)
// - fillCharacter - Fills in the region left behind when the source is "lifted" out of its original location. The symbol to display.
// - fillAttribute - Fills in the region left behind when the source is "lifted" out of its original location. The color to use.
// - enableCmdShim - true iff the client process that's calling this
// method is "cmd.exe". Used to enable certain compatibility shims for
// conpty mode. See GH#3126.
// Return Value:
// - S_OK, E_INVALIDARG, or failure code from thrown exception
[[nodiscard]] HRESULT ApiRoutines::ScrollConsoleScreenBufferWImpl(SCREEN_INFORMATION& context,
const SMALL_RECT& source,
const COORD target,
std::optional<SMALL_RECT> clip,
const wchar_t fillCharacter,
const WORD fillAttribute) noexcept
const WORD fillAttribute,
const bool enableCmdShim) noexcept
{
try
{
Expand All @@ -847,7 +857,36 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
TextAttribute useThisAttr(fillAttribute);
ScrollRegion(buffer, source, clip, target, fillCharacter, useThisAttr);

return S_OK;
HRESULT hr = S_OK;

// GH#3126 - This is a shim for cmd's `cls` function. In the
// legacy console, `cls` is supposed to clear the entire buffer. In
// conpty however, there's no difference between the viewport and the
// entirety of the buffer. We're going to see if this API call exactly
// matched the way we expect cmd to call it. If it does, then
// let's manually emit a ^[[3J to the connected terminal, so that their
// entire buffer will be cleared as well.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
if (enableCmdShim && gci.IsInVtIoMode())
{
const auto currentBufferDimensions = buffer.GetBufferSize().Dimensions();
const bool sourceIsWholeBuffer = (source.Top == 0) &&
(source.Left == 0) &&
(source.Right == currentBufferDimensions.X) &&
(source.Bottom == currentBufferDimensions.Y);
const bool targetIsNegativeBufferHeight = (target.X == 0) &&
(target.Y == -currentBufferDimensions.Y);
const bool noClipProvided = clip == std::nullopt;
const bool fillIsBlank = (fillCharacter == UNICODE_SPACE) &&
(fillAttribute == gci.GenerateLegacyAttributes(buffer.GetAttributes()));

if (sourceIsWholeBuffer && targetIsNegativeBufferHeight && noClipProvided && fillIsBlank)
{
hr = gci.GetVtIo()->ManuallyClearScrollback();
}
}

return hr;
}
CATCH_RETURN();
}
Expand Down
6 changes: 3 additions & 3 deletions src/host/globals.cpp
Expand Up @@ -23,12 +23,12 @@ bool Globals::IsHeadless() const
// true to `IsHeadless`, which will cause the console host to act in conpty
// mode.
// Arguments:
// - <none>
// - vtRenderEngine: a VT renderer that our VtIo should use as the vt engine during these tests
// Return Value:
// - <none>
void Globals::EnableConptyModeForTests()
void Globals::EnableConptyModeForTests(std::unique_ptr<Microsoft::Console::Render::VtEngine> vtRenderEngine)
{
launchArgs.EnableConptyModeForTests();
getConsoleInformation().GetVtIo()->EnableConptyModeForTests();
getConsoleInformation().GetVtIo()->EnableConptyModeForTests(std::move(vtRenderEngine));
}
#endif
2 changes: 1 addition & 1 deletion src/host/globals.h
Expand Up @@ -70,7 +70,7 @@ class Globals
ApiRoutines api;

#ifdef UNIT_TESTING
void EnableConptyModeForTests();
void EnableConptyModeForTests(std::unique_ptr<Microsoft::Console::Render::VtEngine> vtRenderEngine);
#endif

private:
Expand Down
24 changes: 9 additions & 15 deletions src/host/ut_host/ConptyOutputTests.cpp
Expand Up @@ -85,23 +85,23 @@ class ConptyOutputTests
wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE);
Viewport initialViewport = currentBuffer.GetViewport();

_pVtRenderEngine = std::make_unique<Xterm256Engine>(std::move(hFile),
gci,
initialViewport,
gci.GetColorTable(),
static_cast<WORD>(gci.GetColorTableSize()));
auto vtRenderEngine = std::make_unique<Xterm256Engine>(std::move(hFile),
gci,
initialViewport,
gci.GetColorTable(),
static_cast<WORD>(gci.GetColorTableSize()));
auto pfn = std::bind(&ConptyOutputTests::_writeCallback, this, std::placeholders::_1, std::placeholders::_2);
_pVtRenderEngine->SetTestCallback(pfn);
vtRenderEngine->SetTestCallback(pfn);

g.pRender->AddRenderEngine(_pVtRenderEngine.get());
gci.GetActiveOutputBuffer().SetTerminalConnection(_pVtRenderEngine.get());
g.pRender->AddRenderEngine(vtRenderEngine.get());
gci.GetActiveOutputBuffer().SetTerminalConnection(vtRenderEngine.get());

expectedOutput.clear();

// Manually set the console into conpty mode. We're not actually going
// to set up the pipes for conpty, but we want the console to behave
// like it would in conpty mode.
g.EnableConptyModeForTests();
g.EnableConptyModeForTests(std::move(vtRenderEngine));

return true;
}
Expand All @@ -128,7 +128,6 @@ class ConptyOutputTests
bool _writeCallback(const char* const pch, size_t const cch);
void _flushFirstFrame();
std::deque<std::string> expectedOutput;
std::unique_ptr<Microsoft::Console::Render::VtEngine> _pVtRenderEngine;
std::unique_ptr<CommonState> m_state;
};

Expand Down Expand Up @@ -202,7 +201,6 @@ void ConptyOutputTests::ConptyOutputTestCanary()
{
Log::Comment(NoThrowString().Format(
L"This is a simple test to make sure that everything is working as expected."));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

_flushFirstFrame();
}
Expand All @@ -212,7 +210,6 @@ void ConptyOutputTests::SimpleWriteOutputTest()
Log::Comment(NoThrowString().Format(
L"Write some simple output, and make sure it gets rendered largely "
L"unmodified to the terminal"));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
Expand All @@ -232,7 +229,6 @@ void ConptyOutputTests::WriteTwoLinesUsesNewline()
{
Log::Comment(NoThrowString().Format(
L"Write two lines of output. We should use \r\n to move the cursor"));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
Expand Down Expand Up @@ -271,7 +267,6 @@ void ConptyOutputTests::WriteAFewSimpleLines()
{
Log::Comment(NoThrowString().Format(
L"Write more lines of output. We should use \r\n to move the cursor"));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
Expand Down Expand Up @@ -330,7 +325,6 @@ void ConptyOutputTests::InvalidateUntilOneBeforeEnd()
{
Log::Comment(NoThrowString().Format(
L"Make sure we don't use EL and wipe out the last column of text"));
VERIFY_IS_NOT_NULL(_pVtRenderEngine.get());

auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
Expand Down
8 changes: 8 additions & 0 deletions src/inc/til/size.h
Expand Up @@ -250,6 +250,14 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
return result;
}

template<typename T>
T area() const
{
T ret;
THROW_HR_IF(E_ABORT, !base::CheckMul(_width, _height).AssignIfValid(&ret));
return ret;
}

#ifdef _WINCONTYPES_
operator COORD() const
{
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/vt/VtSequences.cpp
Expand Up @@ -109,6 +109,11 @@ using namespace Microsoft::Console::Render;
return _Write("\x1b[2J");
}

[[nodiscard]] HRESULT VtEngine::_ClearScrollback() noexcept
{
return _Write("\x1b[3J");
}

// Method Description:
// - Formats and writes a sequence to either insert or delete a number of lines
// into the buffer at the current cursor location.
Expand Down
14 changes: 14 additions & 0 deletions src/renderer/vt/Xterm256Engine.cpp
Expand Up @@ -110,3 +110,17 @@ Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe,

return S_OK;
}

// Method Description:
// - Manually emit a "Erase Scrollback" sequence to the connected terminal. We
// need to do this in certain cases that we've identified where we believe the
// client wanted the entire terminal buffer cleared, not just the viewport.
// For more information, see GH#3126.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we wrote the sequences successfully, otherwise an appropriate HRESULT
[[nodiscard]] HRESULT Xterm256Engine::ManuallyClearScrollback() noexcept
{
return _ClearScrollback();
}
2 changes: 2 additions & 0 deletions src/renderer/vt/Xterm256Engine.hpp
Expand Up @@ -37,6 +37,8 @@ namespace Microsoft::Console::Render
const ExtendedAttributes extendedAttrs,
const bool isSettingDefaultBrushes) noexcept override;

[[nodiscard]] HRESULT ManuallyClearScrollback() noexcept override;

private:
[[nodiscard]] HRESULT _UpdateExtendedAttrs(const ExtendedAttributes extendedAttrs) noexcept;

Expand Down
17 changes: 17 additions & 0 deletions src/renderer/vt/state.cpp
Expand Up @@ -492,3 +492,20 @@ void VtEngine::SetResizeQuirk(const bool resizeQuirk)
{
_resizeQuirk = resizeQuirk;
}

// Method Description:
// - Manually emit a "Erase Scrollback" sequence to the connected terminal. We
// need to do this in certain cases that we've identified where we believe the
// client wanted the entire terminal buffer cleared, not just the viewport.
// For more information, see GH#3126.
// - This is unimplemented in the win-telnet, xterm-ascii renderers - inbox
// telnet.exe doesn't know how to handle a ^[[3J. This _is_ implemented in the
// Xterm256Engine.
// Arguments:
// - <none>
// Return Value:
// - S_OK if we wrote the sequences successfully, otherwise an appropriate HRESULT
[[nodiscard]] HRESULT VtEngine::ManuallyClearScrollback() noexcept
{
return S_OK;
}

0 comments on commit 7612044

Please sign in to comment.