From 91493b50785cd8f99dfb6c588dca532f9b0b03be Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 3 Jul 2023 19:59:05 +0530 Subject: [PATCH 01/32] add support for colon seperated sub-parameters --- src/terminal/adapter/DispatchTypes.hpp | 53 ++++++-- src/terminal/parser/stateMachine.cpp | 173 +++++++++++++++++++++---- src/terminal/parser/stateMachine.hpp | 8 ++ 3 files changed, 199 insertions(+), 35 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 9e342ec5f80..6a97d17efc6 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -159,49 +159,76 @@ namespace Microsoft::Console::VirtualTerminal { } - constexpr VTParameters(const VTParameter* ptr, const size_t count) noexcept : - _values{ ptr, count } + constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount) noexcept : + _params{ paramsPtr, paramsCount }, + _subParams{}, + _subParamsRange{} + { + } + + constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount, const VTParameter* subParamsPtr, const size_t subParamsCount, const std::pair* subParamsRangePtr, const size_t subParamsRangeCount) noexcept : + _params{ paramsPtr, paramsCount }, + _subParams{ subParamsPtr, subParamsCount }, + _subParamsRange{ subParamsRangePtr, subParamsRangeCount } { } constexpr VTParameter at(const size_t index) const noexcept { // If the index is out of range, we return a parameter with no value. - return index < _values.size() ? til::at(_values, index) : defaultParameter; + return index < _params.size() ? til::at(_params, index) : defaultParameter; } constexpr bool empty() const noexcept { - return _values.empty(); + return _params.empty(); } constexpr size_t size() const noexcept { // We always return a size of at least 1, since an empty parameter // list is the equivalent of a single "default" parameter. - return std::max(_values.size(), 1); + return std::max(_params.size(), 1); } VTParameters subspan(const size_t offset) const noexcept { - const auto subValues = _values.subspan(std::min(offset, _values.size())); - return { subValues.data(), subValues.size() }; + // We need sub parameters to always be in their original index + // because we store their indexes in subParamsRange. So we pass + // _subParams as is and create new span for others. + const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); + const auto newSubParamsRangeSpan = _subParamsRange.subspan(std::min(offset, _params.size())); + return { newParamsSpan.data(), newParamsSpan.size(), _subParams.data(), _subParams.size(), newSubParamsRangeSpan.data(), newSubParamsRangeSpan.size() }; + } + + std::span subParamsFor(const size_t index) const noexcept + { + // If the parameter index is out of range, we return a sub-parameters span with no values. + if (index >= _subParamsRange.size()) + { + return std::span{}; + } + else + { + const auto& range = til::at(_subParamsRange, index); + return _subParams.subspan(range.first, range.second - range.first); + } } template bool for_each(const T&& predicate) const { - auto values = _values; + auto params = _params; // We always return at least 1 value here, since an empty parameter // list is the equivalent of a single "default" parameter. - if (values.empty()) + if (params.empty()) { - values = defaultParameters; + params = defaultParameters; } auto success = true; - for (const auto& v : values) + for (const auto& v : params) { success = predicate(v) && success; } @@ -212,7 +239,9 @@ namespace Microsoft::Console::VirtualTerminal static constexpr VTParameter defaultParameter; static constexpr std::span defaultParameters{ &defaultParameter, 1 }; - std::span _values; + std::span _params; + std::span _subParams; + std::span> _subParamsRange; }; // FlaggedEnumValue is a convenience class that produces enum values (of a specified size) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index ea455f458d2..6f8a21793c6 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -158,38 +158,38 @@ static constexpr bool _isParameterDelimiter(const wchar_t wch) noexcept } // Routine Description: -// - Determines if a character is "control sequence" beginning indicator. -// This immediately follows an escape and signifies a varying length control sequence. +// - Determines if a character is a delimiter between a parameter and its sub-parameters in an escape sequence. // Arguments: // - wch - Character to check. // Return Value: // - True if it is. False if it isn't. -static constexpr bool _isCsiIndicator(const wchar_t wch) noexcept +static constexpr bool _isSubParameterDelimiter(const wchar_t wch) noexcept { - return wch == L'['; // 0x5B + return wch == L':'; // 0x3A } // Routine Description: -// - Determines if a character is a private range marker for a control sequence. -// Private range markers indicate vendor-specific behavior. +// - Determines if a character is "control sequence" beginning indicator. +// This immediately follows an escape and signifies a varying length control sequence. // Arguments: // - wch - Character to check. // Return Value: // - True if it is. False if it isn't. -static constexpr bool _isCsiPrivateMarker(const wchar_t wch) noexcept +static constexpr bool _isCsiIndicator(const wchar_t wch) noexcept { - return wch == L'<' || wch == L'=' || wch == L'>' || wch == L'?'; // 0x3C - 0x3F + return wch == L'['; // 0x5B } // Routine Description: -// - Determines if a character is invalid in a control sequence +// - Determines if a character is a private range marker for a control sequence. +// Private range markers indicate vendor-specific behavior. // Arguments: // - wch - Character to check. // Return Value: // - True if it is. False if it isn't. -static constexpr bool _isCsiInvalid(const wchar_t wch) noexcept +static constexpr bool _isCsiPrivateMarker(const wchar_t wch) noexcept { - return wch == L':'; // 0x3A + return wch == L'<' || wch == L'=' || wch == L'>' || wch == L'?'; // 0x3C - 0x3F } // Routine Description: @@ -201,7 +201,7 @@ static constexpr bool _isCsiInvalid(const wchar_t wch) noexcept static constexpr bool _isIntermediateInvalid(const wchar_t wch) noexcept { // 0x30 - 0x3F - return _isNumericParamValue(wch) || _isCsiInvalid(wch) || _isParameterDelimiter(wch) || _isCsiPrivateMarker(wch); + return _isNumericParamValue(wch) || _isSubParameterDelimiter(wch) || _isParameterDelimiter(wch) || _isCsiPrivateMarker(wch); } // Routine Description: @@ -212,8 +212,8 @@ static constexpr bool _isIntermediateInvalid(const wchar_t wch) noexcept // - True if it is. False if it isn't. static constexpr bool _isParameterInvalid(const wchar_t wch) noexcept { - // 0x3A, 0x3C - 0x3F - return _isCsiInvalid(wch) || _isCsiPrivateMarker(wch); + // 0x3C - 0x3F + return _isCsiPrivateMarker(wch); } // Routine Description: @@ -456,7 +456,7 @@ void StateMachine::_ActionVt52EscDispatch(const wchar_t wch) // Routine Description: // - Triggers the CsiDispatch action to indicate that the listener should handle a control sequence. -// These sequences perform various API-type commands that can include many parameters. +// These sequences perform various API-type commands that can include many parameters and sub parameters. // Arguments: // - wch - Character to dispatch. // Return Value: @@ -465,7 +465,8 @@ void StateMachine::_ActionCsiDispatch(const wchar_t wch) { _trace.TraceOnAction(L"CsiDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { - return _engine->ActionCsiDispatch(_identifier.Finalize(wch), { _parameters.data(), _parameters.size() }); + return _engine->ActionCsiDispatch(_identifier.Finalize(wch), + { _parameters.data(), _parameters.size(), _subParameters.data(), _subParameters.size(), _subParametersRange.data(), _subParametersRange.size() }); })); } @@ -501,6 +502,8 @@ void StateMachine::_ActionParam(const wchar_t wch) if (_parameters.empty()) { _parameters.push_back({}); + auto rangeStart = static_cast(_subParameters.size()); + _subParametersRange.push_back({ rangeStart, rangeStart }); } // On a delimiter, increase the number of params we've seen. @@ -519,6 +522,8 @@ void StateMachine::_ActionParam(const wchar_t wch) { // Otherwise move to next param. _parameters.push_back({}); + auto rangeStart = static_cast(_subParameters.size()); + _subParametersRange.push_back({ rangeStart, rangeStart }); } } else @@ -532,6 +537,51 @@ void StateMachine::_ActionParam(const wchar_t wch) } } +// Routine Description: +// - Triggers the SubParam action to indicate that the state machine should store this character as a part of a sub-parameter +// to a control sequence. +// Arguments: +// - wch - Character to dispatch. +// Return Value: +// - +void StateMachine::_ActionSubParam(const wchar_t wch) +{ + _trace.TraceOnAction(L"SubParam"); + + // Once we've reached the sub-parameter limit, additional sub-parameters are ignored. + if (!_subParameterLimitReached) + { + // On a delimiter, increase the number of sub-params we've seen. + // "Empty" sub-params should still count as a sub-param - + // eg "\x1b[38:2::m" should be three sub-params + if (_isSubParameterDelimiter(wch)) + { + // If we receive a delimiter after we've already accumulated the + // maximum allowed sub-parameters, then we need to set a flag to + // indicate that further sub-parameter characters should be ignored. + if (_subParameters.size() >= MAX_SUBPARAMETER_COUNT) + { + _subParameterLimitReached = true; + } + else + { + // Otherwise move to next sub-param. + _subParameters.push_back({}); + // increment current range's end index. + _subParametersRange.back().second++; + } + } + else + { + // Accumulate the character given into the last (current) sub-parameter. + // If the value hasn't been initialized yet, it'll start as 0. + auto currentParameter = _subParameters.back().value_or(0); + _AccumulateTo(wch, currentParameter); + _subParameters.back() = currentParameter; + } + } +} + // Routine Description: // - Triggers the Clear action to indicate that the state machine should erase all internal state. // Arguments: @@ -548,6 +598,10 @@ void StateMachine::_ActionClear() _parameters.clear(); _parameterLimitReached = false; + _subParameters.clear(); + _subParametersRange.clear(); + _subParameterLimitReached = false; + _oscString.clear(); _oscParameter = 0; @@ -749,6 +803,20 @@ void StateMachine::_EnterCsiParam() noexcept _trace.TraceStateChange(L"CsiParam"); } +// Routine Description: +// - Moves the state machine into the CsiSubParam state. +// This state is entered: +// 1. When valid sub parameter characters are detected in CsiParam state. +// Arguments: +// - +// Return Value: +// - +void StateMachine::_EnterCsiSubParam() noexcept +{ + _state = VTStates::CsiSubParam; + _trace.TraceStateChange(L"CsiSubParam"); +} + // Routine Description: // - Moves the state machine into the CsiIgnore state. // This state is entered: @@ -1120,7 +1188,7 @@ void StateMachine::_EventEscapeIntermediate(const wchar_t wch) // 1. Execute C0 control characters // 2. Ignore Delete characters // 3. Collect Intermediate characters -// 4. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) +// 4. Begin to ignore all remaining parameters when a sub parameter character is detected (CsiIgnore) // 5. Store parameter data // 6. Collect Control Sequence Private markers // 7. Dispatch a control sequence with parameters for action @@ -1144,7 +1212,7 @@ void StateMachine::_EventCsiEntry(const wchar_t wch) _ActionCollect(wch); _EnterCsiIntermediate(); } - else if (_isCsiInvalid(wch)) + else if (_isSubParameterDelimiter(wch)) { _EnterCsiIgnore(); } @@ -1250,7 +1318,8 @@ void StateMachine::_EventCsiIgnore(const wchar_t wch) // 3. Collect Intermediate characters // 4. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) // 5. Store parameter data -// 6. Dispatch a control sequence with parameters for action +// 6. Store sub parameter data +// 7. Dispatch a control sequence with parameters for action // Arguments: // - wch - Character that triggered the event // Return Value: @@ -1270,6 +1339,62 @@ void StateMachine::_EventCsiParam(const wchar_t wch) { _ActionParam(wch); } + else if (_isSubParameterDelimiter(wch)) + { + _ActionSubParam(wch); + _EnterCsiSubParam(); + } + else if (_isIntermediate(wch)) + { + _ActionCollect(wch); + _EnterCsiIntermediate(); + } + else if (_isParameterInvalid(wch)) + { + _EnterCsiIgnore(); + } + else + { + _ActionCsiDispatch(wch); + _EnterGround(); + _ExecuteCsiCompleteCallback(); + } +} + +// Routine Description: +// - Processes a character event into an Action that occurs while in the CsiSubParam state. +// Events in this state will: +// 1. Execute C0 control characters +// 2. Ignore Delete characters +// 3. Store sub parameter data +// 4. Store parameter data +// 5. Collect Intermediate characters for parameter. +// 6. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) +// 7. Dispatch a control sequence with parameters for action +// Arguments: +// - wch - Character that triggered the event +// Return Value: +// - +void StateMachine::_EventCsiSubParam(const wchar_t wch) +{ + _trace.TraceOnEvent(L"CsiSubParam"); + if (_isC0Code(wch)) + { + _ActionExecute(wch); + } + else if (_isDelete(wch)) + { + _ActionIgnore(); + } + else if (_isNumericParamValue(wch) || _isSubParameterDelimiter(wch)) + { + _ActionSubParam(wch); + } + else if (_isParameterDelimiter(wch)) + { + _ActionParam(wch); + _EnterCsiParam(); + } else if (_isIntermediate(wch)) { _ActionCollect(wch); @@ -1403,7 +1528,7 @@ void StateMachine::_EventSs3Entry(const wchar_t wch) { _ActionIgnore(); } - else if (_isCsiInvalid(wch)) + else if (_isSubParameterDelimiter(wch)) { // It's safe for us to go into the CSI ignore here, because both SS3 and // CSI sequences ignore characters the same way. @@ -1448,7 +1573,7 @@ void StateMachine::_EventSs3Param(const wchar_t wch) { _ActionParam(wch); } - else if (_isParameterInvalid(wch)) + else if (_isParameterInvalid(wch) || _isSubParameterDelimiter(wch)) { _EnterCsiIgnore(); } @@ -1521,7 +1646,7 @@ void StateMachine::_EventDcsEntry(const wchar_t wch) { _ActionIgnore(); } - else if (_isCsiInvalid(wch)) + else if (_isSubParameterDelimiter(wch)) { _EnterDcsIgnore(); } @@ -1625,7 +1750,7 @@ void StateMachine::_EventDcsParam(const wchar_t wch) _ActionCollect(wch); _EnterDcsIntermediate(); } - else if (_isParameterInvalid(wch)) + else if (_isParameterInvalid(wch) || _isSubParameterDelimiter(wch)) { _EnterDcsIgnore(); } @@ -1737,6 +1862,8 @@ void StateMachine::ProcessCharacter(const wchar_t wch) return _EventCsiIgnore(wch); case VTStates::CsiParam: return _EventCsiParam(wch); + case VTStates::CsiSubParam: + return _EventCsiSubParam(wch); case VTStates::OscParam: return _EventOscParam(wch); case VTStates::OscString: diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 4023299c0c6..d478b79ff84 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -30,6 +30,7 @@ namespace Microsoft::Console::VirtualTerminal // are supported, but most modern terminal emulators will allow around twice // that number. constexpr size_t MAX_PARAMETER_COUNT = 32; + constexpr size_t MAX_SUBPARAMETER_COUNT = 32; class StateMachine final { @@ -85,6 +86,7 @@ namespace Microsoft::Console::VirtualTerminal void _ActionVt52EscDispatch(const wchar_t wch); void _ActionCollect(const wchar_t wch) noexcept; void _ActionParam(const wchar_t wch); + void _ActionSubParam(const wchar_t wch); void _ActionCsiDispatch(const wchar_t wch); void _ActionOscParam(const wchar_t wch) noexcept; void _ActionOscPut(const wchar_t wch); @@ -101,6 +103,7 @@ namespace Microsoft::Console::VirtualTerminal void _EnterEscapeIntermediate() noexcept; void _EnterCsiEntry(); void _EnterCsiParam() noexcept; + void _EnterCsiSubParam() noexcept; void _EnterCsiIgnore() noexcept; void _EnterCsiIntermediate() noexcept; void _EnterOscParam() noexcept; @@ -123,6 +126,7 @@ namespace Microsoft::Console::VirtualTerminal void _EventCsiIntermediate(const wchar_t wch); void _EventCsiIgnore(const wchar_t wch); void _EventCsiParam(const wchar_t wch); + void _EventCsiSubParam(const wchar_t wch); void _EventOscParam(const wchar_t wch) noexcept; void _EventOscString(const wchar_t wch); void _EventOscTermination(const wchar_t wch); @@ -152,6 +156,7 @@ namespace Microsoft::Console::VirtualTerminal CsiIntermediate, CsiIgnore, CsiParam, + CsiSubParam, OscParam, OscString, OscTermination, @@ -191,6 +196,9 @@ namespace Microsoft::Console::VirtualTerminal VTIDBuilder _identifier; std::vector _parameters; bool _parameterLimitReached; + std::vector _subParameters; + std::vector> _subParametersRange; + bool _subParameterLimitReached; std::wstring _oscString; VTInt _oscParameter; From 1ed4ba2581ff5d6639b63a20c142a0fa7673eeb6 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 3 Jul 2023 22:15:46 +0530 Subject: [PATCH 02/32] use gsl::narrow instead of static_cast --- src/terminal/parser/stateMachine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 6f8a21793c6..643f72d879f 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -502,7 +502,7 @@ void StateMachine::_ActionParam(const wchar_t wch) if (_parameters.empty()) { _parameters.push_back({}); - auto rangeStart = static_cast(_subParameters.size()); + auto rangeStart = gsl::narrow(_subParameters.size()); _subParametersRange.push_back({ rangeStart, rangeStart }); } @@ -522,7 +522,7 @@ void StateMachine::_ActionParam(const wchar_t wch) { // Otherwise move to next param. _parameters.push_back({}); - auto rangeStart = static_cast(_subParameters.size()); + auto rangeStart = gsl::narrow(_subParameters.size()); _subParametersRange.push_back({ rangeStart, rangeStart }); } } From 84735d95908bcf3963c0dc489649a8b514d05a04 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 4 Jul 2023 13:36:02 +0530 Subject: [PATCH 03/32] add tests --- .../parser/ut_parser/OutputEngineTest.cpp | 118 +++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index db3a6f14588..0b1cf191184 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -391,6 +391,122 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); } + TEST_METHOD(TestCsiSubParam) + { + auto dispatch = std::make_unique(); + auto engine = std::make_unique(std::move(dispatch)); + StateMachine mach(std::move(engine)); + + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + mach.ProcessCharacter(AsciiChars::ESC); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L';'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'5'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'8'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'J'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + + VERIFY_ARE_EQUAL(mach._parameters.size(), 2u); + VERIFY_IS_FALSE(mach._parameters.at(0).has_value()); + VERIFY_ARE_EQUAL(mach._parameters.at(1), 3); + + VERIFY_ARE_EQUAL(mach._subParameters.at(0), 35); + VERIFY_IS_FALSE(mach._subParameters.at(1).has_value()); + VERIFY_ARE_EQUAL(mach._subParameters.at(2), 8); + + VERIFY_ARE_EQUAL(mach._subParametersRange.at(0).first, 0); + VERIFY_ARE_EQUAL(mach._subParametersRange.at(0).second, 0); + VERIFY_ARE_EQUAL(mach._subParametersRange.at(1).first, 0); + VERIFY_ARE_EQUAL(mach._subParametersRange.at(1).second, 3); + + VERIFY_ARE_EQUAL(mach._subParametersRange.size(), mach._parameters.size()); + VERIFY_IS_TRUE( + (mach._subParametersRange.back().second == mach._subParameters.size() - 1) // lastIndex + || ( mach._subParametersRange.back().second == mach._subParameters.size()) // or lastIndex + 1 + ); + } + + TEST_METHOD(TestCsiMaxSubParamCount) + { + auto dispatch = std::make_unique(); + auto engine = std::make_unique(std::move(dispatch)); + StateMachine mach(std::move(engine)); + + Log::Comment(L"Output a sequence with 100 sub parameters"); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + mach.ProcessCharacter(AsciiChars::ESC); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + for (size_t i = 0; i < 100; i++) + { + if (i > 0) + { + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + mach.ProcessCharacter(L'0' + i % 10); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + mach.ProcessCharacter(L'J'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + + Log::Comment(L"Only MAX_SUBPARAMETER_COUNT (32) sub parameters should be stored"); + VERIFY_ARE_EQUAL(mach._subParameters.size(), MAX_SUBPARAMETER_COUNT); + for (size_t i = 0; i < MAX_SUBPARAMETER_COUNT; i++) + { + VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); + VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 10)); + } + } + + TEST_METHOD(TestLeadingZeroCsiSubParam) + { + auto dispatch = std::make_unique(); + auto engine = std::make_unique(std::move(dispatch)); + StateMachine mach(std::move(engine)); + + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + mach.ProcessCharacter(AsciiChars::ESC); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + for (auto i = 0; i < 50; i++) // Any number of leading zeros should be supported + { + mach.ProcessCharacter(L'0'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + for (auto i = 0; i < 5; i++) // We're only expecting to be able to keep 5 digits max + { + mach.ProcessCharacter((wchar_t)(L'1' + i)); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + VERIFY_ARE_EQUAL(mach._subParameters.back(), 12345); + mach.ProcessCharacter(L'J'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + } + TEST_METHOD(TestCsiIgnore) { auto dispatch = std::make_unique(); @@ -417,7 +533,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); mach.ProcessCharacter(L';'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); - mach.ProcessCharacter(L':'); + mach.ProcessCharacter(L'='); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiIgnore); mach.ProcessCharacter(L'8'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiIgnore); From b95842752a4f16781fc9e08b72554801cda8e480 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 4 Jul 2023 13:40:49 +0530 Subject: [PATCH 04/32] runformat --- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 0b1cf191184..829f7bc9363 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -437,8 +437,8 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._subParametersRange.size(), mach._parameters.size()); VERIFY_IS_TRUE( (mach._subParametersRange.back().second == mach._subParameters.size() - 1) // lastIndex - || ( mach._subParametersRange.back().second == mach._subParameters.size()) // or lastIndex + 1 - ); + || (mach._subParametersRange.back().second == mach._subParameters.size()) // or lastIndex + 1 + ); } TEST_METHOD(TestCsiMaxSubParamCount) From 470e1637e36b89c3a499b04a727ad3e4a4a9b417 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 4 Jul 2023 18:56:15 +0530 Subject: [PATCH 05/32] first sub param is also followed by colon(:) --- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 829f7bc9363..379b00d9969 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -457,11 +457,8 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); for (size_t i = 0; i < 100; i++) { - if (i > 0) - { - mach.ProcessCharacter(L':'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); - } + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); mach.ProcessCharacter(L'0' + i % 10); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); } From 05b6bf6e468bba8b87a11ca1007652c7258cd3bb Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Thu, 6 Jul 2023 12:33:26 +0530 Subject: [PATCH 06/32] rename sub parameter ranges --- src/terminal/adapter/DispatchTypes.hpp | 18 +++++++++--------- src/terminal/parser/stateMachine.cpp | 16 ++++++++-------- src/terminal/parser/stateMachine.hpp | 2 +- .../parser/ut_parser/OutputEngineTest.cpp | 14 +++++++------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 6a97d17efc6..e7c340bef7f 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -162,14 +162,14 @@ namespace Microsoft::Console::VirtualTerminal constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount) noexcept : _params{ paramsPtr, paramsCount }, _subParams{}, - _subParamsRange{} + _subParamRanges{} { } - constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount, const VTParameter* subParamsPtr, const size_t subParamsCount, const std::pair* subParamsRangePtr, const size_t subParamsRangeCount) noexcept : + constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount, const VTParameter* subParamsPtr, const size_t subParamsCount, const std::pair* subParamRangesPtr, const size_t subParamRangesCount) noexcept : _params{ paramsPtr, paramsCount }, _subParams{ subParamsPtr, subParamsCount }, - _subParamsRange{ subParamsRangePtr, subParamsRangeCount } + _subParamRanges{ subParamRangesPtr, subParamRangesCount } { } @@ -194,23 +194,23 @@ namespace Microsoft::Console::VirtualTerminal VTParameters subspan(const size_t offset) const noexcept { // We need sub parameters to always be in their original index - // because we store their indexes in subParamsRange. So we pass + // because we store their indexes in subParamRanges. So we pass // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); - const auto newSubParamsRangeSpan = _subParamsRange.subspan(std::min(offset, _params.size())); - return { newParamsSpan.data(), newParamsSpan.size(), _subParams.data(), _subParams.size(), newSubParamsRangeSpan.data(), newSubParamsRangeSpan.size() }; + const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _params.size())); + return { newParamsSpan.data(), newParamsSpan.size(), _subParams.data(), _subParams.size(), newSubParamRangesSpan.data(), newSubParamRangesSpan.size() }; } std::span subParamsFor(const size_t index) const noexcept { // If the parameter index is out of range, we return a sub-parameters span with no values. - if (index >= _subParamsRange.size()) + if (index >= _subParamRanges.size()) { return std::span{}; } else { - const auto& range = til::at(_subParamsRange, index); + const auto& range = til::at(_subParamRanges, index); return _subParams.subspan(range.first, range.second - range.first); } } @@ -241,7 +241,7 @@ namespace Microsoft::Console::VirtualTerminal std::span _params; std::span _subParams; - std::span> _subParamsRange; + std::span> _subParamRanges; }; // FlaggedEnumValue is a convenience class that produces enum values (of a specified size) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 643f72d879f..69dd68fd46d 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -169,7 +169,7 @@ static constexpr bool _isSubParameterDelimiter(const wchar_t wch) noexcept } // Routine Description: -// - Determines if a character is "control sequence" beginning indicator. +// - Determines if a character is a "control sequence" beginning indicator. // This immediately follows an escape and signifies a varying length control sequence. // Arguments: // - wch - Character to check. @@ -466,7 +466,7 @@ void StateMachine::_ActionCsiDispatch(const wchar_t wch) _trace.TraceOnAction(L"CsiDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { return _engine->ActionCsiDispatch(_identifier.Finalize(wch), - { _parameters.data(), _parameters.size(), _subParameters.data(), _subParameters.size(), _subParametersRange.data(), _subParametersRange.size() }); + { _parameters.data(), _parameters.size(), _subParameters.data(), _subParameters.size(), _subParameterRanges.data(), _subParameterRanges.size() }); })); } @@ -502,8 +502,8 @@ void StateMachine::_ActionParam(const wchar_t wch) if (_parameters.empty()) { _parameters.push_back({}); - auto rangeStart = gsl::narrow(_subParameters.size()); - _subParametersRange.push_back({ rangeStart, rangeStart }); + const auto rangeStart = gsl::narrow(_subParameters.size()); + _subParameterRanges.push_back({ rangeStart, rangeStart }); } // On a delimiter, increase the number of params we've seen. @@ -522,8 +522,8 @@ void StateMachine::_ActionParam(const wchar_t wch) { // Otherwise move to next param. _parameters.push_back({}); - auto rangeStart = gsl::narrow(_subParameters.size()); - _subParametersRange.push_back({ rangeStart, rangeStart }); + const auto rangeStart = gsl::narrow(_subParameters.size()); + _subParameterRanges.push_back({ rangeStart, rangeStart }); } } else @@ -568,7 +568,7 @@ void StateMachine::_ActionSubParam(const wchar_t wch) // Otherwise move to next sub-param. _subParameters.push_back({}); // increment current range's end index. - _subParametersRange.back().second++; + _subParameterRanges.back().second++; } } else @@ -599,7 +599,7 @@ void StateMachine::_ActionClear() _parameterLimitReached = false; _subParameters.clear(); - _subParametersRange.clear(); + _subParameterRanges.clear(); _subParameterLimitReached = false; _oscString.clear(); diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index d478b79ff84..b88d5eda2e7 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -197,7 +197,7 @@ namespace Microsoft::Console::VirtualTerminal std::vector _parameters; bool _parameterLimitReached; std::vector _subParameters; - std::vector> _subParametersRange; + std::vector> _subParameterRanges; bool _subParameterLimitReached; std::wstring _oscString; diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 379b00d9969..6186f8df6e1 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -429,15 +429,15 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_IS_FALSE(mach._subParameters.at(1).has_value()); VERIFY_ARE_EQUAL(mach._subParameters.at(2), 8); - VERIFY_ARE_EQUAL(mach._subParametersRange.at(0).first, 0); - VERIFY_ARE_EQUAL(mach._subParametersRange.at(0).second, 0); - VERIFY_ARE_EQUAL(mach._subParametersRange.at(1).first, 0); - VERIFY_ARE_EQUAL(mach._subParametersRange.at(1).second, 3); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(0).first, 0); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(0).second, 0); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).first, 0); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).second, 3); - VERIFY_ARE_EQUAL(mach._subParametersRange.size(), mach._parameters.size()); + VERIFY_ARE_EQUAL(mach._subParameterRanges.size(), mach._parameters.size()); VERIFY_IS_TRUE( - (mach._subParametersRange.back().second == mach._subParameters.size() - 1) // lastIndex - || (mach._subParametersRange.back().second == mach._subParameters.size()) // or lastIndex + 1 + (mach._subParameterRanges.back().second == mach._subParameters.size() - 1) // lastIndex + || (mach._subParameterRanges.back().second == mach._subParameters.size()) // or lastIndex + 1 ); } From 41804657855230ea954a12df2f6a435adce17936 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Thu, 6 Jul 2023 14:45:19 +0530 Subject: [PATCH 07/32] fix subspan creation --- src/terminal/adapter/DispatchTypes.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index e7c340bef7f..818b916b5db 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -197,7 +197,7 @@ namespace Microsoft::Console::VirtualTerminal // because we store their indexes in subParamRanges. So we pass // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); - const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _params.size())); + const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); return { newParamsSpan.data(), newParamsSpan.size(), _subParams.data(), _subParams.size(), newSubParamRangesSpan.data(), newSubParamRangesSpan.size() }; } From ce9ce2f3c7307dff0c43145f50d29318de4917eb Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Fri, 7 Jul 2023 21:07:48 +0530 Subject: [PATCH 08/32] invalidate sequences with extra sub parameters --- src/terminal/adapter/DispatchTypes.hpp | 46 ++++++++++--- src/terminal/adapter/adaptDispatch.hpp | 4 ++ .../adapter/adaptDispatchGraphics.cpp | 64 ++++++++++++++++++- .../parser/OutputStateMachineEngine.cpp | 17 +++++ .../parser/OutputStateMachineEngine.hpp | 2 + src/terminal/parser/stateMachine.cpp | 2 +- 6 files changed, 124 insertions(+), 11 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 818b916b5db..b471c91e163 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -166,10 +166,12 @@ namespace Microsoft::Console::VirtualTerminal { } - constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount, const VTParameter* subParamsPtr, const size_t subParamsCount, const std::pair* subParamRangesPtr, const size_t subParamRangesCount) noexcept : - _params{ paramsPtr, paramsCount }, - _subParams{ subParamsPtr, subParamsCount }, - _subParamRanges{ subParamRangesPtr, subParamRangesCount } + constexpr VTParameters(const std::span params, + const std::span subParams = {}, + const std::span> subParamRanges = {}) noexcept : + _params{ params }, + _subParams{ subParams }, + _subParamRanges{ subParamRanges } { } @@ -198,20 +200,37 @@ namespace Microsoft::Console::VirtualTerminal // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); - return { newParamsSpan.data(), newParamsSpan.size(), _subParams.data(), _subParams.size(), newSubParamRangesSpan.data(), newSubParamRangesSpan.size() }; + return { newParamsSpan, _subParams, newSubParamRangesSpan }; } std::span subParamsFor(const size_t index) const noexcept { - // If the parameter index is out of range, we return a sub-parameters span with no values. - if (index >= _subParamRanges.size()) + if (index < _subParamRanges.size()) { - return std::span{}; + const auto& range = til::at(_subParamRanges, index); + return _subParams.subspan(range.first, range.second - range.first); } else + { + return std::span{}; + } + } + + bool hasSubParams() const noexcept + { + return !_subParams.empty(); + } + + bool hasSubParamsFor(const size_t index) const noexcept + { + if (index < _subParamRanges.size()) { const auto& range = til::at(_subParamRanges, index); - return _subParams.subspan(range.first, range.second - range.first); + return ((range.second - range.first) > range.first); + } + else + { + return false; } } @@ -388,6 +407,15 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes BrightBackgroundWhite = 107, }; + // we use this in VT parser to indicate that the GraphicsOptions (parameter) + // accepts sub-parameters. + enum GraphicsOptionsWithSubParams : VTInt + { +#ifdef UNIT_TESTING + TestOption = 12345, +#endif + }; + enum LogicalAttributeOptions : VTInt { Default = 0, diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 674cafc4506..15dd2b65412 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -294,12 +294,16 @@ namespace Microsoft::Console::VirtualTerminal SgrStack _sgrStack; + bool _CanAcceptSubParam(const VTInt param) const noexcept; size_t _SetRgbColorsHelper(const VTParameters options, TextAttribute& attr, const bool isForeground) noexcept; size_t _ApplyGraphicsOption(const VTParameters options, const size_t optionIndex, TextAttribute& attr) noexcept; + void _ApplyGraphicsOptionSubParam(const VTParameter option, + const std::span subParams, + TextAttribute& attr) noexcept; void _ApplyGraphicsOptions(const VTParameters options, TextAttribute& attr) noexcept; diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index 0de71a293ae..ea1527e279e 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -12,6 +12,32 @@ using namespace Microsoft::Console::VirtualTerminal; using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; +// Routine Description: +// - This returns false until we have something meaningful to do with sub parameters. +// - Takes the parameter and determines if it accepts sub parameters. +// Arguments: +// - parameter - the parameter to check for. +// Return Value: +// - True, if it accepts sub parameters, or else False. +bool AdaptDispatch::_CanAcceptSubParam(const VTInt /* param */) const noexcept +{ + // This is how we would've implemented it if we had sub parameters. + /* + using enum DispatchTypes::GraphicsOptionsWithSubParams; + switch (param) + { + case ForegroundExtended: + case BackgroundExtended: + case UnderlineExtended: + case UnderlineColor: + return true; + default: + return false; + } +*/ + return false; +} + // Routine Description: // - Helper to parse extended graphics options, which start with 38 (FG) or 48 (BG) // These options are followed by either a 2 (RGB) or 5 (xterm index) @@ -250,8 +276,23 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options, } } +// Routine Description: +// - This is a no-op until we have something meaningful to do with sub parameters. +// - Helper to apply a single graphic rendition option with sub parameters to an attribute. +// Arguments: +// - option - An option to apply. +// - attr - The attribute that will be updated with the applied option. +// Return Value: +// - +void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */, + const std::span /* subParam */, + TextAttribute& /* attr */) noexcept +{ +} + // Routine Description: // - Helper to apply a number of graphic rendition options to an attribute. +// - Applies the changes iff all the options are valid. // Arguments: // - options - An array of options that will be applied in sequence. // - attr - The attribute that will be updated with the applied options. @@ -260,9 +301,30 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options, void AdaptDispatch::_ApplyGraphicsOptions(const VTParameters options, TextAttribute& attr) noexcept { + // Save the current attribute so we can restore it later. + const auto currentAttr = attr; for (size_t i = 0; i < options.size();) { - i += _ApplyGraphicsOption(options, i, attr); + // We must discard the changes applied to the attr if we encounter + // sub parameters to a parameter which doesn't accept them. This is to + // ensure that we never interpret a sequence ignoring the sub parameters. + if (options.hasSubParamsFor(i)) + { + if (_CanAcceptSubParam(options.at(i))) + { + _ApplyGraphicsOptionSubParam(options.at(i), options.subParamsFor(i), attr); + i += 1; + } + else + { + attr = currentAttr; + break; + } + } + else + { + i += _ApplyGraphicsOption(options, i, attr); + } } } diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 84ec0fa90e0..7d2a8ccb7e2 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -416,6 +416,12 @@ bool OutputStateMachineEngine::ActionVt52EscDispatch(const VTID id, const VTPara // - true iff we successfully dispatched the sequence. bool OutputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameters parameters) { + // Bail out if we receive subparameters, but we don't accept them in the sequence. + if (parameters.hasSubParams() && !_CanSeqAcceptSubParam(id)) [[unlikely]] + { + return false; + } + auto success = false; switch (id) @@ -1098,6 +1104,17 @@ bool OutputStateMachineEngine::_GetOscSetClipboard(const std::wstring_view strin return SUCCEEDED_LOG(Base64::Decode(substr, content)); } +// Routine Description: +// - Takes a sequence id (parameter) and determines if it accepts sub parameters. +// Arguments: +// - id - The sequence id to check for. +// Return Value: +// - True, if it accepts sub parameters or else False. +bool OutputStateMachineEngine::_CanSeqAcceptSubParam(const VTID id) noexcept +{ + return id == CsiActionCodes::SGR_SetGraphicsRendition; +} + // Method Description: // - Clears our last stored character. The last stored character is the last // graphical character we printed, which is reset if any other action is diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp index edb8982f7da..3c91c06712a 100644 --- a/src/terminal/parser/OutputStateMachineEngine.hpp +++ b/src/terminal/parser/OutputStateMachineEngine.hpp @@ -236,6 +236,8 @@ namespace Microsoft::Console::VirtualTerminal std::wstring& params, std::wstring& uri) const; + bool _CanSeqAcceptSubParam(const VTID id) noexcept; + void _ClearLastChar() noexcept; }; } diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 69dd68fd46d..28b9b33c347 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -466,7 +466,7 @@ void StateMachine::_ActionCsiDispatch(const wchar_t wch) _trace.TraceOnAction(L"CsiDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { return _engine->ActionCsiDispatch(_identifier.Finalize(wch), - { _parameters.data(), _parameters.size(), _subParameters.data(), _subParameters.size(), _subParameterRanges.data(), _subParameterRanges.size() }); + { _parameters, _subParameters, _subParameterRanges }); })); } From 94e91563b1d7dc3c2cdb060ee04e467f24bef197 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Fri, 7 Jul 2023 21:13:12 +0530 Subject: [PATCH 09/32] fix typo --- src/terminal/parser/OutputStateMachineEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 7d2a8ccb7e2..5401d350c1b 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -1105,7 +1105,7 @@ bool OutputStateMachineEngine::_GetOscSetClipboard(const std::wstring_view strin } // Routine Description: -// - Takes a sequence id (parameter) and determines if it accepts sub parameters. +// - Takes a sequence id ("final byte") and determines if it accepts sub parameters. // Arguments: // - id - The sequence id to check for. // Return Value: From e6bbfbb549a960d9e58e9e4915889b6a559cb41c Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Fri, 7 Jul 2023 21:21:19 +0530 Subject: [PATCH 10/32] correct spell check --- src/terminal/adapter/adaptDispatchGraphics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index ea1527e279e..2b3796dc7fd 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -21,7 +21,7 @@ using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; // - True, if it accepts sub parameters, or else False. bool AdaptDispatch::_CanAcceptSubParam(const VTInt /* param */) const noexcept { - // This is how we would've implemented it if we had sub parameters. + // This is how we would have implemented it if we had sub parameters. /* using enum DispatchTypes::GraphicsOptionsWithSubParams; switch (param) From 0c0da7208b72a3616d1f8bbcadf36e93ffdd29c8 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Sat, 8 Jul 2023 11:10:13 +0530 Subject: [PATCH 11/32] fix the check for hasSubParamsFor --- src/terminal/adapter/DispatchTypes.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index b471c91e163..04b036003f1 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -226,7 +226,7 @@ namespace Microsoft::Console::VirtualTerminal if (index < _subParamRanges.size()) { const auto& range = til::at(_subParamRanges, index); - return ((range.second - range.first) > range.first); + return range.second > range.first; } else { From 7869d85c63f52944f3b2d89d28d97438902ed4ec Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Sun, 9 Jul 2023 13:19:26 +0530 Subject: [PATCH 12/32] skip parameter with unrecognised sub parameters --- src/terminal/adapter/adaptDispatch.hpp | 1 - .../adapter/adaptDispatchGraphics.cpp | 46 ++----------------- 2 files changed, 5 insertions(+), 42 deletions(-) diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 15dd2b65412..059fb3852c5 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -294,7 +294,6 @@ namespace Microsoft::Console::VirtualTerminal SgrStack _sgrStack; - bool _CanAcceptSubParam(const VTInt param) const noexcept; size_t _SetRgbColorsHelper(const VTParameters options, TextAttribute& attr, const bool isForeground) noexcept; diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index 2b3796dc7fd..fa8bca3d7fd 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -12,32 +12,6 @@ using namespace Microsoft::Console::VirtualTerminal; using namespace Microsoft::Console::VirtualTerminal::DispatchTypes; -// Routine Description: -// - This returns false until we have something meaningful to do with sub parameters. -// - Takes the parameter and determines if it accepts sub parameters. -// Arguments: -// - parameter - the parameter to check for. -// Return Value: -// - True, if it accepts sub parameters, or else False. -bool AdaptDispatch::_CanAcceptSubParam(const VTInt /* param */) const noexcept -{ - // This is how we would have implemented it if we had sub parameters. - /* - using enum DispatchTypes::GraphicsOptionsWithSubParams; - switch (param) - { - case ForegroundExtended: - case BackgroundExtended: - case UnderlineExtended: - case UnderlineColor: - return true; - default: - return false; - } -*/ - return false; -} - // Routine Description: // - Helper to parse extended graphics options, which start with 38 (FG) or 48 (BG) // These options are followed by either a 2 (RGB) or 5 (xterm index) @@ -288,6 +262,9 @@ void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */, const std::span /* subParam */, TextAttribute& /* attr */) noexcept { + // here, we apply our "best effort" rule, while handling sub params if we don't + // recognise the parameter substring (parameter and it's sub parameters) then + // we should just skip over them. } // Routine Description: @@ -301,25 +278,12 @@ void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */, void AdaptDispatch::_ApplyGraphicsOptions(const VTParameters options, TextAttribute& attr) noexcept { - // Save the current attribute so we can restore it later. - const auto currentAttr = attr; for (size_t i = 0; i < options.size();) { - // We must discard the changes applied to the attr if we encounter - // sub parameters to a parameter which doesn't accept them. This is to - // ensure that we never interpret a sequence ignoring the sub parameters. if (options.hasSubParamsFor(i)) { - if (_CanAcceptSubParam(options.at(i))) - { - _ApplyGraphicsOptionSubParam(options.at(i), options.subParamsFor(i), attr); - i += 1; - } - else - { - attr = currentAttr; - break; - } + _ApplyGraphicsOptionSubParam(options.at(i), options.subParamsFor(i), attr); + i += 1; } else { From ee58a9e46d188d6da2373a9e22a5222f648456d4 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Sun, 9 Jul 2023 14:13:39 +0530 Subject: [PATCH 13/32] discard parameter when we can't handle its sub parameter(s) Introduce a new state CsiParamIgnore which helps in ignoring a parameter and its sub parameter. Also, it enters back to CsiParam when a parameter delimiter(;) is detected. --- src/terminal/parser/stateMachine.cpp | 185 ++++++++++++++++++++------- src/terminal/parser/stateMachine.hpp | 5 + 2 files changed, 147 insertions(+), 43 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 28b9b33c347..1bcd1c23e3c 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -17,6 +17,7 @@ StateMachine::StateMachine(std::unique_ptr engine, const bo _trace(Microsoft::Console::VirtualTerminal::ParserTracing()), _parameters{}, _parameterLimitReached(false), + _subParameterLimitReached(false), _oscString{}, _cachedSequence{ std::nullopt } { @@ -538,8 +539,10 @@ void StateMachine::_ActionParam(const wchar_t wch) } // Routine Description: -// - Triggers the SubParam action to indicate that the state machine should store this character as a part of a sub-parameter -// to a control sequence. +// - Triggers the SubParam action to indicate that the state machine should +// store this character as a part of a sub-parameter to a control sequence. +// - Assumes that enough checks have been made to handle a new sub parameter, +// for eg., checking if sub parameter limit is not reached. // Arguments: // - wch - Character to dispatch. // Return Value: @@ -547,38 +550,20 @@ void StateMachine::_ActionParam(const wchar_t wch) void StateMachine::_ActionSubParam(const wchar_t wch) { _trace.TraceOnAction(L"SubParam"); - - // Once we've reached the sub-parameter limit, additional sub-parameters are ignored. - if (!_subParameterLimitReached) + if (_isSubParameterDelimiter(wch)) { - // On a delimiter, increase the number of sub-params we've seen. - // "Empty" sub-params should still count as a sub-param - - // eg "\x1b[38:2::m" should be three sub-params - if (_isSubParameterDelimiter(wch)) - { - // If we receive a delimiter after we've already accumulated the - // maximum allowed sub-parameters, then we need to set a flag to - // indicate that further sub-parameter characters should be ignored. - if (_subParameters.size() >= MAX_SUBPARAMETER_COUNT) - { - _subParameterLimitReached = true; - } - else - { - // Otherwise move to next sub-param. - _subParameters.push_back({}); - // increment current range's end index. - _subParameterRanges.back().second++; - } - } - else - { - // Accumulate the character given into the last (current) sub-parameter. - // If the value hasn't been initialized yet, it'll start as 0. - auto currentParameter = _subParameters.back().value_or(0); - _AccumulateTo(wch, currentParameter); - _subParameters.back() = currentParameter; - } + // Otherwise move to next sub-param. + _subParameters.push_back({}); + // increment current range's end index. + _subParameterRanges.back().second++; + } + else + { + // Accumulate the character given into the last (current) sub-parameter. + // If the value hasn't been initialized yet, it'll start as 0. + auto currentSubParameter = _subParameters.back().value_or(0); + _AccumulateTo(wch, currentSubParameter); + _subParameters.back() = currentSubParameter; } } @@ -613,7 +598,7 @@ void StateMachine::_ActionClear() // Routine Description: // - Triggers the Ignore action to indicate that the state machine should eat this character and say nothing. // Arguments: -// - wch - Character to dispatch. +// - // Return Value: // - void StateMachine::_ActionIgnore() noexcept @@ -622,6 +607,19 @@ void StateMachine::_ActionIgnore() noexcept _trace.TraceOnAction(L"Ignore"); } +// Routine Description: +// - Triggers the ParamIgnore action to indicate that the state machine should eat this character and say nothing. +// Arguments: +// - +// Return Value: +// - +void StateMachine::_ActionParamIgnore() noexcept +{ + _trace.TraceOnAction(L"ParamIgnore"); + _parameters.pop_back(); + _subParameterRanges.pop_back(); +} + // Routine Description: // - Triggers the end of a data string when a CAN, SUB, or ESC is seen. // Arguments: @@ -832,6 +830,22 @@ void StateMachine::_EnterCsiIgnore() noexcept _trace.TraceStateChange(L"CsiIgnore"); } +// Routine Description: +// - Moves the state machine into the CsiParamIgnore state. +// This state is entered: +// 1. When we couldn't handle a character in CSI sequence because sub parameter limit is reached, +// indicating we should ignore the parameter along with its sub parameter(s). +// (From CsiParam, CsiSubParam states.) +// Arguments: +// - +// Return Value: +// - +void StateMachine::_EnterCsiParamIgnore() noexcept +{ + _state = VTStates::CsiParamIgnore; + _trace.TraceStateChange(L"CsiParamIgnore"); +} + // Routine Description: // - Moves the state machine into the CsiIntermediate state. // This state is entered: @@ -1310,6 +1324,54 @@ void StateMachine::_EventCsiIgnore(const wchar_t wch) } } +// Routine Description: +// - Processes a character event into an Action that occurs while in the CsiParamIgnore state. +// Events in this state will: +// 1. Ignore C0 control characters +// 2. Ignore Delete characters +// 3. Ignore Intermediate characters +// 4. Store parameter data +// 5. Ignore sub parameter data +// 6. Ignore intermediate invalid characters +// 7. Return to Ground +// Arguments: +// - wch - Character that triggered the event +// Return Value: +// - +void StateMachine::_EventCsiParamIgnore(const wchar_t wch) +{ + _trace.TraceOnEvent(L"CsiParamIgnore"); + if (_isC0Code(wch)) + { + _ActionIgnore(); + } + else if (_isDelete(wch)) + { + _ActionIgnore(); + } + else if (_isParameterDelimiter(wch)) + { + _ActionParam(wch); + _EnterCsiParam(); + } + else if (_isNumericParamValue(wch) || _isSubParameterDelimiter(wch)) + { + _ActionIgnore(); + } + else if (_isIntermediate(wch)) + { + _ActionIgnore(); + } + else if (_isIntermediateInvalid(wch)) + { + _ActionIgnore(); + } + else + { + _EnterGround(); + } +} + // Routine Description: // - Processes a character event into an Action that occurs while in the CsiParam state. // Events in this state will: @@ -1318,7 +1380,9 @@ void StateMachine::_EventCsiIgnore(const wchar_t wch) // 3. Collect Intermediate characters // 4. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) // 5. Store parameter data -// 6. Store sub parameter data +// 6. Handle sub parameter: +// 6.1. Store the sub param if the sub parameter limit is not reached. +// 6.2. Else, ignore the latest parameter and its sub parameter(s) by entering CsiParamIgnore. // 7. Dispatch a control sequence with parameters for action // Arguments: // - wch - Character that triggered the event @@ -1341,8 +1405,16 @@ void StateMachine::_EventCsiParam(const wchar_t wch) } else if (_isSubParameterDelimiter(wch)) { - _ActionSubParam(wch); - _EnterCsiSubParam(); + if (_CanHandleSubParam()) + { + _ActionSubParam(wch); + _EnterCsiSubParam(); + } + else + { + _ActionParamIgnore(); + _EnterCsiParamIgnore(); + } } else if (_isIntermediate(wch)) { @@ -1366,11 +1438,14 @@ void StateMachine::_EventCsiParam(const wchar_t wch) // Events in this state will: // 1. Execute C0 control characters // 2. Ignore Delete characters -// 3. Store sub parameter data -// 4. Store parameter data -// 5. Collect Intermediate characters for parameter. -// 6. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) -// 7. Dispatch a control sequence with parameters for action +// 3. Store sub parameter digit. +// 4. Handle new sub param: +// 4.1. Store the sub param if the sub parameter limit is not reached. +// 4.2. Else, ignore the latest parameter and its sub parameter(s) by entering CsiParamIgnore. +// 5. Store parameter data +// 6. Collect Intermediate characters for parameter. +// 7. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) +// 8. Dispatch a control sequence with parameters for action // Arguments: // - wch - Character that triggered the event // Return Value: @@ -1386,10 +1461,23 @@ void StateMachine::_EventCsiSubParam(const wchar_t wch) { _ActionIgnore(); } - else if (_isNumericParamValue(wch) || _isSubParameterDelimiter(wch)) + else if (_isNumericParamValue(wch)) { _ActionSubParam(wch); } + else if (_isSubParameterDelimiter(wch)) + { + if (_CanHandleSubParam()) + { + _ActionSubParam(wch); + _EnterCsiSubParam(); + } + else + { + _ActionParamIgnore(); + _EnterCsiParamIgnore(); + } + } else if (_isParameterDelimiter(wch)) { _ActionParam(wch); @@ -1864,6 +1952,8 @@ void StateMachine::ProcessCharacter(const wchar_t wch) return _EventCsiParam(wch); case VTStates::CsiSubParam: return _EventCsiSubParam(wch); + case VTStates::CsiParamIgnore: + return _EventCsiParamIgnore(wch); case VTStates::OscParam: return _EventOscParam(wch); case VTStates::OscString: @@ -2155,6 +2245,8 @@ void StateMachine::ProcessString(const std::wstring_view string) case VTStates::CsiIntermediate: case VTStates::CsiIgnore: case VTStates::CsiParam: + case VTStates::CsiSubParam: + case VTStates::CsiParamIgnore: _ActionCsiDispatch(*wchIter); break; case VTStates::OscParam: @@ -2250,6 +2342,13 @@ void StateMachine::_AccumulateTo(const wchar_t wch, VTInt& value) noexcept } } +bool StateMachine::_CanHandleSubParam() noexcept +{ + // Lazily check if limit is reached. + _subParameterLimitReached = _subParameterLimitReached || (_subParameters.size() >= MAX_SUBPARAMETER_COUNT); + return !_subParameterLimitReached; +} + template bool StateMachine::_SafeExecute(TLambda&& lambda) try diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index b88d5eda2e7..528691f8542 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -96,6 +96,7 @@ namespace Microsoft::Console::VirtualTerminal void _ActionClear(); void _ActionIgnore() noexcept; + void _ActionParamIgnore() noexcept; void _ActionInterrupt(); void _EnterGround() noexcept; @@ -103,6 +104,7 @@ namespace Microsoft::Console::VirtualTerminal void _EnterEscapeIntermediate() noexcept; void _EnterCsiEntry(); void _EnterCsiParam() noexcept; + void _EnterCsiParamIgnore() noexcept; void _EnterCsiSubParam() noexcept; void _EnterCsiIgnore() noexcept; void _EnterCsiIntermediate() noexcept; @@ -125,6 +127,7 @@ namespace Microsoft::Console::VirtualTerminal void _EventCsiEntry(const wchar_t wch); void _EventCsiIntermediate(const wchar_t wch); void _EventCsiIgnore(const wchar_t wch); + void _EventCsiParamIgnore(const wchar_t wch); void _EventCsiParam(const wchar_t wch); void _EventCsiSubParam(const wchar_t wch); void _EventOscParam(const wchar_t wch) noexcept; @@ -141,6 +144,7 @@ namespace Microsoft::Console::VirtualTerminal void _EventSosPmApcString(const wchar_t wch) noexcept; void _AccumulateTo(const wchar_t wch, VTInt& value) noexcept; + bool _CanHandleSubParam() noexcept; template bool _SafeExecute(TLambda&& lambda); @@ -157,6 +161,7 @@ namespace Microsoft::Console::VirtualTerminal CsiIgnore, CsiParam, CsiSubParam, + CsiParamIgnore, OscParam, OscString, OscTermination, From 4d3049aaa1d5233ac8f93bccdc30dbaef22835e7 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Sun, 9 Jul 2023 19:39:35 +0530 Subject: [PATCH 14/32] fix small error message while we are at it constexpr variables must be initialized at the point of declaration if constructor is outside the class. --- src/terminal/adapter/DispatchTypes.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 04b036003f1..b29373ce690 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -255,7 +255,7 @@ namespace Microsoft::Console::VirtualTerminal } private: - static constexpr VTParameter defaultParameter; + static constexpr VTParameter defaultParameter{}; static constexpr std::span defaultParameters{ &defaultParameter, 1 }; std::span _params; From c5334c73c528f769609d4a7952ab5da67b5421b2 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Sun, 9 Jul 2023 22:50:08 +0530 Subject: [PATCH 15/32] handle DECCARA and DECRARA sequences with sub parameters --- src/terminal/adapter/DispatchTypes.hpp | 19 ++++++++++++++----- .../parser/OutputStateMachineEngine.cpp | 19 ++++++++++++++++--- .../parser/OutputStateMachineEngine.hpp | 2 +- src/terminal/parser/stateMachine.cpp | 9 +++++++-- src/terminal/parser/stateMachine.hpp | 7 +++++++ 5 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index b29373ce690..f49e843bc1c 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -162,16 +162,19 @@ namespace Microsoft::Console::VirtualTerminal constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount) noexcept : _params{ paramsPtr, paramsCount }, _subParams{}, - _subParamRanges{} + _subParamRanges{}, + _isParameterOmitted{ false } { } constexpr VTParameters(const std::span params, - const std::span subParams = {}, - const std::span> subParamRanges = {}) noexcept : + const std::span subParams, + const std::span> subParamRanges, + const bool isParamOmitted) noexcept : _params{ params }, _subParams{ subParams }, - _subParamRanges{ subParamRanges } + _subParamRanges{ subParamRanges }, + _isParameterOmitted{ isParamOmitted } { } @@ -200,7 +203,7 @@ namespace Microsoft::Console::VirtualTerminal // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); - return { newParamsSpan, _subParams, newSubParamRangesSpan }; + return { newParamsSpan, _subParams, newSubParamRangesSpan, _isParameterOmitted }; } std::span subParamsFor(const size_t index) const noexcept @@ -234,6 +237,11 @@ namespace Microsoft::Console::VirtualTerminal } } + bool isParameterOmitted() const noexcept + { + return _isParameterOmitted; + } + template bool for_each(const T&& predicate) const { @@ -261,6 +269,7 @@ namespace Microsoft::Console::VirtualTerminal std::span _params; std::span _subParams; std::span> _subParamRanges; + bool _isParameterOmitted; }; // FlaggedEnumValue is a convenience class that produces enum values (of a specified size) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index 5401d350c1b..ecabdf647d5 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -417,7 +417,7 @@ bool OutputStateMachineEngine::ActionVt52EscDispatch(const VTID id, const VTPara bool OutputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameters parameters) { // Bail out if we receive subparameters, but we don't accept them in the sequence. - if (parameters.hasSubParams() && !_CanSeqAcceptSubParam(id)) [[unlikely]] + if (parameters.hasSubParams() && !_CanSeqAcceptSubParam(id, parameters)) [[unlikely]] { return false; } @@ -1110,9 +1110,22 @@ bool OutputStateMachineEngine::_GetOscSetClipboard(const std::wstring_view strin // - id - The sequence id to check for. // Return Value: // - True, if it accepts sub parameters or else False. -bool OutputStateMachineEngine::_CanSeqAcceptSubParam(const VTID id) noexcept +bool OutputStateMachineEngine::_CanSeqAcceptSubParam(const VTID id, const VTParameters& parameters) noexcept { - return id == CsiActionCodes::SGR_SetGraphicsRendition; + switch (id) + { + case SGR_SetGraphicsRendition: + return true; + case DECCARA_ChangeAttributesRectangularArea: + case DECRARA_ReverseAttributesRectangularArea: + return !parameters.isParameterOmitted() + && !parameters.hasSubParamsFor(0) + && !parameters.hasSubParamsFor(1) + && !parameters.hasSubParamsFor(2) + && !parameters.hasSubParamsFor(3); + default: + return false; + } } // Method Description: diff --git a/src/terminal/parser/OutputStateMachineEngine.hpp b/src/terminal/parser/OutputStateMachineEngine.hpp index 3c91c06712a..b078ade9fb7 100644 --- a/src/terminal/parser/OutputStateMachineEngine.hpp +++ b/src/terminal/parser/OutputStateMachineEngine.hpp @@ -236,7 +236,7 @@ namespace Microsoft::Console::VirtualTerminal std::wstring& params, std::wstring& uri) const; - bool _CanSeqAcceptSubParam(const VTID id) noexcept; + bool _CanSeqAcceptSubParam(const VTID id, const VTParameters& parameters) noexcept; void _ClearLastChar() noexcept; }; diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 1bcd1c23e3c..8d8d8ad5788 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -16,10 +16,13 @@ StateMachine::StateMachine(std::unique_ptr engine, const bo _state(VTStates::Ground), _trace(Microsoft::Console::VirtualTerminal::ParserTracing()), _parameters{}, + _subParameters{}, + _subParameterRanges{}, _parameterLimitReached(false), _subParameterLimitReached(false), _oscString{}, - _cachedSequence{ std::nullopt } + _cachedSequence{ std::nullopt }, + _isParameterOmitted{false} { _ActionClear(); } @@ -467,7 +470,7 @@ void StateMachine::_ActionCsiDispatch(const wchar_t wch) _trace.TraceOnAction(L"CsiDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { return _engine->ActionCsiDispatch(_identifier.Finalize(wch), - { _parameters, _subParameters, _subParameterRanges }); + { _parameters, _subParameters, _subParameterRanges, _isParameterOmitted }); })); } @@ -582,6 +585,7 @@ void StateMachine::_ActionClear() _parameters.clear(); _parameterLimitReached = false; + _isParameterOmitted = false; _subParameters.clear(); _subParameterRanges.clear(); @@ -616,6 +620,7 @@ void StateMachine::_ActionIgnore() noexcept void StateMachine::_ActionParamIgnore() noexcept { _trace.TraceOnAction(L"ParamIgnore"); + _isParameterOmitted = false; _parameters.pop_back(); _subParameterRanges.pop_back(); } diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 528691f8542..547dc584d54 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -205,6 +205,13 @@ namespace Microsoft::Console::VirtualTerminal std::vector> _subParameterRanges; bool _subParameterLimitReached; + // This flag is used to indicate that some parameters were omitted by the + // parser. This could happen if the parser was unable to parse all of the + // sub parameters for a given parameter. + // Sequences that are sensitive to parameter omission can use this flag + // to determine if any parameters were omitted. + bool _isParameterOmitted; + std::wstring _oscString; VTInt _oscParameter; From e17c1cfe3b77ff1a1ee8cf4570c198ef553a6144 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 14:26:44 +0530 Subject: [PATCH 16/32] start ignoring (sub)parameters once we overflow the limit --- src/terminal/adapter/DispatchTypes.hpp | 17 +- .../parser/OutputStateMachineEngine.cpp | 3 +- src/terminal/parser/stateMachine.cpp | 221 ++++++------------ src/terminal/parser/stateMachine.hpp | 14 +- 4 files changed, 73 insertions(+), 182 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index f49e843bc1c..8a199a19673 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -162,19 +162,16 @@ namespace Microsoft::Console::VirtualTerminal constexpr VTParameters(const VTParameter* paramsPtr, const size_t paramsCount) noexcept : _params{ paramsPtr, paramsCount }, _subParams{}, - _subParamRanges{}, - _isParameterOmitted{ false } + _subParamRanges{} { } constexpr VTParameters(const std::span params, const std::span subParams, - const std::span> subParamRanges, - const bool isParamOmitted) noexcept : + const std::span> subParamRanges) noexcept : _params{ params }, _subParams{ subParams }, - _subParamRanges{ subParamRanges }, - _isParameterOmitted{ isParamOmitted } + _subParamRanges{ subParamRanges } { } @@ -203,7 +200,7 @@ namespace Microsoft::Console::VirtualTerminal // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); - return { newParamsSpan, _subParams, newSubParamRangesSpan, _isParameterOmitted }; + return { newParamsSpan, _subParams, newSubParamRangesSpan }; } std::span subParamsFor(const size_t index) const noexcept @@ -237,11 +234,6 @@ namespace Microsoft::Console::VirtualTerminal } } - bool isParameterOmitted() const noexcept - { - return _isParameterOmitted; - } - template bool for_each(const T&& predicate) const { @@ -269,7 +261,6 @@ namespace Microsoft::Console::VirtualTerminal std::span _params; std::span _subParams; std::span> _subParamRanges; - bool _isParameterOmitted; }; // FlaggedEnumValue is a convenience class that produces enum values (of a specified size) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index ecabdf647d5..cccaa1f1805 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -1118,8 +1118,7 @@ bool OutputStateMachineEngine::_CanSeqAcceptSubParam(const VTID id, const VTPara return true; case DECCARA_ChangeAttributesRectangularArea: case DECRARA_ReverseAttributesRectangularArea: - return !parameters.isParameterOmitted() - && !parameters.hasSubParamsFor(0) + return !parameters.hasSubParamsFor(0) && !parameters.hasSubParamsFor(1) && !parameters.hasSubParamsFor(2) && !parameters.hasSubParamsFor(3); diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 8d8d8ad5788..73212ee4df6 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -18,11 +18,9 @@ StateMachine::StateMachine(std::unique_ptr engine, const bo _parameters{}, _subParameters{}, _subParameterRanges{}, - _parameterLimitReached(false), - _subParameterLimitReached(false), + _parameterLimitOverflowed(false), _oscString{}, - _cachedSequence{ std::nullopt }, - _isParameterOmitted{false} + _cachedSequence{ std::nullopt } { _ActionClear(); } @@ -470,7 +468,7 @@ void StateMachine::_ActionCsiDispatch(const wchar_t wch) _trace.TraceOnAction(L"CsiDispatch"); _trace.DispatchSequenceTrace(_SafeExecute([=]() { return _engine->ActionCsiDispatch(_identifier.Finalize(wch), - { _parameters, _subParameters, _subParameterRanges, _isParameterOmitted }); + { _parameters, _subParameters, _subParameterRanges }); })); } @@ -500,7 +498,7 @@ void StateMachine::_ActionParam(const wchar_t wch) _trace.TraceOnAction(L"Param"); // Once we've reached the parameter limit, additional parameters are ignored. - if (!_parameterLimitReached) + if (!_parameterLimitOverflowed) { // If we have no parameters and we're about to add one, get the next value ready here. if (_parameters.empty()) @@ -520,7 +518,7 @@ void StateMachine::_ActionParam(const wchar_t wch) // indicate that further parameter characters should be ignored. if (_parameters.size() >= MAX_PARAMETER_COUNT) { - _parameterLimitReached = true; + _parameterLimitOverflowed = true; } else { @@ -542,10 +540,8 @@ void StateMachine::_ActionParam(const wchar_t wch) } // Routine Description: -// - Triggers the SubParam action to indicate that the state machine should +// - Triggers the SubParam action to indicate that the state machine should // store this character as a part of a sub-parameter to a control sequence. -// - Assumes that enough checks have been made to handle a new sub parameter, -// for eg., checking if sub parameter limit is not reached. // Arguments: // - wch - Character to dispatch. // Return Value: @@ -553,20 +549,51 @@ void StateMachine::_ActionParam(const wchar_t wch) void StateMachine::_ActionSubParam(const wchar_t wch) { _trace.TraceOnAction(L"SubParam"); - if (_isSubParameterDelimiter(wch)) - { - // Otherwise move to next sub-param. - _subParameters.push_back({}); - // increment current range's end index. - _subParameterRanges.back().second++; - } - else + + // Once we've reached the parameter limit, sub parameters are ignored. + if (!_parameterLimitOverflowed) { - // Accumulate the character given into the last (current) sub-parameter. - // If the value hasn't been initialized yet, it'll start as 0. - auto currentSubParameter = _subParameters.back().value_or(0); - _AccumulateTo(wch, currentSubParameter); - _subParameters.back() = currentSubParameter; + // If we have no parameters and we're about to add a sub parameter, add an empty parameter here. + if (_parameters.empty()) + { + _parameters.push_back({}); + const auto rangeStart = gsl::narrow(_subParameters.size()); + _subParameterRanges.push_back({ rangeStart, rangeStart }); + + } + + // On a delimiter, increase the number of sub params we've seen. + // "Empty" sub params should still count as a sub param - + // eg "\x1b[0:::m" should be three sub params + if (_isSubParameterDelimiter(wch)) + { + // If we receive a delimiter after we've already accumulated the + // maximum allowed sub parameters, then we need to set a flag to + // indicate that further sub parameter characters should be ignored. + // Similarly, we need to remove the last parameter so it isn't interpreted + // by the engine. + if (_subParameters.size() >= MAX_SUBPARAMETER_COUNT) + { + _parameterLimitOverflowed = true; + _parameters.pop_back(); + _subParameterRanges.pop_back(); + } + else + { + // Otherwise move to next sub-param. + _subParameters.push_back({}); + // increment current range's end index. + _subParameterRanges.back().second++; + } + } + else + { + // Accumulate the character given into the last (current) sub-parameter. + // If the value hasn't been initialized yet, it'll start as 0. + auto currentSubParameter = _subParameters.back().value_or(0); + _AccumulateTo(wch, currentSubParameter); + _subParameters.back() = currentSubParameter; + } } } @@ -584,12 +611,10 @@ void StateMachine::_ActionClear() _identifier.Clear(); _parameters.clear(); - _parameterLimitReached = false; - _isParameterOmitted = false; + _parameterLimitOverflowed = false; _subParameters.clear(); _subParameterRanges.clear(); - _subParameterLimitReached = false; _oscString.clear(); _oscParameter = 0; @@ -611,20 +636,6 @@ void StateMachine::_ActionIgnore() noexcept _trace.TraceOnAction(L"Ignore"); } -// Routine Description: -// - Triggers the ParamIgnore action to indicate that the state machine should eat this character and say nothing. -// Arguments: -// - -// Return Value: -// - -void StateMachine::_ActionParamIgnore() noexcept -{ - _trace.TraceOnAction(L"ParamIgnore"); - _isParameterOmitted = false; - _parameters.pop_back(); - _subParameterRanges.pop_back(); -} - // Routine Description: // - Triggers the end of a data string when a CAN, SUB, or ESC is seen. // Arguments: @@ -835,22 +846,6 @@ void StateMachine::_EnterCsiIgnore() noexcept _trace.TraceStateChange(L"CsiIgnore"); } -// Routine Description: -// - Moves the state machine into the CsiParamIgnore state. -// This state is entered: -// 1. When we couldn't handle a character in CSI sequence because sub parameter limit is reached, -// indicating we should ignore the parameter along with its sub parameter(s). -// (From CsiParam, CsiSubParam states.) -// Arguments: -// - -// Return Value: -// - -void StateMachine::_EnterCsiParamIgnore() noexcept -{ - _state = VTStates::CsiParamIgnore; - _trace.TraceStateChange(L"CsiParamIgnore"); -} - // Routine Description: // - Moves the state machine into the CsiIntermediate state. // This state is entered: @@ -1207,8 +1202,8 @@ void StateMachine::_EventEscapeIntermediate(const wchar_t wch) // 1. Execute C0 control characters // 2. Ignore Delete characters // 3. Collect Intermediate characters -// 4. Begin to ignore all remaining parameters when a sub parameter character is detected (CsiIgnore) -// 5. Store parameter data +// 4. Store parameter data +// 5. Store sub parameter data // 6. Collect Control Sequence Private markers // 7. Dispatch a control sequence with parameters for action // Arguments: @@ -1231,15 +1226,16 @@ void StateMachine::_EventCsiEntry(const wchar_t wch) _ActionCollect(wch); _EnterCsiIntermediate(); } - else if (_isSubParameterDelimiter(wch)) - { - _EnterCsiIgnore(); - } else if (_isNumericParamValue(wch) || _isParameterDelimiter(wch)) { _ActionParam(wch); _EnterCsiParam(); } + else if (_isSubParameterDelimiter(wch)) + { + _ActionSubParam(wch); + _EnterCsiSubParam(); + } else if (_isCsiPrivateMarker(wch)) { _ActionCollect(wch); @@ -1329,54 +1325,6 @@ void StateMachine::_EventCsiIgnore(const wchar_t wch) } } -// Routine Description: -// - Processes a character event into an Action that occurs while in the CsiParamIgnore state. -// Events in this state will: -// 1. Ignore C0 control characters -// 2. Ignore Delete characters -// 3. Ignore Intermediate characters -// 4. Store parameter data -// 5. Ignore sub parameter data -// 6. Ignore intermediate invalid characters -// 7. Return to Ground -// Arguments: -// - wch - Character that triggered the event -// Return Value: -// - -void StateMachine::_EventCsiParamIgnore(const wchar_t wch) -{ - _trace.TraceOnEvent(L"CsiParamIgnore"); - if (_isC0Code(wch)) - { - _ActionIgnore(); - } - else if (_isDelete(wch)) - { - _ActionIgnore(); - } - else if (_isParameterDelimiter(wch)) - { - _ActionParam(wch); - _EnterCsiParam(); - } - else if (_isNumericParamValue(wch) || _isSubParameterDelimiter(wch)) - { - _ActionIgnore(); - } - else if (_isIntermediate(wch)) - { - _ActionIgnore(); - } - else if (_isIntermediateInvalid(wch)) - { - _ActionIgnore(); - } - else - { - _EnterGround(); - } -} - // Routine Description: // - Processes a character event into an Action that occurs while in the CsiParam state. // Events in this state will: @@ -1385,9 +1333,7 @@ void StateMachine::_EventCsiParamIgnore(const wchar_t wch) // 3. Collect Intermediate characters // 4. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) // 5. Store parameter data -// 6. Handle sub parameter: -// 6.1. Store the sub param if the sub parameter limit is not reached. -// 6.2. Else, ignore the latest parameter and its sub parameter(s) by entering CsiParamIgnore. +// 6. Store sub parameter data // 7. Dispatch a control sequence with parameters for action // Arguments: // - wch - Character that triggered the event @@ -1410,16 +1356,8 @@ void StateMachine::_EventCsiParam(const wchar_t wch) } else if (_isSubParameterDelimiter(wch)) { - if (_CanHandleSubParam()) - { - _ActionSubParam(wch); - _EnterCsiSubParam(); - } - else - { - _ActionParamIgnore(); - _EnterCsiParamIgnore(); - } + _ActionSubParam(wch); + _EnterCsiSubParam(); } else if (_isIntermediate(wch)) { @@ -1443,14 +1381,11 @@ void StateMachine::_EventCsiParam(const wchar_t wch) // Events in this state will: // 1. Execute C0 control characters // 2. Ignore Delete characters -// 3. Store sub parameter digit. -// 4. Handle new sub param: -// 4.1. Store the sub param if the sub parameter limit is not reached. -// 4.2. Else, ignore the latest parameter and its sub parameter(s) by entering CsiParamIgnore. -// 5. Store parameter data -// 6. Collect Intermediate characters for parameter. -// 7. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) -// 8. Dispatch a control sequence with parameters for action +// 3. Store sub parameter data +// 4. Store parameter data +// 5. Collect Intermediate characters for parameter. +// 6. Begin to ignore all remaining parameters when an invalid character is detected (CsiIgnore) +// 7. Dispatch a control sequence with parameters for action // Arguments: // - wch - Character that triggered the event // Return Value: @@ -1466,23 +1401,10 @@ void StateMachine::_EventCsiSubParam(const wchar_t wch) { _ActionIgnore(); } - else if (_isNumericParamValue(wch)) + else if (_isNumericParamValue(wch) || _isSubParameterDelimiter(wch)) { _ActionSubParam(wch); } - else if (_isSubParameterDelimiter(wch)) - { - if (_CanHandleSubParam()) - { - _ActionSubParam(wch); - _EnterCsiSubParam(); - } - else - { - _ActionParamIgnore(); - _EnterCsiParamIgnore(); - } - } else if (_isParameterDelimiter(wch)) { _ActionParam(wch); @@ -1957,8 +1879,6 @@ void StateMachine::ProcessCharacter(const wchar_t wch) return _EventCsiParam(wch); case VTStates::CsiSubParam: return _EventCsiSubParam(wch); - case VTStates::CsiParamIgnore: - return _EventCsiParamIgnore(wch); case VTStates::OscParam: return _EventOscParam(wch); case VTStates::OscString: @@ -2347,13 +2267,6 @@ void StateMachine::_AccumulateTo(const wchar_t wch, VTInt& value) noexcept } } -bool StateMachine::_CanHandleSubParam() noexcept -{ - // Lazily check if limit is reached. - _subParameterLimitReached = _subParameterLimitReached || (_subParameters.size() >= MAX_SUBPARAMETER_COUNT); - return !_subParameterLimitReached; -} - template bool StateMachine::_SafeExecute(TLambda&& lambda) try diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 547dc584d54..9498a1f5067 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -96,7 +96,6 @@ namespace Microsoft::Console::VirtualTerminal void _ActionClear(); void _ActionIgnore() noexcept; - void _ActionParamIgnore() noexcept; void _ActionInterrupt(); void _EnterGround() noexcept; @@ -104,7 +103,6 @@ namespace Microsoft::Console::VirtualTerminal void _EnterEscapeIntermediate() noexcept; void _EnterCsiEntry(); void _EnterCsiParam() noexcept; - void _EnterCsiParamIgnore() noexcept; void _EnterCsiSubParam() noexcept; void _EnterCsiIgnore() noexcept; void _EnterCsiIntermediate() noexcept; @@ -127,7 +125,6 @@ namespace Microsoft::Console::VirtualTerminal void _EventCsiEntry(const wchar_t wch); void _EventCsiIntermediate(const wchar_t wch); void _EventCsiIgnore(const wchar_t wch); - void _EventCsiParamIgnore(const wchar_t wch); void _EventCsiParam(const wchar_t wch); void _EventCsiSubParam(const wchar_t wch); void _EventOscParam(const wchar_t wch) noexcept; @@ -144,7 +141,6 @@ namespace Microsoft::Console::VirtualTerminal void _EventSosPmApcString(const wchar_t wch) noexcept; void _AccumulateTo(const wchar_t wch, VTInt& value) noexcept; - bool _CanHandleSubParam() noexcept; template bool _SafeExecute(TLambda&& lambda); @@ -200,17 +196,9 @@ namespace Microsoft::Console::VirtualTerminal VTIDBuilder _identifier; std::vector _parameters; - bool _parameterLimitReached; + bool _parameterLimitOverflowed; std::vector _subParameters; std::vector> _subParameterRanges; - bool _subParameterLimitReached; - - // This flag is used to indicate that some parameters were omitted by the - // parser. This could happen if the parser was unable to parse all of the - // sub parameters for a given parameter. - // Sequences that are sensitive to parameter omission can use this flag - // to determine if any parameters were omitted. - bool _isParameterOmitted; std::wstring _oscString; VTInt _oscParameter; From a67ffbb0f4cb3ce5127fc44aabbbd123ab7ae81b Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 14:27:45 +0530 Subject: [PATCH 17/32] format --- src/terminal/parser/OutputStateMachineEngine.cpp | 5 +---- src/terminal/parser/stateMachine.cpp | 1 - 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/terminal/parser/OutputStateMachineEngine.cpp b/src/terminal/parser/OutputStateMachineEngine.cpp index cccaa1f1805..80a77d89173 100644 --- a/src/terminal/parser/OutputStateMachineEngine.cpp +++ b/src/terminal/parser/OutputStateMachineEngine.cpp @@ -1118,10 +1118,7 @@ bool OutputStateMachineEngine::_CanSeqAcceptSubParam(const VTID id, const VTPara return true; case DECCARA_ChangeAttributesRectangularArea: case DECRARA_ReverseAttributesRectangularArea: - return !parameters.hasSubParamsFor(0) - && !parameters.hasSubParamsFor(1) - && !parameters.hasSubParamsFor(2) - && !parameters.hasSubParamsFor(3); + return !parameters.hasSubParamsFor(0) && !parameters.hasSubParamsFor(1) && !parameters.hasSubParamsFor(2) && !parameters.hasSubParamsFor(3); default: return false; } diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 73212ee4df6..217434a13c2 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -559,7 +559,6 @@ void StateMachine::_ActionSubParam(const wchar_t wch) _parameters.push_back({}); const auto rangeStart = gsl::narrow(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); - } // On a delimiter, increase the number of sub params we've seen. From 0c86064a0d26f4e7276c9fcabfa6c11211391ce5 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 18:49:11 +0530 Subject: [PATCH 18/32] remove last few traces of CsiParamIgnore because we don't need it --- src/terminal/parser/stateMachine.cpp | 1 - src/terminal/parser/stateMachine.hpp | 1 - 2 files changed, 2 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 217434a13c2..2e47d3cf2d1 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -2170,7 +2170,6 @@ void StateMachine::ProcessString(const std::wstring_view string) case VTStates::CsiIgnore: case VTStates::CsiParam: case VTStates::CsiSubParam: - case VTStates::CsiParamIgnore: _ActionCsiDispatch(*wchIter); break; case VTStates::OscParam: diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 9498a1f5067..0c64e00fac7 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -157,7 +157,6 @@ namespace Microsoft::Console::VirtualTerminal CsiIgnore, CsiParam, CsiSubParam, - CsiParamIgnore, OscParam, OscString, OscTermination, From 0778eea2e87cbd2e30dc97432ec5c64fd7587ee9 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 18:56:15 +0530 Subject: [PATCH 19/32] remove few words, seemed unneccesary --- src/terminal/adapter/adaptDispatchGraphics.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index fa8bca3d7fd..c3555692ae8 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -269,7 +269,6 @@ void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */, // Routine Description: // - Helper to apply a number of graphic rendition options to an attribute. -// - Applies the changes iff all the options are valid. // Arguments: // - options - An array of options that will be applied in sequence. // - attr - The attribute that will be updated with the applied options. From 1d83b80eaf3ffafde29a1c67cda08624422d54e0 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 22:28:58 +0530 Subject: [PATCH 20/32] update TestCsiMaxSubParamCount --- .../parser/ut_parser/OutputEngineTest.cpp | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 6186f8df6e1..cebf7f2db14 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -465,6 +465,29 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final mach.ProcessCharacter(L'J'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + Log::Comment(L"Recieving 100 sub parameters should lead to removal of last parameter"); + VERIFY_IS_FALSE(mach._parameters.at(0).has_value()); + Log::Comment(L"Receiving 100 sub parameter should set the overflow flag"); + VERIFY_IS_TRUE(mach._parameterLimitOverflowed); + + Log::Comment(L"Output a sequence with MAX_SUBPARAMETER_COUNT(32) sub parameters"); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + mach.ProcessCharacter(AsciiChars::ESC); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + for (size_t i = 0; i < MAX_SUBPARAMETER_COUNT; i++) + { + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'0' + i % 10); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + mach.ProcessCharacter(L'J'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); + Log::Comment(L"Only MAX_SUBPARAMETER_COUNT (32) sub parameters should be stored"); VERIFY_ARE_EQUAL(mach._subParameters.size(), MAX_SUBPARAMETER_COUNT); for (size_t i = 0; i < MAX_SUBPARAMETER_COUNT; i++) From cc59dde2fc3ca3174b4040b18af82412131685c5 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Mon, 10 Jul 2023 22:59:18 +0530 Subject: [PATCH 21/32] fix typo in OutputEngineTest --- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index cebf7f2db14..f13a8d85317 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -465,7 +465,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final mach.ProcessCharacter(L'J'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - Log::Comment(L"Recieving 100 sub parameters should lead to removal of last parameter"); + Log::Comment(L"Receiving 100 sub parameters should lead to removal of last parameter"); VERIFY_IS_FALSE(mach._parameters.at(0).has_value()); Log::Comment(L"Receiving 100 sub parameter should set the overflow flag"); VERIFY_IS_TRUE(mach._parameterLimitOverflowed); From 95067c865881b4abbdb712929e58e3a4d20e16d3 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 11 Jul 2023 01:04:51 +0530 Subject: [PATCH 22/32] remove outdated test cases --- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index f13a8d85317..70649982881 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -466,7 +466,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); Log::Comment(L"Receiving 100 sub parameters should lead to removal of last parameter"); - VERIFY_IS_FALSE(mach._parameters.at(0).has_value()); + VERIFY_IS_TRUE(mach._parameters.empty()); Log::Comment(L"Receiving 100 sub parameter should set the overflow flag"); VERIFY_IS_TRUE(mach._parameterLimitOverflowed); @@ -533,18 +533,6 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final auto engine = std::make_unique(std::move(dispatch)); StateMachine mach(std::move(engine)); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - mach.ProcessCharacter(AsciiChars::ESC); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); - mach.ProcessCharacter(L'['); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); - mach.ProcessCharacter(L':'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiIgnore); - mach.ProcessCharacter(L'3'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiIgnore); - mach.ProcessCharacter(L'q'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - mach.ProcessCharacter(AsciiChars::ESC); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); mach.ProcessCharacter(L'['); From ecad2af486a60033284ceac6ae8093c5cb5661d8 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 11 Jul 2023 11:50:01 +0530 Subject: [PATCH 23/32] refactor _ApplyGraphicOption so DECRARA can support sub params --- .../adapter/adaptDispatchGraphics.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index c3555692ae8..cd05158fe99 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -64,6 +64,7 @@ size_t AdaptDispatch::_SetRgbColorsHelper(const VTParameters options, // Routine Description: // - Helper to apply a single graphic rendition option to an attribute. +// - Calls appropriate helper to apply the option with sub parameters when necessary. // Arguments: // - options - An array of options. // - optionIndex - The start index of the option that will be applied. @@ -75,6 +76,14 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options, TextAttribute& attr) noexcept { const GraphicsOptions opt = options.at(optionIndex); + + if (options.hasSubParamsFor(optionIndex)) + { + const auto subParams = options.subParamsFor(optionIndex); + _ApplyGraphicsOptionSubParam(opt, subParams, attr); + return 1; + } + switch (opt) { case Off: @@ -279,15 +288,7 @@ void AdaptDispatch::_ApplyGraphicsOptions(const VTParameters options, { for (size_t i = 0; i < options.size();) { - if (options.hasSubParamsFor(i)) - { - _ApplyGraphicsOptionSubParam(options.at(i), options.subParamsFor(i), attr); - i += 1; - } - else - { - i += _ApplyGraphicsOption(options, i, attr); - } + i += _ApplyGraphicsOption(options, i, attr); } } From 16cb42471b36542cb1a6ef869625747596fa4afc Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 11 Jul 2023 21:13:18 +0530 Subject: [PATCH 24/32] store upto MAX_SUBPARAMETER_COUNT sub params per parameter --- src/terminal/parser/stateMachine.cpp | 22 +++++++++++++--------- src/terminal/parser/stateMachine.hpp | 9 ++++++++- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index 2e47d3cf2d1..a7670462bec 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -19,6 +19,8 @@ StateMachine::StateMachine(std::unique_ptr engine, const bo _subParameters{}, _subParameterRanges{}, _parameterLimitOverflowed(false), + _subParameterLimitOverflowed(false), + _subParameterCounter(0), _oscString{}, _cachedSequence{ std::nullopt } { @@ -524,6 +526,7 @@ void StateMachine::_ActionParam(const wchar_t wch) { // Otherwise move to next param. _parameters.push_back({}); + _subParameterCounter = 0; const auto rangeStart = gsl::narrow(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); } @@ -550,8 +553,8 @@ void StateMachine::_ActionSubParam(const wchar_t wch) { _trace.TraceOnAction(L"SubParam"); - // Once we've reached the parameter limit, sub parameters are ignored. - if (!_parameterLimitOverflowed) + // Once we've reached the sub parameter limit, sub parameters are ignored. + if (!_subParameterLimitOverflowed) { // If we have no parameters and we're about to add a sub parameter, add an empty parameter here. if (_parameters.empty()) @@ -567,15 +570,12 @@ void StateMachine::_ActionSubParam(const wchar_t wch) if (_isSubParameterDelimiter(wch)) { // If we receive a delimiter after we've already accumulated the - // maximum allowed sub parameters, then we need to set a flag to - // indicate that further sub parameter characters should be ignored. - // Similarly, we need to remove the last parameter so it isn't interpreted - // by the engine. - if (_subParameters.size() >= MAX_SUBPARAMETER_COUNT) + // maximum allowed sub parameters for the parameter, then we need to + // set a flag to indicate that further sub parameter characters + // should be ignored. + if (_subParameterCounter >= MAX_SUBPARAMETER_COUNT) { _parameterLimitOverflowed = true; - _parameters.pop_back(); - _subParameterRanges.pop_back(); } else { @@ -583,6 +583,8 @@ void StateMachine::_ActionSubParam(const wchar_t wch) _subParameters.push_back({}); // increment current range's end index. _subParameterRanges.back().second++; + // increment counter + _subParameterCounter++; } } else @@ -614,6 +616,8 @@ void StateMachine::_ActionClear() _subParameters.clear(); _subParameterRanges.clear(); + _subParameterCounter = 0; + _subParameterLimitOverflowed = false; _oscString.clear(); _oscParameter = 0; diff --git a/src/terminal/parser/stateMachine.hpp b/src/terminal/parser/stateMachine.hpp index 0c64e00fac7..01976b21b23 100644 --- a/src/terminal/parser/stateMachine.hpp +++ b/src/terminal/parser/stateMachine.hpp @@ -30,7 +30,12 @@ namespace Microsoft::Console::VirtualTerminal // are supported, but most modern terminal emulators will allow around twice // that number. constexpr size_t MAX_PARAMETER_COUNT = 32; - constexpr size_t MAX_SUBPARAMETER_COUNT = 32; + + // Sub parameter limit for each parameter. + constexpr size_t MAX_SUBPARAMETER_COUNT = 6; + // we limit ourself to 256 sub parameters because we use bytes to store + // the their indexes. + static_assert(MAX_PARAMETER_COUNT * MAX_SUBPARAMETER_COUNT <= 256); class StateMachine final { @@ -198,6 +203,8 @@ namespace Microsoft::Console::VirtualTerminal bool _parameterLimitOverflowed; std::vector _subParameters; std::vector> _subParameterRanges; + bool _subParameterLimitOverflowed; + BYTE _subParameterCounter; std::wstring _oscString; VTInt _oscParameter; From 483ec2bdf8cc5be9892493af5b258fa4e4ecbc2a Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 11 Jul 2023 21:37:10 +0530 Subject: [PATCH 25/32] update test final --- .../parser/ut_parser/OutputEngineTest.cpp | 91 +++++++++---------- 1 file changed, 44 insertions(+), 47 deletions(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 70649982881..516067e8749 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -397,19 +397,22 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final auto engine = std::make_unique(std::move(dispatch)); StateMachine mach(std::move(engine)); + // "\e[:3;9:5::8J" VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); mach.ProcessCharacter(AsciiChars::ESC); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); mach.ProcessCharacter(L'['); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); mach.ProcessCharacter(L';'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); - mach.ProcessCharacter(L'3'); + mach.ProcessCharacter(L'9'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); mach.ProcessCharacter(L':'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); - mach.ProcessCharacter(L'3'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); mach.ProcessCharacter(L'5'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); mach.ProcessCharacter(L':'); @@ -423,16 +426,18 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._parameters.size(), 2u); VERIFY_IS_FALSE(mach._parameters.at(0).has_value()); - VERIFY_ARE_EQUAL(mach._parameters.at(1), 3); + VERIFY_ARE_EQUAL(mach._parameters.at(1), 9); - VERIFY_ARE_EQUAL(mach._subParameters.at(0), 35); - VERIFY_IS_FALSE(mach._subParameters.at(1).has_value()); - VERIFY_ARE_EQUAL(mach._subParameters.at(2), 8); + VERIFY_ARE_EQUAL(mach._subParameters.size(), 4u); + VERIFY_ARE_EQUAL(mach._subParameters.at(0), 3); + VERIFY_ARE_EQUAL(mach._subParameters.at(1), 5); + VERIFY_IS_FALSE(mach._subParameters.at(2).has_value()); + VERIFY_ARE_EQUAL(mach._subParameters.at(3), 8); VERIFY_ARE_EQUAL(mach._subParameterRanges.at(0).first, 0); - VERIFY_ARE_EQUAL(mach._subParameterRanges.at(0).second, 0); - VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).first, 0); - VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).second, 3); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(0).second, 1); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).first, 1); + VERIFY_ARE_EQUAL(mach._subParameterRanges.at(1).second, 4); VERIFY_ARE_EQUAL(mach._subParameterRanges.size(), mach._parameters.size()); VERIFY_IS_TRUE( @@ -447,54 +452,46 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final auto engine = std::make_unique(std::move(dispatch)); StateMachine mach(std::move(engine)); - Log::Comment(L"Output a sequence with 100 sub parameters"); + Log::Comment(L"Output two parameters with 100 sub parameters each"); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); mach.ProcessCharacter(AsciiChars::ESC); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); - mach.ProcessCharacter(L'['); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); - mach.ProcessCharacter(L'3'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); - for (size_t i = 0; i < 100; i++) + for (size_t nParam = 0; nParam < 2; nParam++) { - mach.ProcessCharacter(L':'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); - mach.ProcessCharacter(L'0' + i % 10); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); + mach.ProcessCharacter(L'3'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + for (size_t i = 0; i < 100; i++) + { + mach.ProcessCharacter(L':'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + mach.ProcessCharacter(L'0' + i % 10); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); + } + Log::Comment(L"Receiving 100 sub parameters should set the overflow flag"); + VERIFY_IS_TRUE(mach._subParameterLimitOverflowed); } mach.ProcessCharacter(L'J'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - Log::Comment(L"Receiving 100 sub parameters should lead to removal of last parameter"); - VERIFY_IS_TRUE(mach._parameters.empty()); - Log::Comment(L"Receiving 100 sub parameter should set the overflow flag"); - VERIFY_IS_TRUE(mach._parameterLimitOverflowed); + Log::Comment(L"Only MAX_SUBPARAMETER_COUNT (6) sub parameters should be stored for each parameter"); + VERIFY_ARE_EQUAL(mach._subParameters.size(), 12u); - Log::Comment(L"Output a sequence with MAX_SUBPARAMETER_COUNT(32) sub parameters"); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - mach.ProcessCharacter(AsciiChars::ESC); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); - mach.ProcessCharacter(L'['); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); - mach.ProcessCharacter(L'3'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); - for (size_t i = 0; i < MAX_SUBPARAMETER_COUNT; i++) + for (size_t nParam = 0; nParam < 2; nParam++) { - mach.ProcessCharacter(L':'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); - mach.ProcessCharacter(L'0' + i % 10); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiSubParam); - } - mach.ProcessCharacter(L'J'); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); - - Log::Comment(L"Only MAX_SUBPARAMETER_COUNT (32) sub parameters should be stored"); - VERIFY_ARE_EQUAL(mach._subParameters.size(), MAX_SUBPARAMETER_COUNT); - for (size_t i = 0; i < MAX_SUBPARAMETER_COUNT; i++) - { - VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); - VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 10)); + for (size_t i = nParam * MAX_SUBPARAMETER_COUNT; i < (nParam + 1) * MAX_SUBPARAMETER_COUNT; i++) + { + VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); + VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 10)); + } } + auto firstRange = mach._subParameterRanges.at(0); + auto secondRange = mach._subParameterRanges.at(1); + VERIFY_ARE_EQUAL(firstRange.first, 0); + VERIFY_ARE_EQUAL(firstRange.second, 6); + VERIFY_ARE_EQUAL(secondRange.first, 6); + VERIFY_ARE_EQUAL(secondRange.second, 12); } TEST_METHOD(TestLeadingZeroCsiSubParam) From f5177a0726f50b63657c841149ef16f52b1527fc Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Tue, 11 Jul 2023 21:53:04 +0530 Subject: [PATCH 26/32] use correct flag --- src/terminal/parser/stateMachine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index a7670462bec..e0294658371 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -575,7 +575,7 @@ void StateMachine::_ActionSubParam(const wchar_t wch) // should be ignored. if (_subParameterCounter >= MAX_SUBPARAMETER_COUNT) { - _parameterLimitOverflowed = true; + _subParameterLimitOverflowed = true; } else { From f135dfc2bbf6e982f196bad2f6f9b8097d61775d Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Wed, 12 Jul 2023 12:17:40 +0530 Subject: [PATCH 27/32] set flag once limit is overflowed --- src/terminal/parser/stateMachine.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index e0294658371..b84592abde1 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -527,6 +527,7 @@ void StateMachine::_ActionParam(const wchar_t wch) // Otherwise move to next param. _parameters.push_back({}); _subParameterCounter = 0; + _subParameterLimitOverflowed = false; const auto rangeStart = gsl::narrow(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); } From 9630376d173977cba0d3a277348e5c78842349da Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Wed, 12 Jul 2023 12:18:38 +0530 Subject: [PATCH 28/32] refactor test --- .../parser/ut_parser/OutputEngineTest.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 516067e8749..5b506e27cbe 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -456,10 +456,10 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); mach.ProcessCharacter(AsciiChars::ESC); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Escape); + mach.ProcessCharacter(L'['); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); for (size_t nParam = 0; nParam < 2; nParam++) { - mach.ProcessCharacter(L'['); - VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiEntry); mach.ProcessCharacter(L'3'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); for (size_t i = 0; i < 100; i++) @@ -471,6 +471,9 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final } Log::Comment(L"Receiving 100 sub parameters should set the overflow flag"); VERIFY_IS_TRUE(mach._subParameterLimitOverflowed); + mach.ProcessCharacter(L';'); + VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::CsiParam); + VERIFY_IS_FALSE(mach._subParameterLimitOverflowed); } mach.ProcessCharacter(L'J'); VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::Ground); @@ -478,14 +481,14 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final Log::Comment(L"Only MAX_SUBPARAMETER_COUNT (6) sub parameters should be stored for each parameter"); VERIFY_ARE_EQUAL(mach._subParameters.size(), 12u); - for (size_t nParam = 0; nParam < 2; nParam++) + // Verify that first 6 sub parameters are stored for each parameter. + // subParameters = { 0, 1, 2, 3, 4, 5, 0, 1, 2, 3, 4, 5 } + for (size_t i = 0; i < 12 ; i++) { - for (size_t i = nParam * MAX_SUBPARAMETER_COUNT; i < (nParam + 1) * MAX_SUBPARAMETER_COUNT; i++) - { - VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); - VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 10)); - } + VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); + VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 6)); } + auto firstRange = mach._subParameterRanges.at(0); auto secondRange = mach._subParameterRanges.at(1); VERIFY_ARE_EQUAL(firstRange.first, 0); From 86f128dbac8779cd1bbd0cbba77f87157e02eb61 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Wed, 12 Jul 2023 12:21:25 +0530 Subject: [PATCH 29/32] add VTSubParameters class --- src/terminal/adapter/DispatchTypes.hpp | 50 +++++++++++++++++-- src/terminal/adapter/adaptDispatch.hpp | 2 +- .../adapter/adaptDispatchGraphics.cpp | 2 +- 3 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 8a199a19673..088714e5cf9 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -152,6 +152,48 @@ namespace Microsoft::Console::VirtualTerminal VTInt _value; }; + class VTSubParameters + { + public: + constexpr VTSubParameters() noexcept + { + } + + constexpr VTSubParameters(const std::span subParams) noexcept : + _subParams{ subParams } + { + } + + constexpr VTParameter at(const size_t index) const noexcept + { + return til::at(_subParams, index); + } + + VTSubParameters subspan(const size_t offset, const size_t count) const noexcept + { + const auto subParamsSpan = _subParams.subspan(offset, count); + return { subParamsSpan }; + } + + bool empty() const noexcept + { + return _subParams.empty(); + } + + size_t size() const noexcept + { + return _subParams.size(); + } + + constexpr operator std::span() const noexcept + { + return _subParams; + } + + private: + std::span _subParams; + }; + class VTParameters { public: @@ -200,10 +242,10 @@ namespace Microsoft::Console::VirtualTerminal // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); - return { newParamsSpan, _subParams, newSubParamRangesSpan }; + return { newParamsSpan, _subParams , newSubParamRangesSpan }; } - std::span subParamsFor(const size_t index) const noexcept + VTSubParameters subParamsFor(const size_t index) const noexcept { if (index < _subParamRanges.size()) { @@ -212,7 +254,7 @@ namespace Microsoft::Console::VirtualTerminal } else { - return std::span{}; + return VTSubParameters{}; } } @@ -259,7 +301,7 @@ namespace Microsoft::Console::VirtualTerminal static constexpr std::span defaultParameters{ &defaultParameter, 1 }; std::span _params; - std::span _subParams; + VTSubParameters _subParams; std::span> _subParamRanges; }; diff --git a/src/terminal/adapter/adaptDispatch.hpp b/src/terminal/adapter/adaptDispatch.hpp index 059fb3852c5..e0c346dc9d0 100644 --- a/src/terminal/adapter/adaptDispatch.hpp +++ b/src/terminal/adapter/adaptDispatch.hpp @@ -301,7 +301,7 @@ namespace Microsoft::Console::VirtualTerminal const size_t optionIndex, TextAttribute& attr) noexcept; void _ApplyGraphicsOptionSubParam(const VTParameter option, - const std::span subParams, + const VTSubParameters subParams, TextAttribute& attr) noexcept; void _ApplyGraphicsOptions(const VTParameters options, TextAttribute& attr) noexcept; diff --git a/src/terminal/adapter/adaptDispatchGraphics.cpp b/src/terminal/adapter/adaptDispatchGraphics.cpp index cd05158fe99..5259cf8ce64 100644 --- a/src/terminal/adapter/adaptDispatchGraphics.cpp +++ b/src/terminal/adapter/adaptDispatchGraphics.cpp @@ -268,7 +268,7 @@ size_t AdaptDispatch::_ApplyGraphicsOption(const VTParameters options, // Return Value: // - void AdaptDispatch::_ApplyGraphicsOptionSubParam(const VTParameter /* option */, - const std::span /* subParam */, + const VTSubParameters /* subParam */, TextAttribute& /* attr */) noexcept { // here, we apply our "best effort" rule, while handling sub params if we don't From 2fdc975525d4808695e1c4d7c1c3ac43ab96fb67 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Wed, 12 Jul 2023 12:22:51 +0530 Subject: [PATCH 30/32] format --- src/terminal/adapter/DispatchTypes.hpp | 4 ++-- src/terminal/parser/ut_parser/OutputEngineTest.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index 088714e5cf9..a9e7cf7dfbe 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -158,7 +158,7 @@ namespace Microsoft::Console::VirtualTerminal constexpr VTSubParameters() noexcept { } - + constexpr VTSubParameters(const std::span subParams) noexcept : _subParams{ subParams } { @@ -242,7 +242,7 @@ namespace Microsoft::Console::VirtualTerminal // _subParams as is and create new span for others. const auto newParamsSpan = _params.subspan(std::min(offset, _params.size())); const auto newSubParamRangesSpan = _subParamRanges.subspan(std::min(offset, _subParamRanges.size())); - return { newParamsSpan, _subParams , newSubParamRangesSpan }; + return { newParamsSpan, _subParams, newSubParamRangesSpan }; } VTSubParameters subParamsFor(const size_t index) const noexcept diff --git a/src/terminal/parser/ut_parser/OutputEngineTest.cpp b/src/terminal/parser/ut_parser/OutputEngineTest.cpp index 5b506e27cbe..9695c80936f 100644 --- a/src/terminal/parser/ut_parser/OutputEngineTest.cpp +++ b/src/terminal/parser/ut_parser/OutputEngineTest.cpp @@ -483,7 +483,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final // Verify that first 6 sub parameters are stored for each parameter. // subParameters = { 0, 1, 2, 3, 4, 5, 0, 1, 2, 3, 4, 5 } - for (size_t i = 0; i < 12 ; i++) + for (size_t i = 0; i < 12; i++) { VERIFY_IS_TRUE(mach._subParameters.at(i).has_value()); VERIFY_ARE_EQUAL(mach._subParameters.at(i).value(), gsl::narrow_cast(i % 6)); From edecf0a9afd267f2b5502a09a80f0a2bbfb36ef9 Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Thu, 13 Jul 2023 21:53:39 +0530 Subject: [PATCH 31/32] remove unused GraphicOptionsWithSubParams --- src/terminal/adapter/DispatchTypes.hpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/terminal/adapter/DispatchTypes.hpp b/src/terminal/adapter/DispatchTypes.hpp index a9e7cf7dfbe..81864c0c503 100644 --- a/src/terminal/adapter/DispatchTypes.hpp +++ b/src/terminal/adapter/DispatchTypes.hpp @@ -449,15 +449,6 @@ namespace Microsoft::Console::VirtualTerminal::DispatchTypes BrightBackgroundWhite = 107, }; - // we use this in VT parser to indicate that the GraphicsOptions (parameter) - // accepts sub-parameters. - enum GraphicsOptionsWithSubParams : VTInt - { -#ifdef UNIT_TESTING - TestOption = 12345, -#endif - }; - enum LogicalAttributeOptions : VTInt { Default = 0, From adaa8414871a47d347d55370ed7f5567feb5bc7c Mon Sep 17 00:00:00 2001 From: Tushar Singh Date: Thu, 13 Jul 2023 21:54:37 +0530 Subject: [PATCH 32/32] use narrow_cast --- src/terminal/parser/stateMachine.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/terminal/parser/stateMachine.cpp b/src/terminal/parser/stateMachine.cpp index b84592abde1..eb22ff95382 100644 --- a/src/terminal/parser/stateMachine.cpp +++ b/src/terminal/parser/stateMachine.cpp @@ -506,7 +506,7 @@ void StateMachine::_ActionParam(const wchar_t wch) if (_parameters.empty()) { _parameters.push_back({}); - const auto rangeStart = gsl::narrow(_subParameters.size()); + const auto rangeStart = gsl::narrow_cast(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); } @@ -528,7 +528,7 @@ void StateMachine::_ActionParam(const wchar_t wch) _parameters.push_back({}); _subParameterCounter = 0; _subParameterLimitOverflowed = false; - const auto rangeStart = gsl::narrow(_subParameters.size()); + const auto rangeStart = gsl::narrow_cast(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); } } @@ -561,7 +561,7 @@ void StateMachine::_ActionSubParam(const wchar_t wch) if (_parameters.empty()) { _parameters.push_back({}); - const auto rangeStart = gsl::narrow(_subParameters.size()); + const auto rangeStart = gsl::narrow_cast(_subParameters.size()); _subParameterRanges.push_back({ rangeStart, rangeStart }); }