Skip to content

Commit

Permalink
vt: make sure to Flush the entire parsed sequence when unrecognized
Browse files Browse the repository at this point in the history
When we had to flush unknown sequences to the terminal, we were only
taking the _most recent run_ with us; therefore, if we received `\e[?12`
and `34h` in separate packets we would _only_ send out `34h`.

This change fixes that issue by ensuring that we cache partial bits of
sequences we haven't yet completed, just in case we need to flush them.

Fixes #3080.
Fixes #3081.
  • Loading branch information
DHowett committed Mar 10, 2020
1 parent d954ad6 commit e50f605
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 4 deletions.
17 changes: 16 additions & 1 deletion src/terminal/parser/stateMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ StateMachine::StateMachine(std::unique_ptr<IStateMachineEngine> engine) :
_intermediates{},
_parameters{},
_oscString{},
_cachedSequence{ std::nullopt },
_processingIndividually(false)
{
_ActionClear();
Expand Down Expand Up @@ -533,6 +534,7 @@ void StateMachine::_ActionSs3Dispatch(const wchar_t wch)
void StateMachine::_EnterGround() noexcept
{
_state = VTStates::Ground;
_cachedSequence.reset(); // entering ground means we've completed the pending sequence
_trace.TraceStateChange(L"Ground");
}

Expand Down Expand Up @@ -1230,7 +1232,13 @@ bool StateMachine::FlushToTerminal()
// that pwchCurr was processed.
// However, if we're here, then the processing of pwchChar triggered the
// engine to request the entire sequence get passed through, including pwchCurr.
return _engine->ActionPassThroughString(_run);
bool succeeded = true;
if (succeeded && _cachedSequence.has_value())
{
succeeded = _engine->ActionPassThroughString(*_cachedSequence);
_cachedSequence.reset();
}
return succeeded && _engine->ActionPassThroughString(_run);
}

// Routine Description:
Expand Down Expand Up @@ -1365,6 +1373,13 @@ void StateMachine::ProcessString(const std::wstring_view string)
// after dispatching the characters
_EnterGround();
}
else
{
// If the engine doesn't require flushing at the end of the string, we
// want to cache the partial sequence in case we have to flush the whole
// thing to the terminal later.
_cachedSequence = _cachedSequence.value_or(std::wstring{}) + std::wstring{ _run };
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/terminal/parser/stateMachine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ namespace Microsoft::Console::VirtualTerminal
std::wstring _oscString;
size_t _oscParameter;

std::optional<std::wstring> _cachedSequence;

// This is tracked per state machine instance so that separate calls to Process*
// can start and finish a sequence.
bool _processingIndividually;
Expand Down
68 changes: 65 additions & 3 deletions src/terminal/parser/ut_parser/StateMachineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,25 @@ using namespace Microsoft::Console::VirtualTerminal;
class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStateMachineEngine
{
public:
void ResetTestState()
{
printed.clear();
passedThrough.clear();
csiParams.reset();
}

bool ActionExecute(const wchar_t /* wch */) override { return true; };
bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; };
bool ActionPrint(const wchar_t /* wch */) override { return true; };
bool ActionPrintString(const std::wstring_view string) override
{
printed = string;
printed += string;
return true;
};

bool ActionPassThroughString(const std::wstring_view string) override
{
passedThrough = string;
passedThrough += string;
return true;
};

Expand All @@ -52,7 +59,15 @@ class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStat

bool ActionOscDispatch(const wchar_t /* wch */,
const size_t /* parameter */,
const std::wstring_view /* string */) override { return true; };
const std::wstring_view /* string */) override
{
if (pfnFlushToTerminal)
{
pfnFlushToTerminal();
return true;
}
return true;
};

bool ActionSs3Dispatch(const wchar_t /* wch */,
const std::basic_string_view<size_t> /* parameters */) override { return true; };
Expand Down Expand Up @@ -111,6 +126,7 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest
TEST_METHOD(PassThroughUnhandled);
TEST_METHOD(RunStorageBeforeEscape);
TEST_METHOD(BulkTextPrint);
TEST_METHOD(PassThroughUnhandledSplitAcrossWrites);
};

void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother()
Expand Down Expand Up @@ -182,3 +198,49 @@ void StateMachineTest::BulkTextPrint()
// Then ensure the entire buffered run was printed all at once back to us.
VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str()));
}

void StateMachineTest::PassThroughUnhandledSplitAcrossWrites()
{
auto enginePtr{ std::make_unique<TestStateMachineEngine>() };
// this dance is required because StateMachine presumes to take ownership of its engine.
auto& engine{ *enginePtr.get() };
StateMachine machine{ std::move(enginePtr) };

// Hook up the passthrough function.
engine.pfnFlushToTerminal = std::bind(&StateMachine::FlushToTerminal, &machine);

// Broken in two pieces (test case from GH#3081)
machine.ProcessString(L"\x1b[?12");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"34h");
VERIFY_ARE_EQUAL(L"\x1b[?1234h", engine.passedThrough); // whole sequence out, no other output
VERIFY_ARE_EQUAL(L"", engine.printed);

engine.ResetTestState();

// Three pieces
machine.ProcessString(L"\x1b[?2");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"34");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"5h");
VERIFY_ARE_EQUAL(L"\x1b[?2345h", engine.passedThrough); // whole sequence out, no other output
VERIFY_ARE_EQUAL(L"", engine.printed);

engine.ResetTestState();

// Split during OSC terminator (test case from GH#3080)
machine.ProcessString(L"\x1b]99;foo\x1b");
VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
VERIFY_ARE_EQUAL(L"", engine.printed);

machine.ProcessString(L"\\");
VERIFY_ARE_EQUAL(L"\x1b]99;foo\x1b\\", engine.passedThrough);
VERIFY_ARE_EQUAL(L"", engine.printed);
}

0 comments on commit e50f605

Please sign in to comment.