From a49a302dfdb3401f75dc4cbb44658f0ecc43567a Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 9 Sep 2020 13:21:37 -0700 Subject: [PATCH 1/2] Switch all DSR responses to appending instead of prepending This fixes an issue where two CPRs could end up corrupted in the input buffer. An application that sent two CPRs back-to-back could accidentally read the first few characters of the first prepended CPR before handing us another CPR. We would dutifully prepend it to the buffer, causing them to overlap. ``` ^[^[2;2R[1;1R ^^ ^^^^^ First CPR ^^^^^^ Second CPR ``` Response prepending was implemented in !997738 without much comment. There's very little in the way of audit trail as to why we switched. Michael believes that we wanted to make sure that applications got DSR responses immediately. I discussed our options with him, and he suggested that we could implement a priority queue in InputBuffer and make sure that "response" input was dispatched to a client application before any application- or user-generated input. This was deemed to be too much work. We decided that DSR responses getting top billing was likely to be a stronger guarantee than most terminals are capable of giving, and that we should be fine if we just switch it back to append. Fixes #1637. --- src/host/directio.cpp | 35 ------ src/host/directio.h | 4 - src/host/outputStream.cpp | 16 --- src/host/outputStream.hpp | 3 - src/terminal/adapter/adaptDispatch.cpp | 6 +- src/terminal/adapter/conGetSet.hpp | 2 - .../adapter/ut_adapter/adapterTest.cpp | 110 ++++++++++++------ 7 files changed, 77 insertions(+), 99 deletions(-) diff --git a/src/host/directio.cpp b/src/host/directio.cpp index 2ff831c2971..a02153f88e6 100644 --- a/src/host/directio.cpp +++ b/src/host/directio.cpp @@ -534,41 +534,6 @@ void EventsToUnicode(_Inout_ std::deque>& inEvents, CATCH_RETURN(); } -// Function Description: -// - Writes the input records to the beginning of the input buffer. This is used -// by VT sequences that need a response immediately written back to the -// input. -// Arguments: -// - pInputBuffer - the input buffer to write to -// - events - the events to written -// - eventsWritten - on output, the number of events written -// Return Value: -// - HRESULT indicating success or failure -[[nodiscard]] HRESULT DoSrvPrivatePrependConsoleInput(_Inout_ InputBuffer* const pInputBuffer, - _Inout_ std::deque>& events, - _Out_ size_t& eventsWritten) -{ - LockConsole(); - auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); - - eventsWritten = 0; - - try - { - // add partial byte event if necessary - if (pInputBuffer->IsWritePartialByteSequenceAvailable()) - { - events.push_front(pInputBuffer->FetchWritePartialByteSequence(false)); - } - } - CATCH_RETURN(); - - // add to InputBuffer - eventsWritten = pInputBuffer->Prepend(events); - - return S_OK; -} - // Function Description: // - Writes the input KeyEvent to the console as a console control event. This // can be used for potentially generating Ctrl-C events, as diff --git a/src/host/directio.h b/src/host/directio.h index e4b6a29547c..e33d980a89c 100644 --- a/src/host/directio.h +++ b/src/host/directio.h @@ -31,9 +31,5 @@ class SCREEN_INFORMATION; _In_ PCD_CREATE_OBJECT_INFORMATION Information, _In_ PCONSOLE_CREATESCREENBUFFER_MSG a); -[[nodiscard]] NTSTATUS DoSrvPrivatePrependConsoleInput(_Inout_ InputBuffer* const pInputBuffer, - _Inout_ std::deque>& events, - _Out_ size_t& eventsWritten); - [[nodiscard]] NTSTATUS DoSrvPrivateWriteConsoleControlInput(_Inout_ InputBuffer* const pInputBuffer, _In_ KeyEvent key); diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp index 53aed7ad1ee..72bd2e93d13 100644 --- a/src/host/outputStream.cpp +++ b/src/host/outputStream.cpp @@ -540,22 +540,6 @@ bool ConhostInternalGetSet::SetCursorStyle(const CursorType style) return true; } -// Routine Description: -// - Connects the PrivatePrependConsoleInput API call directly into our Driver Message servicing call inside Conhost.exe -// Arguments: -// - events - the input events to be copied into the head of the input -// buffer for the underlying attached process -// - eventsWritten - on output, the number of events written -// Return Value: -// - true if successful (see DoSrvPrivatePrependConsoleInput). false otherwise. -bool ConhostInternalGetSet::PrivatePrependConsoleInput(std::deque>& events, - size_t& eventsWritten) -{ - return SUCCEEDED(DoSrvPrivatePrependConsoleInput(_io.GetActiveInputBuffer(), - events, - eventsWritten)); -} - // Routine Description: // - Connects the PrivatePrependConsoleInput API call directly into our Driver Message servicing call inside Conhost.exe // Arguments: diff --git a/src/host/outputStream.hpp b/src/host/outputStream.hpp index da8838b5053..48ccdfb50be 100644 --- a/src/host/outputStream.hpp +++ b/src/host/outputStream.hpp @@ -104,9 +104,6 @@ class ConhostInternalGetSet final : public Microsoft::Console::VirtualTerminal:: bool PrivateEnableAlternateScroll(const bool enabled) override; bool PrivateEraseAll() override; - bool PrivatePrependConsoleInput(std::deque>& events, - size_t& eventsWritten) override; - bool GetUserDefaultCursorStyle(CursorType& style) override; bool SetCursorStyle(CursorType const style) override; bool SetCursorColor(COLORREF const color) override; diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp index 5dec740da5a..a3e1342151e 100644 --- a/src/terminal/adapter/adaptDispatch.cpp +++ b/src/terminal/adapter/adaptDispatch.cpp @@ -855,7 +855,11 @@ bool AdaptDispatch::_WriteResponse(const std::wstring_view reply) const } size_t eventsWritten; - success = _pConApi->PrivatePrependConsoleInput(inEvents, eventsWritten); + // TODO GH#4954 During the input refactor we may want to add a "priority" input list + // to make sure that "response" input is spooled directly into the application. + // We switched this to an append (vs. a prepend) to fix GH#1637, a bug where two CPR + // could collide with eachother. + success = _pConApi->PrivateWriteConsoleInputW(inEvents, eventsWritten); return success; } diff --git a/src/terminal/adapter/conGetSet.hpp b/src/terminal/adapter/conGetSet.hpp index 8eb97bffcc0..186d88bd79c 100644 --- a/src/terminal/adapter/conGetSet.hpp +++ b/src/terminal/adapter/conGetSet.hpp @@ -73,8 +73,6 @@ namespace Microsoft::Console::VirtualTerminal virtual bool GetUserDefaultCursorStyle(CursorType& style) = 0; virtual bool SetCursorStyle(const CursorType style) = 0; virtual bool SetCursorColor(const COLORREF color) = 0; - virtual bool PrivatePrependConsoleInput(std::deque>& events, - size_t& eventsWritten) = 0; virtual bool PrivateWriteConsoleControlInput(const KeyEvent key) = 0; virtual bool PrivateRefreshWindow() = 0; diff --git a/src/terminal/adapter/ut_adapter/adapterTest.cpp b/src/terminal/adapter/ut_adapter/adapterTest.cpp index 862c035468a..c3693a6e6e6 100644 --- a/src/terminal/adapter/ut_adapter/adapterTest.cpp +++ b/src/terminal/adapter/ut_adapter/adapterTest.cpp @@ -259,32 +259,21 @@ class TestGetSet final : public ConGetSet // move all the input events we were given into local storage so we can test against them Log::Comment(NoThrowString().Format(L"Moving %zu input events into local storage...", events.size())); - _events.clear(); - _events.swap(events); + if (_retainInput) + { + std::move(events.begin(), events.end(), std::back_inserter(_events)); + } + else + { + _events.clear(); + _events.swap(events); + } eventsWritten = _events.size(); } return _privateWriteConsoleInputWResult; } - bool PrivatePrependConsoleInput(std::deque>& events, - size_t& eventsWritten) override - { - Log::Comment(L"PrivatePrependConsoleInput MOCK called..."); - - if (_privatePrependConsoleInputResult) - { - // move all the input events we were given into local storage so we can test against them - Log::Comment(NoThrowString().Format(L"Moving %zu input events into local storage...", events.size())); - - _events.clear(); - _events.swap(events); - eventsWritten = _events.size(); - } - - return _privatePrependConsoleInputResult; - } - bool PrivateWriteConsoleControlInput(_In_ KeyEvent key) override { Log::Comment(L"PrivateWriteConsoleControlInput MOCK called..."); @@ -619,7 +608,6 @@ class TestGetSet final : public ConGetSet _privateGetTextAttributesResult = TRUE; _privateSetTextAttributesResult = TRUE; _privateWriteConsoleInputWResult = TRUE; - _privatePrependConsoleInputResult = TRUE; _privateWriteConsoleControlInputResult = TRUE; _setConsoleWindowInfoResult = TRUE; _moveToBottomResult = true; @@ -645,6 +633,9 @@ class TestGetSet final : public ConGetSet // Attribute default is gray on black. _attribute = TextAttribute{ FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED }; _expectedAttribute = _attribute; + + _events.clear(); + _retainInput = false; } void PrepCursor(CursorX xact, CursorY yact) @@ -746,6 +737,16 @@ class TestGetSet final : public ConGetSet static const WORD s_defaultFill = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED; // dark gray on black. std::deque> _events; + bool _retainInput{ false }; + + auto EnableInputRetentionInScope() + { + auto oldRetainValue{ _retainInput }; + _retainInput = true; + return wil::scope_exit([oldRetainValue, this] { + _retainInput = oldRetainValue; + }); + } COORD _bufferSize = { 0, 0 }; SMALL_RECT _viewport = { 0, 0, 0, 0 }; @@ -775,7 +776,6 @@ class TestGetSet final : public ConGetSet bool _privateGetTextAttributesResult = false; bool _privateSetTextAttributesResult = false; bool _privateWriteConsoleInputWResult = false; - bool _privatePrependConsoleInputResult = false; bool _privateWriteConsoleControlInputResult = false; bool _setConsoleWindowInfoResult = false; @@ -1714,25 +1714,59 @@ class AdapterTest { Log::Comment(L"Starting test..."); - Log::Comment(L"Test 1: Verify normal cursor response position."); - _testGetSet->PrepData(CursorX::XCENTER, CursorY::YCENTER); + { + Log::Comment(L"Test 1: Verify normal cursor response position."); + _testGetSet->PrepData(CursorX::XCENTER, CursorY::YCENTER); + + // start with the cursor position in the buffer. + COORD coordCursorExpected = _testGetSet->_cursorPos; + + // to get to VT, we have to adjust it to its position relative to the viewport top. + coordCursorExpected.Y -= _testGetSet->_viewport.Top; + + // Then note that VT is 1,1 based for the top left, so add 1. (The rest of the console uses 0,0 for array index bases.) + coordCursorExpected.X++; + coordCursorExpected.Y++; - // start with the cursor position in the buffer. - COORD coordCursorExpected = _testGetSet->_cursorPos; + VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport)); - // to get to VT, we have to adjust it to its position relative to the viewport top. - coordCursorExpected.Y -= _testGetSet->_viewport.Top; + wchar_t pwszBuffer[50]; + + swprintf_s(pwszBuffer, ARRAYSIZE(pwszBuffer), L"\x1b[%d;%dR", coordCursorExpected.Y, coordCursorExpected.X); + _testGetSet->ValidateInputEvent(pwszBuffer); + } + + { + Log::Comment(L"Test 2: Verify multiple CPRs with a cursor move between them"); + _testGetSet->PrepData(CursorX::XCENTER, CursorY::YCENTER); - // Then note that VT is 1,1 based for the top left, so add 1. (The rest of the console uses 0,0 for array index bases.) - coordCursorExpected.X++; - coordCursorExpected.Y++; + // enable retention so that the two DSR responses don't delete eachother + auto retentionScope{ _testGetSet->EnableInputRetentionInScope() }; - VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport)); + // start with the cursor position in the buffer. + til::point coordCursorExpectedFirst{ _testGetSet->_cursorPos }; - wchar_t pwszBuffer[50]; + // to get to VT, we have to adjust it to its position relative to the viewport top. + coordCursorExpectedFirst -= til::point{ 0, _testGetSet->_viewport.Top }; - swprintf_s(pwszBuffer, ARRAYSIZE(pwszBuffer), L"\x1b[%d;%dR", coordCursorExpected.Y, coordCursorExpected.X); - _testGetSet->ValidateInputEvent(pwszBuffer); + // Then note that VT is 1,1 based for the top left, so add 1. (The rest of the console uses 0,0 for array index bases.) + coordCursorExpectedFirst += til::point{ 1, 1 }; + + VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport)); + + _testGetSet->_cursorPos.X++; + _testGetSet->_cursorPos.Y++; + + auto coordCursorExpectedSecond{ coordCursorExpectedFirst }; + coordCursorExpectedSecond += til::point{ 1, 1 }; + + VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport)); + + wchar_t pwszBuffer[50]; + + swprintf_s(pwszBuffer, ARRAYSIZE(pwszBuffer), L"\x1b[%d;%dR\x1b[%d;%dR", coordCursorExpectedFirst.y(), coordCursorExpectedFirst.x(), coordCursorExpectedSecond.y(), coordCursorExpectedSecond.x()); + _testGetSet->ValidateInputEvent(pwszBuffer); + } } TEST_METHOD(DeviceAttributesTests) @@ -1748,7 +1782,7 @@ class AdapterTest Log::Comment(L"Test 2: Verify failure when WriteConsoleInput doesn't work."); _testGetSet->PrepData(); - _testGetSet->_privatePrependConsoleInputResult = FALSE; + _testGetSet->_privateWriteConsoleInputWResult = FALSE; VERIFY_IS_FALSE(_pDispatch.get()->DeviceAttributes()); } @@ -1766,7 +1800,7 @@ class AdapterTest Log::Comment(L"Test 2: Verify failure when WriteConsoleInput doesn't work."); _testGetSet->PrepData(); - _testGetSet->_privatePrependConsoleInputResult = FALSE; + _testGetSet->_privateWriteConsoleInputWResult = FALSE; VERIFY_IS_FALSE(_pDispatch.get()->SecondaryDeviceAttributes()); } @@ -1784,7 +1818,7 @@ class AdapterTest Log::Comment(L"Test 2: Verify failure when WriteConsoleInput doesn't work."); _testGetSet->PrepData(); - _testGetSet->_privatePrependConsoleInputResult = FALSE; + _testGetSet->_privateWriteConsoleInputWResult = FALSE; VERIFY_IS_FALSE(_pDispatch.get()->TertiaryDeviceAttributes()); } From 986f20e031d8a50321b1a640fe55c81b7e0b61cf Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Wed, 9 Sep 2020 13:33:56 -0700 Subject: [PATCH 2/2] weave a spell --- .github/actions/spell-check/dictionary/microsoft.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spell-check/dictionary/microsoft.txt b/.github/actions/spell-check/dictionary/microsoft.txt index 2f520a5c2f3..a5d56bf5bc8 100644 --- a/.github/actions/spell-check/dictionary/microsoft.txt +++ b/.github/actions/spell-check/dictionary/microsoft.txt @@ -2,6 +2,7 @@ ACLs altform appendwttlogging backplating +CPRs DACL DACLs dotnetfeed