Skip to content

Commit e50eba0

Browse files
carlos-zamoraDHowett
carlos-zamora
authored and
DHowett
committed
Fix Reverse Walking in AttrRowIterator (#3566)
(cherry picked from commit 5bbf7e2)
1 parent af1d06a commit e50eba0

File tree

5 files changed

+167
-18
lines changed

5 files changed

+167
-18
lines changed

‎src/buffer/out/AttrRowIterator.cpp

+26-10
Original file line numberDiff line numberDiff line change
@@ -16,20 +16,22 @@ AttrRowIterator AttrRowIterator::CreateEndIterator(const ATTR_ROW* const attrRow
1616
AttrRowIterator::AttrRowIterator(const ATTR_ROW* const attrRow) noexcept :
1717
_pAttrRow{ attrRow },
1818
_run{ attrRow->_list.cbegin() },
19-
_currentAttributeIndex{ 0 }
19+
_currentAttributeIndex{ 0 },
20+
_exceeded{ false }
2021
{
2122
}
2223

2324
AttrRowIterator::operator bool() const
2425
{
25-
return _run < _pAttrRow->_list.cend();
26+
return !_exceeded && _run < _pAttrRow->_list.cend();
2627
}
2728

2829
bool AttrRowIterator::operator==(const AttrRowIterator& it) const
2930
{
3031
return (_pAttrRow == it._pAttrRow &&
3132
_run == it._run &&
32-
_currentAttributeIndex == it._currentAttributeIndex);
33+
_currentAttributeIndex == it._currentAttributeIndex &&
34+
_exceeded == it._exceeded);
3335
}
3436

3537
bool AttrRowIterator::operator!=(const AttrRowIterator& it) const
@@ -52,13 +54,16 @@ AttrRowIterator AttrRowIterator::operator++(int)
5254

5355
AttrRowIterator& AttrRowIterator::operator+=(const ptrdiff_t& movement)
5456
{
55-
if (movement >= 0)
57+
if (!_exceeded)
5658
{
57-
_increment(gsl::narrow<size_t>(movement));
58-
}
59-
else
60-
{
61-
_decrement(gsl::narrow<size_t>(-movement));
59+
if (movement >= 0)
60+
{
61+
_increment(gsl::narrow<size_t>(movement));
62+
}
63+
else
64+
{
65+
_decrement(gsl::narrow<size_t>(-movement));
66+
}
6267
}
6368

6469
return *this;
@@ -84,11 +89,13 @@ AttrRowIterator AttrRowIterator::operator--(int)
8489

8590
const TextAttribute* AttrRowIterator::operator->() const
8691
{
92+
THROW_HR_IF(E_BOUNDS, _exceeded);
8793
return &_run->GetAttributes();
8894
}
8995

9096
const TextAttribute& AttrRowIterator::operator*() const
9197
{
98+
THROW_HR_IF(E_BOUNDS, _exceeded);
9299
return _run->GetAttributes();
93100
}
94101

@@ -123,14 +130,23 @@ void AttrRowIterator::_decrement(size_t count)
123130
{
124131
while (count > 0)
125132
{
133+
// If there's still space within this color attribute to move left, do so.
126134
if (count <= _currentAttributeIndex)
127135
{
128136
_currentAttributeIndex -= count;
129137
return;
130138
}
139+
// If there's not space, move to the previous attribute run
140+
// We'll walk through above on the if branch to move left further (if necessary)
131141
else
132142
{
133-
count -= _currentAttributeIndex;
143+
// make sure we don't go out of bounds
144+
if (_run == _pAttrRow->_list.cbegin())
145+
{
146+
_exceeded = true;
147+
return;
148+
}
149+
count -= _currentAttributeIndex + 1;
134150
--_run;
135151
_currentAttributeIndex = _run->GetLength() - 1;
136152
}

‎src/buffer/out/AttrRowIterator.hpp

+1
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ class AttrRowIterator final
5454
std::vector<TextAttributeRun>::const_iterator _run;
5555
const ATTR_ROW* _pAttrRow;
5656
size_t _currentAttributeIndex; // index of TextAttribute within the current TextAttributeRun
57+
bool _exceeded;
5758

5859
void _increment(size_t count);
5960
void _decrement(size_t count);

‎src/cascadia/TerminalCore/TerminalSelection.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ COORD Terminal::_ExpandDoubleClickSelectionLeft(const COORD position) const
377377
while (positionWithOffsets.X > bufferViewport.Left() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter))
378378
{
379379
bufferViewport.DecrementInBounds(positionWithOffsets);
380-
bufferIterator--;
380+
--bufferIterator;
381381
}
382382

383383
if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter)
@@ -415,7 +415,7 @@ COORD Terminal::_ExpandDoubleClickSelectionRight(const COORD position) const
415415
while (positionWithOffsets.X < bufferViewport.RightInclusive() && (_GetDelimiterClass(*bufferIterator) == startedOnDelimiter))
416416
{
417417
bufferViewport.IncrementInBounds(positionWithOffsets);
418-
bufferIterator++;
418+
++bufferIterator;
419419
}
420420

421421
if (_GetDelimiterClass(*bufferIterator) != startedOnDelimiter)

‎src/host/ut_host/AttrRowTests.cpp

+129
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,135 @@ class AttrRowTests
498498
}
499499
}
500500

