Audit: the #18106 "exclusive selection range" effort

Repository: microsoft/terminal (D:\projects\terminal)
Branch audited: dev/cazamor/fix/search-selection-off-by-one @ ebde93363 ("Fix conhost off-by-one errors")
Date: 2026-08-06

Scope & method

Traced the full change surface of #18106 (64d4fbab1, "Make selection an exclusive range") plus every follow-up fix:

Layers reviewed: src/inc/til, src/types (Viewport + UIA), src/buffer/out, src/host, src/renderer, src/cascadia/TerminalCore, src/cascadia/TerminalControl.

Findings A, B, and C were empirically proven by building Conhost.Unit.Tests / Types.Unit.Tests / TextBuffer.Unit.Tests and running throwaway TAEF tests. Those temporary tests were reverted; the working tree is clean.

Verdict: the off-by-one errors are not all resolved. 8 real defects remain, plus a documentation defect that is the root cause of the entire series.

🔴 The root cause — fix this first

TextBuffer::SearchText still documents its result as inclusive.

src/buffer/out/textBuffer.cpp:3066 and :3073:

// Searches through the entire (committed) text buffer for `needle` ...
// The end coordinates of the returned ranges are considered inclusive.        // ← WRONG
std::optional<std::vector<til::point_span>> TextBuffer::SearchText(...) const

// Searches through the given rows [rowBeg,rowEnd) for `needle` ...
// While the end coordinates of the returned ranges are considered inclusive,  // ← WRONG
//   the [rowBeg,rowEnd) range is half-open.

#18106 changed the underlying ICU::BufferRangeFromMatch to return a half-open range and correctly updated that function's own comment:

- // Returns an inclusive point range given a text start and end position.
+ // Returns a half-open [beg,end) range given a text start and end position.
...
-     auto nativeIndexEnd = uregex_end64(re, 0, &status);
-     // The parameters are given as a half-open [beg,end) range, but the point_span we return in closed [beg,end].
-     nativeIndexEnd--;
+     const auto nativeIndexEnd = uregex_end64(re, 0, &status);
...
-         ret.end.x = ...GetTrailingColumnAtCharOffset(ut->chunkOffset);
+         ret.end.x = ...GetLeadingColumnAtCharOffset(ut->chunkOffset);

…but the public SearchText API comment directly above it was never updated. Every consumer that produced GH#20152 was reading a comment that told them the end was inclusive. This one stale comment is why the same bug keeps recurring.

Fix: change both comments to "half-open [beg, end)", matching UTextAdapter.cpp:406.

Real bugs

A. Viewport::WalkInExclusiveBounds — off-by-one in max ✅ proven

src/types/viewport.cpp:344-356

