Skip to content

Commit

Permalink
Clamp parameter values to a maximum of 32767. (#5200)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

This PR clamps the parameter values in the VT `StateMachine` parser to 32767, which was the initial limit prior to PR #3956. This fixes a number of overflow bugs (some of which could cause the app to crash), since much of the code is not prepared to handle values outside the range of a `short`.

## References

#3956 - the PR where the cap was changed to the range of `size_t`
#4254 - one example of a crash caused by the higher range

## PR Checklist
* [x] Closes #5160
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [x] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

The DEC STD 070 reference recommends supporting up to at least 16384 for parameter values, so 32767 should be more than enough for any standard VT sequence. It might be nice to increase the limit to 65535 at some point, since that is the cap used by both XTerm and VTE. However, that is not essential, since there are very few situations where you'd even notice the difference. For now, 32767 is the safest choice for us, since anything greater than that has the potential to overflow and crash the app in a number of places.

## Validation Steps Performed

I had to make a couple of modifications to the range checks in the `OutputEngineTest`, more or less reverting to the pre-#3956 behavior, but after that all of the unit tests passed as expected.

I manually confirmed that this fixes the hanging test case from #5160, as well as overflow issues in the cursor operations, and crashes in `IL` and `DL` (see microsoft/terminal#4254 (comment)).
  • Loading branch information
donno2048 committed Apr 1, 2020
1 parent 7190f02 commit fed20c8
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 15 deletions.
17 changes: 9 additions & 8 deletions src/terminal/parser/stateMachine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ void StateMachine::_ActionIgnore() noexcept
// - wch - Character to collect.
// Return Value:
// - <none>
void StateMachine::_ActionOscParam(const wchar_t wch)
void StateMachine::_ActionOscParam(const wchar_t wch) noexcept
{
_trace.TraceOnAction(L"OscParamCollect");

Expand Down Expand Up @@ -1002,7 +1002,7 @@ void StateMachine::_EventCsiParam(const wchar_t wch)
// - wch - Character that triggered the event
// Return Value:
// - <none>
void StateMachine::_EventOscParam(const wchar_t wch)
void StateMachine::_EventOscParam(const wchar_t wch) noexcept
{
_trace.TraceOnEvent(L"OscParam");
if (_isOscTerminator(wch))
Expand Down Expand Up @@ -1416,20 +1416,21 @@ void StateMachine::ResetState() noexcept
// into the given size_t. All existing value is moved up by 10.
// - For example, if your value had 437 and you put in the printable number 2,
// this function will update value to 4372.
// - Clamps to size_t max if it gets too big.
// - Clamps to 32767 if it gets too big.
// Arguments:
// - wch - Printable character to accumulate into the value (after conversion to number, of course)
// - value - The value to update with the printable character. See example above.
// Return Value:
// - <none> - But really it's the update to the given value parameter.
void StateMachine::_AccumulateTo(const wchar_t wch, size_t& value)
void StateMachine::_AccumulateTo(const wchar_t wch, size_t& value) noexcept
{
const size_t digit = wch - L'0';

// If we overflow while multiplying and adding, the value is just size_t max.
if (FAILED(SizeTMult(value, 10, &value)) ||
FAILED(SizeTAdd(value, digit, &value)))
value = value * 10 + digit;

// Values larger than the maximum should be mapped to the largest supported value.
if (value > MAX_PARAMETER_VALUE)
{
value = std::numeric_limits<size_t>().max();
value = MAX_PARAMETER_VALUE;
}
}
12 changes: 9 additions & 3 deletions src/terminal/parser/stateMachine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ Module Name:

namespace Microsoft::Console::VirtualTerminal
{
// The DEC STD 070 reference recommends supporting up to at least 16384 for
// parameter values, so 32767 should be more than enough. At most we might
// want to increase this to 65535, since that is what XTerm and VTE support,
// but for now 32767 is the safest limit for our existing code base.
constexpr size_t MAX_PARAMETER_VALUE = 32767;

class StateMachine final
{
#ifdef UNIT_TESTING
Expand Down Expand Up @@ -49,7 +55,7 @@ namespace Microsoft::Console::VirtualTerminal
void _ActionCollect(const wchar_t wch);
void _ActionParam(const wchar_t wch);
void _ActionCsiDispatch(const wchar_t wch);
void _ActionOscParam(const wchar_t wch);
void _ActionOscParam(const wchar_t wch) noexcept;
void _ActionOscPut(const wchar_t wch);
void _ActionOscDispatch(const wchar_t wch);
void _ActionSs3Dispatch(const wchar_t wch);
Expand Down Expand Up @@ -77,13 +83,13 @@ namespace Microsoft::Console::VirtualTerminal
void _EventCsiIntermediate(const wchar_t wch);
void _EventCsiIgnore(const wchar_t wch);
void _EventCsiParam(const wchar_t wch);
void _EventOscParam(const wchar_t wch);
void _EventOscParam(const wchar_t wch) noexcept;
void _EventOscString(const wchar_t wch);
void _EventOscTermination(const wchar_t wch);
void _EventSs3Entry(const wchar_t wch);
void _EventSs3Param(const wchar_t wch);

void _AccumulateTo(const wchar_t wch, size_t& value);
void _AccumulateTo(const wchar_t wch, size_t& value) noexcept;

enum class VTStates
{
Expand Down
13 changes: 9 additions & 4 deletions src/terminal/parser/ut_parser/OutputEngineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final
mach.ProcessCharacter(wch);
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscParam);
}
VERIFY_ARE_EQUAL(mach._oscParameter, sizeMax);
VERIFY_ARE_EQUAL(mach._oscParameter, MAX_PARAMETER_VALUE);
mach.ProcessCharacter(L';');
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscString);
mach.ProcessCharacter(L's');
Expand All @@ -542,7 +542,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final
mach.ProcessCharacter(wch);
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscParam);
}
VERIFY_ARE_EQUAL(mach._oscParameter, sizeMax);
VERIFY_ARE_EQUAL(mach._oscParameter, MAX_PARAMETER_VALUE);
mach.ProcessCharacter(L';');
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscString);
mach.ProcessCharacter(L's');
Expand Down Expand Up @@ -1064,15 +1064,20 @@ class StateMachineExternalTest final
void ApplyParameterBoundary(size_t* uiExpected, size_t uiGiven)
{
// 0 and 1 should be 1. Use the preset value.
// 2019-12: No longer bound by SHORT_MAX. Goes all the way to size_t.
// 1-MAX_PARAMETER_VALUE should be what we set.
// > MAX_PARAMETER_VALUE should be MAX_PARAMETER_VALUE.
if (uiGiven <= 1)
{
*uiExpected = 1u;
}
else if (uiGiven > 1)
else if (uiGiven > 1 && uiGiven <= MAX_PARAMETER_VALUE)
{
*uiExpected = uiGiven;
}
else if (uiGiven > MAX_PARAMETER_VALUE)
{
*uiExpected = MAX_PARAMETER_VALUE; // 32767 is our max value.
}
}

void TestCsiCursorMovement(wchar_t const wchCommand,
Expand Down

0 comments on commit fed20c8

Please sign in to comment.