501+
TEST_METHOD(TestReverseIteratorWalkFromMiddle)
502+
{
503+
// GH #3409, walking backwards through color range runs out of bounds
504+
// We're going to create an attribute row with assorted colors and varying lengths
505+
// just like the row of text on the Ubuntu prompt line that triggered this bug being found.
506+
// Then we're going to walk backwards through the iterator like a selection-expand-to-left
507+
// operation and ensure we don't run off the bounds.
508+
509+
// walk the chain, from index, stepSize at a time
510+
// ensure we don't crash
511+
auto testWalk = [](ATTR_ROW* chain, size_t index, int stepSize) {
512+
// move to starting index
513+
auto iter = chain->cbegin();
514+
iter += index;
515+
516+
// Now walk backwards in a loop until 0.
517+
while (iter)
518+
{
519+
iter -= stepSize;
520+
}
521+
522+
Log::Comment(L"We made it through without crashing!");
523+
};
524+
525+
// take one step of size stepSize on the chain
526+
// index is where we start from
527+
// expectedAttribute is what we expect to read here
528+
auto verifyStep = [](ATTR_ROW* chain, size_t index, int stepSize, TextAttribute expectedAttribute) {
529+
// move to starting index
530+
auto iter = chain->cbegin();
531+
iter += index;
532+
533+
// Now step backwards
534+
iter -= stepSize;
535+
536+
VERIFY_ARE_EQUAL(expectedAttribute, *iter);
537+
};
538+
539+
Log::Comment(L"Reverse iterate through ubuntu prompt");
540+
{
541+
// Create attr row representing a buffer that's 121 wide.
542+
auto chain = std::make_unique<ATTR_ROW>(121, _DefaultAttr);
543+
544+
// The repro case had 4 chain segments.
545+
chain->_list.resize(4);
546+
547+
// The color 10 went for the first 18.
548+
chain->_list[0].SetAttributes(TextAttribute(0xA));
549+
chain->_list[0].SetLength(18);
550+
551+
// Default color for the next 1
552+
chain->_list[1].SetAttributes(TextAttribute());
553+
chain->_list[1].SetLength(1);
554+
555+
// Color 12 for the next 29
556+
chain->_list[2].SetAttributes(TextAttribute(0xC));
557+
chain->_list[2].SetLength(29);
558+
559+
// Then default color to end the run
560+
chain->_list[3].SetAttributes(TextAttribute());
561+
chain->_list[3].SetLength(73);
562+
563+
// The sum of the lengths should be 121.
564+
VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength + chain->_list[3]._cchLength);
565+
566+
auto index = chain->_list[0].GetLength();
567+
auto stepSize = 1;
568+
testWalk(chain.get(), index, stepSize);
569+
}
570+
571+
Log::Comment(L"Reverse iterate across a text run in the chain");
572+
{
573+
// Create attr row representing a buffer that's 3 wide.
574+
auto chain = std::make_unique<ATTR_ROW>(3, _DefaultAttr);
575+
576+
// The repro case had 3 chain segments.
577+
chain->_list.resize(3);
578+
579+
// The color 10 went for the first 1.
580+
chain->_list[0].SetAttributes(TextAttribute(0xA));
581+
chain->_list[0].SetLength(1);
582+
583+
// The color 11 for the next 1
584+
chain->_list[1].SetAttributes(TextAttribute(0xB));
585+
chain->_list[1].SetLength(1);
586+
587+
// Color 12 for the next 1
588+
chain->_list[2].SetAttributes(TextAttribute(0xC));
589+
chain->_list[2].SetLength(1);
590+
591+
// The sum of the lengths should be 3.
592+
VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength);
593+
594+
// on 'ABC', step from B to A
595+
auto index = 1;
596+
auto stepSize = 1;
597+
verifyStep(chain.get(), index, stepSize, TextAttribute(0xA));
598+
}
599+
600+
Log::Comment(L"Reverse iterate across two text runs in the chain");
601+
{
602+
// Create attr row representing a buffer that's 3 wide.
603+
auto chain = std::make_unique<ATTR_ROW>(3, _DefaultAttr);
604+
605+
// The repro case had 3 chain segments.
606+
chain->_list.resize(3);
607+
608+
// The color 10 went for the first 1.
609+
chain->_list[0].SetAttributes(TextAttribute(0xA));
610+
chain->_list[0].SetLength(1);
611+
612+
// The color 11 for the next 1
613+
chain->_list[1].SetAttributes(TextAttribute(0xB));
614+
chain->_list[1].SetLength(1);
615+
616+
// Color 12 for the next 1
617+
chain->_list[2].SetAttributes(TextAttribute(0xC));
618+
chain->_list[2].SetLength(1);
619+
620+
// The sum of the lengths should be 3.
621+
VERIFY_ARE_EQUAL(chain->_cchRowWidth, chain->_list[0]._cchLength + chain->_list[1]._cchLength + chain->_list[2]._cchLength);
622+
623+
// on 'ABC', step from C to A
624+
auto index = 2;
625+
auto stepSize = 2;
626+
verifyStep(chain.get(), index, stepSize, TextAttribute(0xA));
627+
}
628+
}
629+
501630
TEST_METHOD(TestSetAttrToEnd)
502631
{
503632
const WORD wTestAttr = FOREGROUND_BLUE | BACKGROUND_GREEN;

‎tools/ConsoleTypes.natvis

+9-6
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,19 @@
1212
<!-- You can't do too much trickyness inside the DisplayString format
1313
string, so we'd have to add entries for each flag if we really
1414
wanted them to show up like that. -->
15-
<DisplayString Condition="_isBold">{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Bold}}</DisplayString>
16-
<DisplayString>{{FG:{_foreground},BG:{_background},{_wAttrLegacy}, Normal}}</DisplayString>
15+
<DisplayString>{{FG: {_foreground}, BG: {_background}, Legacy: {_wAttrLegacy}, {_extendedAttrs}}</DisplayString>
1716
<Expand>
18-
<Item Name="_wAttrLegacy">_wAttrLegacy</Item>
19-
<Item Name="_isBold">_isBold</Item>
20-
<Item Name="_foreground">_foreground</Item>
21-
<Item Name="_background">_background</Item>
17+
<Item Name="Legacy">_wAttrLegacy</Item>
18+
<Item Name="FG">_foreground</Item>
19+
<Item Name="BG">_background</Item>
20+
<Item Name="Extended">_extendedAttrs</Item>
2221
</Expand>
2322
</Type>
2423

24+
<Type Name="TextAttributeRun">
25+
<DisplayString>Length={_cchLength} Attr={_attributes}</DisplayString>
26+
</Type>
27+
2528
<Type Name="Microsoft::Console::Types::Viewport">
2629
<!-- Can't call functions in here -->
2730
<DisplayString>{{LT({_sr.Left}, {_sr.Top}) RB({_sr.Right}, {_sr.Bottom}) [{_sr.Right-_sr.Left+1} x { _sr.Bottom-_sr.Top+1}]}}</DisplayString>

0 commit comments

Comments
 (0)