bool Viewport::WalkInExclusiveBounds(til::point& pos, const til::CoordType delta) const noexcept
{
    const auto w = static_cast<ptrdiff_t>(std::max(0, _sr.right - _sr.left + 2));
    const auto h = static_cast<ptrdiff_t>(std::max(0, _sr.bottom - _sr.top + 1));
    const auto max = w * h;   // ← should be w * h - 1

In exclusive space the last valid position is BottomInclusiveRightExclusive() = {W, H-1}, whose offset is (W+1)*H - 1 = w*h - 1. Allowing max = w*h lets the walk land on offset w*h, which decodes to {0, H} — a row that does not exist.

Contrast the inclusive sibling, which gets this right:

// WalkInBounds
const auto max = w * h - !allowEndExclusive;

Proof (5×4 viewport):

BottomInclusiveRightExclusive=(X:5, Y:3)
Verify: IsTrue(vp.IsInExclusiveBounds(pos))       ← passes
After increment: pos=(X:0, Y:4) moved=1
Error: Verify: IsFalse(moved)                     ← FAILED

IncrementInExclusiveBounds returned true (claiming success) and produced an out-of-bounds point.

Reachable from: TerminalSelection.cpp:230 — Shift+Click forward at the very end of the buffer calls IncrementInExclusiveBounds(textBufferPos) with no upper guard.

Fix:

-    const auto max = w * h;
+    const auto max = w * h - 1;

B. CopyRequest clamps an exclusive end to an inclusive maximum ✅ proven

src/buffer/out/textBuffer.hpp:216-220

constexpr CopyRequest(const TextBuffer& buffer, const til::point& beg, const til::point& end, ...) noexcept :
    beg{ std::max(beg, til::point{ 0, 0 }) },
    end{ std::min(end, til::point{ buffer._width - 1, buffer._height - 1 }) },   // ← width - 1 is wrong
    minX{ std::min(this->beg.x, this->end.x) },
    maxX{ std::max(this->beg.x, this->end.x) },

end is exclusive, so its legal maximum is {width, height-1} (= BottomInclusiveRightExclusive). Clamping to width - 1 silently truncates any selection that reaches the last column of the last row of the buffer. (The height - 1 on y is correct — _RowCopyHelper uses end.y inclusively.)

Proof (10×20 buffer, row 19 = "ABCDEFGHIJ", select [(0,19), (10,19))):

req.beg=(X:0, Y:19) req.end=(X:9, Y:19)   (passed end was (X:10, Y:19))
GetPlainText -> 'ABCDEFGHI'
Error: Verify: AreEqual(...) - Values (ABCDEFGHIJ, ABCDEFGHI)   ← FAILED

Blast radius — every CopyRequest consumer:

• src/interactivity/win32/Clipboard.cpp:364 — conhost copy to clipboard
• src/host/selectionInput.cpp:678 — conhost Alt+Shift+# search string
• src/cascadia/TerminalCore/TerminalSelection.cpp:951 — Terminal copy (plain / HTML / RTF)
• src/types/UiaTextRangeBase.cpp:994 — UIA GetText

⚠️ The pending commit's srSelectionRect.right + 1 (selectionInput.cpp:680) is correct, but it makes this latent bug newly reachable on the bottom row.

Fix:

-    end{ std::min(end, til::point{ buffer._width - 1, buffer._height - 1 }) },
+    end{ std::min(end, til::point{ buffer._width, buffer._height - 1 }) },

C. MoveToPreviousGlyph2 cannot move backward from the buffer end ✅ proven

src/buffer/out/textBuffer.cpp:1636-1660

bool TextBuffer::MoveToPreviousGlyph2(til::point& pos, std::optional<til::point> limitOptional) const
{
    const auto limit{ limitOptional.value_or(bufferSize.BottomInclusiveRightExclusive()) };

    if (pos >= limit)          // ← copy-pasted from MoveToNextGlyph2; inverted for a backward move
    {
        pos = limit;
        return false;
    }

The guard is correct for MoveToNextGlyph2 (you can't move forward past the limit) but wrong for a backward move. The v1 function gets it right — MoveToPreviousGlyph uses CompareInBounds(pos, limit, true) > 0 (strictly past) and returns true.

Proof (10×20 buffer):

from (X:10, Y:19) -> (X:10, Y:19) (moved=0)
Error: Verify: IsTrue(moved): Should be able to move backward from the end of the buffer   ← FAILED

Impact: mark-mode ← does nothing when the selection endpoint sits at the very end of the buffer (TerminalSelection.cpp:796).

D. Terminal::SelectAll() uses RightInclusive() for an exclusive end

src/cascadia/TerminalCore/TerminalSelection.cpp:778

void Terminal::SelectAll()
{
    const auto bufferSize{ _activeBuffer().GetSize() };
    const til::point end{ bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() };  // ← BUG

Compare _MoveByBuffer at line 906 — the identical conceptual endpoint, written correctly:

    pos = { bufferSize.RightExclusive(), _GetMutableViewport().BottomInclusive() };

With an exclusive end of {width-1, bottom}, iterate_rows_exclusive highlights the bottom row over [0, width-1) — the last column is never selected. Other RightExclusive() uses for the same concept: line 312 (_ExpandSelectionAnchors, Line), line 815 (_MoveByChar, Down), line 875, line 889.

Why it survived: the line predates #18106 and #18106 never touched SelectAll. There is no SelectAll unit test in UnitTests_TerminalCore.

Fix:

-    const til::point end{ bufferSize.RightInclusive(), _GetMutableViewport().BottomInclusive() };
+    const til::point end{ bufferSize.RightExclusive(), _GetMutableViewport().BottomInclusive() };

E / F / G. Three UIA guards reject a legitimate {width, y} endpoint

src/types/UiaTextRangeBase.cpp — :239 (CompareEndpoints), :1145 (MoveEndpointByRange), :1170 (Select)

All three validate with:

RETURN_HR_IF(E_FAIL, !bufferSize.IsInBounds(mine, true) || !bufferSize.IsInBounds(other, true));

But Viewport::IsInBounds(pos, /*allowEndExclusive*/ true) whitelists only the single point EndExclusive() == {Left, BottomExclusive}:

bool Viewport::IsInBounds(const til::point pos, bool allowEndExclusive) const noexcept
{
    if (allowEndExclusive && pos == EndExclusive()) { return true; }
    return pos.x >= Left() && pos.x < RightExclusive() && ...;   // rejects x == width
}

It rejects a per-row RightExclusive ({width, y}). #18106 fixed exactly this in _getTextValue (:989) but missed the other three:

auto isValid = [&](const til::point& point) {
    return bufferSize.IsInExclusiveBounds(point) || point == bufferSize.EndExclusive();
};

Terminal stores line/word/full-line selection ends as {RightExclusive(), y} (TerminalSelection.cpp:312, :875, :906), and TermControlUiaProvider::GetSelectionRange now passes GetSelectionEnd() through unmodified. So screen readers get E_FAIL when comparing, moving, or selecting such a range.

Note the bodies are fine — Select()'s DecrementInBounds({width,y}) → {width-1,y} is correct. Only the guards need relaxing to the isValid pattern above.

H. Debug-only assert in Terminal::ColorSelection

src/cascadia/TerminalCore/Terminal.cpp:1555

const auto spanLength = textBuffer.GetSize().CompareInBounds(coordEndExclusive, coordStartInclusive, true);

CompareInBounds(..., allowEndExclusive=true) asserts IsInBounds(pos, true), which — per E/F/G — rejects {width, y} for y < bottom. A Line-expanded selection on a non-bottom row produces exactly that, so ColorSelection with matchMode == None trips the assert in debug builds. Release math is convention-agnostic and correct.

Fix: use CompareInExclusiveBounds(coordEndExclusive, coordStartInclusive), whose assert accepts any row's RightExclusive. (Bonus: this gives the currently-dead function a caller — see improvement ii.)

Stale / misleading comments

Simple improvements

i. til::point_span::iterate_rows now has zero callers.
The pending PR converts the last one (host/selectionInput.cpp:704) to iterate_rows_exclusive. Delete the inclusive version, then rename iterate_rows_exclusive → iterate_rows. This structurally removes the ability to make this class of mistake again — the highest-leverage cleanup available.

ii. Viewport::CompareInExclusiveBounds has zero callers.
Either delete it or wire it into Terminal.cpp:1555 (finding H). Also note an internal inconsistency: it uses Width() as the row stride while WalkInExclusiveBounds uses Width() + 1, so the two disagree on whether {w, y} and {0, y+1} are the same position. (Compare's stride is right for measuring cell distance; just be aware Compare(a,b) == 0 does not imply a == b.)

iii. AtlasEngine::_invalidateSpans passes til::CoordTypeMax as the width.
renderer/atlas/AtlasEngine.api.cpp:92

sp.iterate_rows_exclusive(til::CoordTypeMax, [&](til::CoordType row, til::CoordType beg, til::CoordType end) {
    const auto shift = buffer.GetLineRendition(row) != LineRendition::SingleWidth ? 1 : 0;
    end <<= shift;                    // INT32_MAX << 1  → signed-overflow UB

The function already has buffer, so it can pass the real width. Consequences today: non-final rows get end == INT32_MAX; on double-width rows end <<= 1 is UB, the rect collapses to {} via operator&, and invalidatedRows.start is forced to 0. Benign (over-invalidation) and it pre-dates #18106, but it's adjacent and cheap. The sentinel also defeats iterate_rows_exclusive's ax == w normalization. renderer/base/renderer.cpp:702 does it correctly with bufferWidth.

Confirmed correct (no action needed)

The pending commit ebde93363 is sound. Specifically verified:

• search.cpp — DecrementInBounds is genuinely the right choice here (not DecrementInExclusiveBounds): both {w,y} and {0,y+1} correctly map to {w-1,y}, whereas the exclusive variant would map {0,y+1} → {w,y}. The added comment is accurate.
• selectionInput.cpp — right + 1 and the iterate_rows_exclusive switch are both correct (modulo finding B, which is pre-existing).
• UiaTextRangeBase::FindText — removing the IncrementInBounds is correct; hitEnd <= _end now compares like-for-like.
• Existing SelectionTests + SearchTests pass, including the new TestColorSelectionSearchAndColorAllMatches.

Also verified correct under the new convention:

• renderer/base/renderer.cpp:702 (TriggerSelection) and AtlasEngine::_drawHighlighted — properly exclusive; the redundant max += 1 was correctly removed.
• TextBuffer::_ExpandTextRow — happens to be correct for a half-open [left, right) rect (checking cell[right] == Trailing and incrementing is exactly right when right is exclusive).
• Conhost block-selection anchor math in Selection::_RegenerateSelectionSpans / GetSelectionAnchors — the two blocks are logically equivalent and produce correct exclusive ends for all four drag directions.
• ControlCore::_selectSpan, ControlCore::ClearSearch, SelectionStartForRendering/SelectionEndForRendering, _ConvertToBufferCell, GetHyperlinkAtBufferPosition, _getPatterns.
• GDI / UIA renderers — consume pre-computed til::rects; no inclusive↔exclusive math.

Suggested fix order

1. SearchText comments — the root cause; prevents the next recurrence.
2. B — user-visible data loss on copy, and the pending PR widens its reach.
3. D, E/F/G — user-visible (Select All, accessibility).
4. A, C, H — latent / edge-case correctness.
5. Remaining stale comments.
6. Dead-code cleanup (i, ii) — ideally fold i into this PR while the last iterate_rows caller is already being touched.