From ebde933638c8d8f6ec93f0aaf35e940ef1136b1f Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 5 Aug 2026 17:12:04 -0700 Subject: [PATCH] Fix conhost off-by-one errors --- src/buffer/out/search.cpp | 9 +++++++-- src/buffer/out/textBuffer.hpp | 5 ++--- src/host/selection.cpp | 4 ++-- src/host/selectionInput.cpp | 12 +++++++++--- src/host/ut_host/SearchTests.cpp | 3 ++- src/host/ut_host/SelectionTests.cpp | 28 ++++++++++++++++++++++++++++ src/types/UiaTextRangeBase.cpp | 3 +-- 7 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/buffer/out/search.cpp b/src/buffer/out/search.cpp index 75143f047ca..461c1a75f3a 100644 --- a/src/buffer/out/search.cpp +++ b/src/buffer/out/search.cpp @@ -119,11 +119,16 @@ bool Search::SelectCurrent() const { if (const auto s = GetCurrent()) { + const auto& textBuffer = _renderData->GetTextBuffer(); + + // GH#20152: s->end is exclusive, but SelectNewRegion expects an inclusive endpoint. + auto inclusiveEnd = s->end; + textBuffer.GetSize().DecrementInBounds(inclusiveEnd); + // Convert buffer selection offsets into the equivalent screen coordinates // required by SelectNewRegion, taking line renditions into account. - const auto& textBuffer = _renderData->GetTextBuffer(); const auto selStart = textBuffer.BufferToScreenPosition(s->start); - const auto selEnd = textBuffer.BufferToScreenPosition(s->end); + const auto selEnd = textBuffer.BufferToScreenPosition(inclusiveEnd); _renderData->SelectNewRegion(selStart, selEnd); return true; } diff --git a/src/buffer/out/textBuffer.hpp b/src/buffer/out/textBuffer.hpp index 9628fb3ab08..768151bf32c 100644 --- a/src/buffer/out/textBuffer.hpp +++ b/src/buffer/out/textBuffer.hpp @@ -198,9 +198,8 @@ class TextBuffer final struct CopyRequest { - // beg and end coordinates are inclusive - til::point beg; - til::point end; + til::point beg; // inclusive + til::point end; // exclusive til::CoordType minX; til::CoordType maxX; diff --git a/src/host/selection.cpp b/src/host/selection.cpp index 352b558d77e..4b7934f299f 100644 --- a/src/host/selection.cpp +++ b/src/host/selection.cpp @@ -520,8 +520,8 @@ void Selection::InitializeMarkSelection() // Routine Description: // - Resets the current selection and selects a new region from the start to end coordinates // Arguments: -// - coordStart - Position to start selection area from -// - coordEnd - Position to select up to +// - coordStart - Position to start selection area from (inclusive) +// - coordEnd - Position to select up to (inclusive) // Return Value: // - void Selection::SelectNewRegion(const til::point coordStart, const til::point coordEnd) diff --git a/src/host/selectionInput.cpp b/src/host/selectionInput.cpp index f7bcb3e6374..e3bf0420fba 100644 --- a/src/host/selectionInput.cpp +++ b/src/host/selectionInput.cpp @@ -673,9 +673,11 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo) if (fShiftPressed) { // Search the selection and color *that* + // GH#20152: srSelectionRect.right is inclusive, but CopyRequest::end is + // exclusive, so it must be adjusted by one const auto req = TextBuffer::CopyRequest::FromConfig(textBuffer, til::point{ _d->srSelectionRect.left, _d->srSelectionRect.top }, - til::point{ _d->srSelectionRect.right, _d->srSelectionRect.bottom }, + til::point{ _d->srSelectionRect.right + 1, _d->srSelectionRect.bottom }, true /* multi-line search doesn't make sense; concatenate all lines */, false /* we filtered out block search above */, true /* trim block selection */, @@ -687,7 +689,10 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo) const auto hits = textBuffer.SearchText(str, SearchFlag::CaseInsensitive).value_or(std::vector{}); for (const auto& s : hits) { - ColorSelection(s.start, s.end, selectionAttr); + // GH#20152: s.end is exclusive, but ColorSelection expects an inclusive endpoint. + auto inclusiveEnd = s.end; + textBuffer.GetSize().DecrementInBounds(inclusiveEnd); + ColorSelection(s.start, inclusiveEnd, selectionAttr); } } else @@ -695,7 +700,8 @@ bool Selection::_HandleColorSelection(const INPUT_KEY_INFO* const pInputKeyInfo) const auto selection = GetSelectionSpans(); for (auto&& sp : selection) { - sp.iterate_rows(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) { + // GH#20152: GetSelectionSpans() returns exclusive-end spans + sp.iterate_rows_exclusive(textBuffer.GetSize().Width(), [&](til::CoordType row, til::CoordType beg, til::CoordType end) { ColorSelection({ beg, row, end, row + 1 }, selectionAttr); }); } diff --git a/src/host/ut_host/SearchTests.cpp b/src/host/ut_host/SearchTests.cpp index 74f5e8ce6bf..48636efb867 100644 --- a/src/host/ut_host/SearchTests.cpp +++ b/src/host/ut_host/SearchTests.cpp @@ -63,8 +63,9 @@ class SearchTests { const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + // GH#20152: end is inclusive. Add 1 to point to the last column of the 2-wide match, not 1 past it. auto coordEndExpected = coordStartExpected; - coordEndExpected.x += 2; + coordEndExpected.x += 1; VERIFY_IS_TRUE(s.SelectCurrent()); VERIFY_ARE_EQUAL(coordStartExpected, gci.renderData.GetSelectionAnchor()); diff --git a/src/host/ut_host/SelectionTests.cpp b/src/host/ut_host/SelectionTests.cpp index f314e8dcc9e..04037dc6be5 100644 --- a/src/host/ut_host/SelectionTests.cpp +++ b/src/host/ut_host/SelectionTests.cpp @@ -348,6 +348,34 @@ class SelectionTests VERIFY_ARE_EQUAL(srOriginal.left + sDeltaLeft, srSelection.left); VERIFY_ARE_EQUAL(srOriginal.right + sDeltaRight, srSelection.right); } + + // GH#20152: verify Alt+Shift+ ("search the selection and color all matches") colors + // every character of each match, including the last one. + TEST_METHOD(TestColorSelectionSearchAndColorAllMatches) + { + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + auto& screenInfo = gci.GetActiveOutputBuffer(); + auto& textBuffer = screenInfo.GetTextBuffer(); + + RowWriteState state{ .text = L"foo bar foo" }; + textBuffer.GetMutableRowByOffset(0).Reset(TextAttribute{}); + textBuffer.Replace(0, TextAttribute{}, state); + + // Select the first "foo" (columns 0-2, inclusive) + m_pSelection->SelectNewRegion({ 0, 0 }, { 2, 0 }); + + // Simulate Alt+Shift+'1' + INPUT_KEY_INFO keyInfo{ static_cast('1'), LEFT_ALT_PRESSED | SHIFT_PRESSED }; + VERIFY_IS_TRUE(m_pSelection->_HandleColorSelection(&keyInfo)); + + // Both occurrences of "foo" (columns 0-2 and 8-10) should be fully colored + for (til::CoordType x = 0; x < 15; ++x) + { + const auto isColored = textBuffer.GetCellDataAt({ x, 0 })->TextAttr() != TextAttribute{}; + const auto shouldBeColored = (x >= 0 && x <= 2) || (x >= 8 && x <= 10); + VERIFY_ARE_EQUAL(shouldBeColored, isColored, NoThrowString().Format(L"column %d", x)); + } + } }; class SelectionInputTests diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index 1ec4f68338a..05886e2ae6b 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -637,9 +637,8 @@ try if (const auto hit = _searcher.GetCurrent()) { hitBeg = hit->start; + // GH#20152: hit->end is already exclusive, so no conversion is needed here hitEnd = hit->end; - // we need to increment the position of end because it's exclusive - _pData->GetTextBuffer().GetSize().IncrementInBounds(hitEnd, true); } if (hitBeg >= _start && hitEnd <= _end)