maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix Reverse Walking in AttrRowIterator (#3566)

Browse source
This commit is contained in:
Carlos Zamora 2019-12-10 14:46:08 -08:00 committed by GitHub
parent 402b7ff0e0
commit 5bbf7e2650
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 167 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
src/buffer/out/AttrRowIterator.cpp
View file

@ -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<size_t>(movement));
}
else
{
_decrement(gsl::narrow<size_t>(-movement));
if (movement >= 0)
{
_increment(gsl::narrow<size_t>(movement));
}
else
{
_decrement(gsl::narrow<size_t>(-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;
}

1
src/buffer/out/AttrRowIterator.hpp
View file

@ -54,6 +54,7 @@ private:
std::vector<TextAttributeRun>::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);

4
src/cascadia/TerminalCore/TerminalSelection.cpp
View file

@ -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)

129
src/host/ut_host/AttrRowTests.cpp
View file

@ -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<ATTR_ROW>(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<ATTR_ROW>(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<ATTR_ROW>(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;

15
tools/ConsoleTypes.natvis
View file

@ -12,16 +12,19 @@
<!-- You can't do too much trickyness inside the DisplayString format
string, so we'd have to add entries for each flag if we really
wanted them to show up like that. -->
<DisplayString Condition="_isBold">{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Bold}}</DisplayString>
<DisplayString>{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Normal}}</DisplayString>
<DisplayString>{{FG: {_foreground}, BG: {_background}, Legacy: {_wAttrLegacy}, {_extendedAttrs}}</DisplayString>
<Expand>
<Item Name="_wAttrLegacy">_wAttrLegacy</Item>
<Item Name="_isBold">_isBold</Item>
<Item Name="_foreground">_foreground</Item>
<Item Name="_background">_background</Item>
<Item Name="Legacy">_wAttrLegacy</Item>
<Item Name="FG">_foreground</Item>
<Item Name="BG">_background</Item>
<Item Name="Extended">_extendedAttrs</Item>
</Expand>
</Type>
<Type Name="TextAttributeRun">
<DisplayString>Length={_cchLength} Attr={_attributes}</DisplayString>
</Type>
<Type Name="Microsoft::Console::Types::Viewport">
<!-- Can't call functions in here -->
<DisplayString>{{LT({_sr.Left}, {_sr.Top}) RB({_sr.Right}, {_sr.Bottom}) [{_sr.Right-_sr.Left+1} x { _sr.Bottom-_sr.Top+1}]}}</DisplayString>