maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Switch all DSR responses to appending instead of prepending (#7583)

Browse source 
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
end up reading 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
```

The end result of this corruption is that a requesting application
would receive an unbidden `R` on stdin; for vim, this would trigger
replace mode immediately on startup.

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. It had the unfortunate side effect of causing
subsequence CPRs across cursor moves to come out in the wrong order.

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.

Thanks to @k-takata, @tekki and @brammool for the investigation on the
vim side.

Fixes #1637.
This commit is contained in:
Dustin L. Howett 2020-09-09 16:55:22 -07:00 committed by GitHub
parent 27f7ce7c6e
commit cb037f3953
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 78 additions and 99 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
.github/actions/spell-check/dictionary/microsoft.txt vendored
View file

@ -2,6 +2,7 @@ ACLs
altform
appendwttlogging
backplating
CPRs
DACL
DACLs
dotnetfeed

35
src/host/directio.cpp
View file

@ -534,41 +534,6 @@ void EventsToUnicode(_Inout_ std::deque<std::unique_ptr<IInputEvent>>& 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<std::unique_ptr<IInputEvent>>& 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

4
src/host/directio.h
View file

@ -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<std::unique_ptr<IInputEvent>>& events,
_Out_ size_t& eventsWritten);
[[nodiscard]] NTSTATUS DoSrvPrivateWriteConsoleControlInput(_Inout_ InputBuffer* const pInputBuffer,
_In_ KeyEvent key);

16
src/host/outputStream.cpp
View file

@ -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<std::unique_ptr<IInputEvent>>& 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:

3
src/host/outputStream.hpp
View file

@ -104,9 +104,6 @@ public:
bool PrivateEnableAlternateScroll(const bool enabled) override;
bool PrivateEraseAll() override;
bool PrivatePrependConsoleInput(std::deque<std::unique_ptr<IInputEvent>>& events,
size_t& eventsWritten) override;
bool GetUserDefaultCursorStyle(CursorType& style) override;
bool SetCursorStyle(CursorType const style) override;
bool SetCursorColor(COLORREF const color) override;

6
src/terminal/adapter/adaptDispatch.cpp
View file

@ -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;
}

2
src/terminal/adapter/conGetSet.hpp
View file

@ -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<std::unique_ptr<IInputEvent>>& events,
size_t& eventsWritten) = 0;
virtual bool PrivateWriteConsoleControlInput(const KeyEvent key) = 0;
virtual bool PrivateRefreshWindow() = 0;

110
src/terminal/adapter/ut_adapter/adapterTest.cpp
View file

@ -259,32 +259,21 @@ public:
// 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<std::unique_ptr<IInputEvent>>& 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 @@ public:
_privateGetTextAttributesResult = TRUE;
_privateSetTextAttributesResult = TRUE;
_privateWriteConsoleInputWResult = TRUE;
_privatePrependConsoleInputResult = TRUE;
_privateWriteConsoleControlInputResult = TRUE;
_setConsoleWindowInfoResult = TRUE;
_moveToBottomResult = true;
@ -645,6 +633,9 @@ public:
// 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 @@ public:
static const WORD s_defaultFill = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED; // dark gray on black.
std::deque<std::unique_ptr<IInputEvent>> _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 @@ public:
bool _privateGetTextAttributesResult = false;
bool _privateSetTextAttributesResult = false;
bool _privateWriteConsoleInputWResult = false;
bool _privatePrependConsoleInputResult = false;
bool _privateWriteConsoleControlInputResult = false;
bool _setConsoleWindowInfoResult = false;
@ -1714,25 +1714,59 @@ public:
{
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;
// 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;
// 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++;
// 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++;
VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport));
VERIFY_IS_TRUE(_pDispatch.get()->DeviceStatusReport(DispatchTypes::AnsiStatusType::CPR_CursorPositionReport));
wchar_t pwszBuffer[50];
wchar_t pwszBuffer[50];
swprintf_s(pwszBuffer, ARRAYSIZE(pwszBuffer), L"\x1b[%d;%dR", coordCursorExpected.Y, coordCursorExpected.X);
_testGetSet->ValidateInputEvent(pwszBuffer);
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);
// enable retention so that the two DSR responses don't delete eachother
auto retentionScope{ _testGetSet->EnableInputRetentionInScope() };
// start with the cursor position in the buffer.
til::point coordCursorExpectedFirst{ _testGetSet->_cursorPos };
// to get to VT, we have to adjust it to its position relative to the viewport top.
coordCursorExpectedFirst -= til::point{ 0, _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.)
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<int>(), coordCursorExpectedFirst.x<int>(), coordCursorExpectedSecond.y<int>(), coordCursorExpectedSecond.x<int>());
_testGetSet->ValidateInputEvent(pwszBuffer);
}
}
TEST_METHOD(DeviceAttributesTests)
@ -1748,7 +1782,7 @@ public:
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 @@ public:
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 @@ public:
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());
}