Skip to content

Commit 8585bc6

Browse files
authored
Clamp parameter values to a maximum of 32767. (#5200)
## 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 #4254 (comment)).
1 parent 9a0b6e3 commit 8585bc6

File tree

3 files changed

+27
-15
lines changed

3 files changed

+27
-15
lines changed

src/terminal/parser/stateMachine.cpp

+9-8
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ void StateMachine::_ActionIgnore() noexcept
456456
// - wch - Character to collect.
457457
// Return Value:
458458
// - <none>
459-
void StateMachine::_ActionOscParam(const wchar_t wch)
459+
void StateMachine::_ActionOscParam(const wchar_t wch) noexcept
460460
{
461461
_trace.TraceOnAction(L"OscParamCollect");
462462

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

1429-
// If we overflow while multiplying and adding, the value is just size_t max.
1430-
if (FAILED(SizeTMult(value, 10, &value)) ||
1431-
FAILED(SizeTAdd(value, digit, &value)))
1429+
value = value * 10 + digit;
1430+
1431+
// Values larger than the maximum should be mapped to the largest supported value.
1432+
if (value > MAX_PARAMETER_VALUE)
14321433
{
1433-
value = std::numeric_limits<size_t>().max();
1434+
value = MAX_PARAMETER_VALUE;
14341435
}
14351436
}

src/terminal/parser/stateMachine.hpp

+9-3
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ Module Name:
2121

2222
namespace Microsoft::Console::VirtualTerminal
2323
{
24+
// The DEC STD 070 reference recommends supporting up to at least 16384 for
25+
// parameter values, so 32767 should be more than enough. At most we might
26+
// want to increase this to 65535, since that is what XTerm and VTE support,
27+
// but for now 32767 is the safest limit for our existing code base.
28+
constexpr size_t MAX_PARAMETER_VALUE = 32767;
29+
2430
class StateMachine final
2531
{
2632
#ifdef UNIT_TESTING
@@ -49,7 +55,7 @@ namespace Microsoft::Console::VirtualTerminal
4955
void _ActionCollect(const wchar_t wch);
5056
void _ActionParam(const wchar_t wch);
5157
void _ActionCsiDispatch(const wchar_t wch);
52-
void _ActionOscParam(const wchar_t wch);
58+
void _ActionOscParam(const wchar_t wch) noexcept;
5359
void _ActionOscPut(const wchar_t wch);
5460
void _ActionOscDispatch(const wchar_t wch);
5561
void _ActionSs3Dispatch(const wchar_t wch);
@@ -77,13 +83,13 @@ namespace Microsoft::Console::VirtualTerminal
7783
void _EventCsiIntermediate(const wchar_t wch);
7884
void _EventCsiIgnore(const wchar_t wch);
7985
void _EventCsiParam(const wchar_t wch);
80-
void _EventOscParam(const wchar_t wch);
86+
void _EventOscParam(const wchar_t wch) noexcept;
8187
void _EventOscString(const wchar_t wch);
8288
void _EventOscTermination(const wchar_t wch);
8389
void _EventSs3Entry(const wchar_t wch);
8490
void _EventSs3Param(const wchar_t wch);
8591

86-
void _AccumulateTo(const wchar_t wch, size_t& value);
92+
void _AccumulateTo(const wchar_t wch, size_t& value) noexcept;
8793

8894
enum class VTStates
8995
{

src/terminal/parser/ut_parser/OutputEngineTest.cpp

+9-4
Original file line numberDiff line numberDiff line change
@@ -525,7 +525,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final
525525
mach.ProcessCharacter(wch);
526526
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscParam);
527527
}
528-
VERIFY_ARE_EQUAL(mach._oscParameter, sizeMax);
528+
VERIFY_ARE_EQUAL(mach._oscParameter, MAX_PARAMETER_VALUE);
529529
mach.ProcessCharacter(L';');
530530
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscString);
531531
mach.ProcessCharacter(L's');
@@ -542,7 +542,7 @@ class Microsoft::Console::VirtualTerminal::OutputEngineTest final
542542
mach.ProcessCharacter(wch);
543543
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscParam);
544544
}
545-
VERIFY_ARE_EQUAL(mach._oscParameter, sizeMax);
545+
VERIFY_ARE_EQUAL(mach._oscParameter, MAX_PARAMETER_VALUE);
546546
mach.ProcessCharacter(L';');
547547
VERIFY_ARE_EQUAL(mach._state, StateMachine::VTStates::OscString);
548548
mach.ProcessCharacter(L's');
@@ -1064,15 +1064,20 @@ class StateMachineExternalTest final
10641064
void ApplyParameterBoundary(size_t* uiExpected, size_t uiGiven)
10651065
{
10661066
// 0 and 1 should be 1. Use the preset value.
1067-
// 2019-12: No longer bound by SHORT_MAX. Goes all the way to size_t.
1067+
// 1-MAX_PARAMETER_VALUE should be what we set.
1068+
// > MAX_PARAMETER_VALUE should be MAX_PARAMETER_VALUE.
10681069
if (uiGiven <= 1)
10691070
{
10701071
*uiExpected = 1u;
10711072
}
1072-
else if (uiGiven > 1)
1073+
else if (uiGiven > 1 && uiGiven <= MAX_PARAMETER_VALUE)
10731074
{
10741075
*uiExpected = uiGiven;
10751076
}
1077+
else if (uiGiven > MAX_PARAMETER_VALUE)
1078+
{
1079+
*uiExpected = MAX_PARAMETER_VALUE; // 32767 is our max value.
1080+
}
10761081
}
10771082

10781083
void TestCsiCursorMovement(wchar_t const wchCommand,

0 commit comments

Comments
 (0)