diff --git a/src/buffer/out/AttrRowIterator.cpp b/src/buffer/out/AttrRowIterator.cpp index c4e735d3e11..590e8c93db0 100644 --- a/src/buffer/out/AttrRowIterator.cpp +++ b/src/buffer/out/AttrRowIterator.cpp @@ -16,20 +16,22 @@ AttrRowIterator AttrRowIterator::CreateEndIterator(const ATTR_ROW* const attrRow AttrRowIterator::AttrRowIterator(const ATTR_ROW* const attrRow) noexcept : _pAttrRow{ attrRow }, _run{ attrRow->_list.cbegin() }, - _currentAttributeIndex{ 0 } + _currentAttributeIndex{ 0 }, + _exceeded{ false } { } AttrRowIterator::operator bool() const { - return _run < _pAttrRow->_list.cend(); + return !_exceeded && _run < _pAttrRow->_list.cend(); } bool AttrRowIterator::operator==(const AttrRowIterator& it) const { return (_pAttrRow == it._pAttrRow && _run == it._run && - _currentAttributeIndex == it._currentAttributeIndex); + _currentAttributeIndex == it._currentAttributeIndex && + _exceeded == it._exceeded); } bool AttrRowIterator::operator!=(const AttrRowIterator& it) const @@ -52,13 +54,16 @@ AttrRowIterator AttrRowIterator::operator++(int) AttrRowIterator& AttrRowIterator::operator+=(const ptrdiff_t& movement) { - if (movement >= 0) + if (!_exceeded) { - _increment(gsl::narrow(movement)); - } - else - { - _decrement(gsl::narrow(-movement)); + if (movement >= 0) + { + _increment(gsl::narrow(movement)); + } + else + { + _decrement(gsl::narrow(-movement)); + } } return *this; @@ -84,11 +89,13 @@ AttrRowIterator AttrRowIterator::operator--(int) const TextAttribute* AttrRowIterator::operator->() const { + THROW_HR_IF(E_BOUNDS, _exceeded); return &_run->GetAttributes(); } const TextAttribute& AttrRowIterator::operator*() const { + THROW_HR_IF(E_BOUNDS, _exceeded); return _run->GetAttributes(); } @@ -123,14 +130,23 @@ void AttrRowIterator::_decrement(size_t count) { while (count > 0) { + // If there's still space within this color attribute to move left, do so. if (count <= _currentAttributeIndex) { _currentAttributeIndex -= count; return; } + // If there's not space, move to the previous attribute run + // We'll walk through above on the if branch to move left further (if necessary) else { - count -= _currentAttributeIndex; + // make sure we don't go out of bounds + if (_run == _pAttrRow->_list.cbegin()) + { + _exceeded = true; + return; + } + count -= _currentAttributeIndex + 1; --_run; _currentAttributeIndex = _run->GetLength() - 1; } diff --git a/src/buffer/out/AttrRowIterator.hpp b/src/buffer/out/AttrRowIterator.hpp index c9e053ea247..3dbd4fa6926 100644 --- a/src/buffer/out/AttrRowIterator.hpp +++ b/src/buffer/out/AttrRowIterator.hpp @@ -54,6 +54,7 @@ class AttrRowIterator final std::vector::const_iterator _run; const ATTR_ROW* _pAttrRow; size_t _currentAttributeIndex; // index of TextAttribute within the current TextAttributeRun + bool _exceeded; void _increment(size_t count); void _decrement(size_t count); diff --git a/src/cascadia/TerminalCore/TerminalSelection.cpp b/src/cascadia/TerminalCore/TerminalSelection.cpp index 4ff0efa7529..fc6fa53602b 100644 --- a/src/cascadia/TerminalCore/TerminalSelection.cpp +++ b/src/cascadia/TerminalCore/TerminalSelection.cpp @@ -377,7 +377,7 @@ COORD Terminal::_ExpandDoubleClickSelectionLeft(const COORD position) const while (positionWithOffsets.X > bufferViewport.Left() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter)) { bufferViewport.DecrementInBounds(positionWithOffsets); - bufferIterator--; + --bufferIterator; } if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter) @@ -415,7 +415,7 @@ COORD Terminal::_ExpandDoubleClickSelectionRight(const COORD position) const while (positionWithOffsets.X < bufferViewport.RightInclusive() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter)) { bufferViewport.IncrementInBounds(positionWithOffsets); - bufferIterator++; + ++bufferIterator; } if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter) diff --git a/src/host/ut_host/AttrRowTests.cpp b/src/host/ut_host/AttrRowTests.cpp index 78366327a79..f87345ecc21 100644 --- a/src/host/ut_host/AttrRowTests.cpp +++ b/src/host/ut_host/AttrRowTests.cpp @@ -498,6 +498,135 @@ class AttrRowTests } } + TEST_METHOD(TestReverseIteratorWalkFromMiddle) + { + // GH #3409, walking backwards through color range runs out of bounds + // We're going to create an attribute row with assorted colors and varying lengths + // just like the row of text on the Ubuntu prompt line that triggered this bug being found. + // Then we're going to walk backwards through the iterator like a selection-expand-to-left + // operation and ensure we don't run off the bounds. + + // walk the chain, from index, stepSize at a time + // ensure we don't crash + auto testWalk = [](ATTR_ROW* chain, size_t index, int stepSize) { + // move to starting index + auto iter = chain->cbegin(); + iter += index; + + // Now walk backwards in a loop until 0. + while (iter) + { + iter -= stepSize; + } + + Log::Comment(L"We made it through without crashing!"); + }; + + // take one step of size stepSize on the chain + // index is where we start from + // expectedAttribute is what we expect to read here + auto verifyStep = [](ATTR_ROW* chain, size_t index, int stepSize, TextAttribute expectedAttribute) { + // move to starting index + auto iter = chain->cbegin(); + iter += index; + + // Now step backwards + iter -= stepSize; + + VERIFY_ARE_EQUAL(expectedAttribute, *iter); + }; + + Log::Comment(L"Reverse iterate through ubuntu prompt"); + { + // Create attr row representing a buffer that's 121 wide. + auto chain = std::make_unique(121, _DefaultAttr); + + // The repro case had 4 chain segments. + chain->_list.resize(4); + + // The color 10 went for the first 18. + chain->_list[0].SetAttributes(TextAttribute(0xA)); + chain->_list[0].SetLength(18); + + // Default color for the next 1 + chain->_list[1].SetAttributes(TextAttribute()); + chain->_list[1].SetLength(1); + + // Color 12 for the next 29 + chain->_list[2].SetAttributes(TextAttribute(0xC)); + chain->_list[2].SetLength(29); + + // Then default color to end the run + chain->_list[3].SetAttributes(TextAttribute()); + chain->_list[3].SetLength(73); + + // The sum of the lengths should be 121. + VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength + chain->_list[3]._cchLength); + + auto index = chain->_list[0].GetLength(); + auto stepSize = 1; + testWalk(chain.get(), index, stepSize); + } + + Log::Comment(L"Reverse iterate across a text run in the chain"); + { + // Create attr row representing a buffer that's 3 wide. + auto chain = std::make_unique(3, _DefaultAttr); + + // The repro case had 3 chain segments. + chain->_list.resize(3); + + // The color 10 went for the first 1. + chain->_list[0].SetAttributes(TextAttribute(0xA)); + chain->_list[0].SetLength(1); + + // The color 11 for the next 1 + chain->_list[1].SetAttributes(TextAttribute(0xB)); + chain->_list[1].SetLength(1); + + // Color 12 for the next 1 + chain->_list[2].SetAttributes(TextAttribute(0xC)); + chain->_list[2].SetLength(1); + + // The sum of the lengths should be 3. + VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength); + + // on 'ABC', step from B to A + auto index = 1; + auto stepSize = 1; + verifyStep(chain.get(), index, stepSize, TextAttribute(0xA)); + } + + Log::Comment(L"Reverse iterate across two text runs in the chain"); + { + // Create attr row representing a buffer that's 3 wide. + auto chain = std::make_unique(3, _DefaultAttr); + + // The repro case had 3 chain segments. + chain->_list.resize(3); + + // The color 10 went for the first 1. + chain->_list[0].SetAttributes(TextAttribute(0xA)); + chain->_list[0].SetLength(1); + + // The color 11 for the next 1 + chain->_list[1].SetAttributes(TextAttribute(0xB)); + chain->_list[1].SetLength(1); + + // Color 12 for the next 1 + chain->_list[2].SetAttributes(TextAttribute(0xC)); + chain->_list[2].SetLength(1); + + // The sum of the lengths should be 3. + VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength); + + // on 'ABC', step from C to A + auto index = 2; + auto stepSize = 2; + verifyStep(chain.get(), index, stepSize, TextAttribute(0xA)); + } + } + TEST_METHOD(TestSetAttrToEnd) { const WORD wTestAttr = FOREGROUND_BLUE | BACKGROUND_GREEN; diff --git a/tools/ConsoleTypes.natvis b/tools/ConsoleTypes.natvis index dab839ec6b1..1fbab1b1ce6 100644 --- a/tools/ConsoleTypes.natvis +++ b/tools/ConsoleTypes.natvis @@ -12,16 +12,19 @@ - {{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Bold}} - {{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Normal}} + {{FG: {_foreground}, BG: {_background}, Legacy: {_wAttrLegacy}, {_extendedAttrs}} - _wAttrLegacy - _isBold - _foreground - _background + _wAttrLegacy + _foreground + _background + _extendedAttrs + + Length={_cchLength} Attr={_attributes} + + {{LT({_sr.Left}, {_sr.Top}) RB({_sr.Right}, {_sr.Bottom}) [{_sr.Right-_sr.Left+1} x { _sr.Bottom-_sr.Top+1}]}}