Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter out control characters that don't do anything #15075

Merged
merged 4 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/expect/expect.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ ansicpg
ANSISYS
ANSISYSRC
ANSISYSSC
answerback
antialiasing
ANull
anycpu
Expand Down
37 changes: 23 additions & 14 deletions src/host/ut_host/ScreenBufferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ class ScreenBufferTests

TEST_METHOD(EraseAllTests);

TEST_METHOD(OutputNULTest);
TEST_METHOD(InactiveControlCharactersTest);

TEST_METHOD(VtResize);
TEST_METHOD(VtResizeComprehensive);
Expand Down Expand Up @@ -1014,8 +1014,19 @@ void ScreenBufferTests::EraseAllTests()
viewport.BottomInclusive()));
}

void ScreenBufferTests::OutputNULTest()
void ScreenBufferTests::InactiveControlCharactersTest()
{
// These are the control characters that don't write anything to the
// output buffer, and are expected not to move the cursor position.

BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:ordinal", L"{0, 1, 2, 3, 4, 5, 6, 7, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 28, 29, 30, 31}")
END_TEST_METHOD_PROPERTIES()

unsigned ordinal;
VERIFY_SUCCEEDED(TestData::TryGetValue(L"ordinal", ordinal));
const auto ch = static_cast<wchar_t>(ordinal);

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& stateMachine = si.GetStateMachine();
Expand All @@ -1024,31 +1035,29 @@ void ScreenBufferTests::OutputNULTest()
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing a single NUL"));
stateMachine.ProcessString({ L"\0", 1 });
Log::Comment(L"Writing a single control character");
const auto singleChar = std::wstring(1, ch);
stateMachine.ProcessString(singleChar);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing many NULs"));
stateMachine.ProcessString({ L"\0\0\0\0\0\0\0\0", 8 });
Log::Comment(L"Writing many control characters");
const auto manyChars = std::wstring(8, ch);
stateMachine.ProcessString(manyChars);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Testing a single NUL followed by real text"));
stateMachine.ProcessString({ L"\0foo", 4 });
Log::Comment(L"Testing a single control character followed by real text");
stateMachine.ProcessString(singleChar + L"foo");
VERIFY_ARE_EQUAL(3, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(0, cursor.GetPosition().y);

stateMachine.ProcessString(L"\n");
VERIFY_ARE_EQUAL(0, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);

Log::Comment(NoThrowString().Format(
L"Writing NULs in between other strings"));
stateMachine.ProcessString({ L"\0foo\0bar\0", 9 });
Log::Comment(L"Writing controls in between other strings");
stateMachine.ProcessString(singleChar + L"foo" + singleChar + L"bar" + singleChar);
VERIFY_ARE_EQUAL(6, cursor.GetPosition().x);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);
}
Expand Down
25 changes: 20 additions & 5 deletions src/terminal/parser/OutputStateMachineEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
{
switch (wch)
{
case AsciiChars::NUL:
// microsoft/terminal#1825 - VT applications expect to be able to write NUL
// and have _nothing_ happen. Filter the NULs here, so they don't fill the
// buffer with empty spaces.
case AsciiChars::ENQ:
// GH#11946: At some point we may want to add support for the VT
// answerback feature, which requires responding to an ENQ control
// with a user-defined reply, but until then we just ignore it.
break;
case AsciiChars::BEL:
_dispatch->WarningBell();
Expand Down Expand Up @@ -79,9 +79,24 @@ bool OutputStateMachineEngine::ActionExecute(const wchar_t wch)
case AsciiChars::SO:
_dispatch->LockingShift(1);
break;
default:
case AsciiChars::SUB:
// The SUB control is used to cancel a control sequence in the same
// way as CAN, but unlike CAN it also displays an error character,
// typically a reverse question mark.
_dispatch->Print(L'\u2E2E');
break;
case AsciiChars::DEL:
// The DEL control can sometimes be translated into a printable glyph
// if a 96-character set is designated, so we need to pass it through
// to the Print method. If not translated, it will be filtered out
// there.
_dispatch->Print(wch);
break;
default:
// GH#1825, GH#10786: VT applications expect to be able to write other
// control characters and have _nothing_ happen. We filter out these
// characters here, so they don't fill the buffer.
break;
}

_ClearLastChar();
Expand Down