From efa02f35e25d0e1f8119c28a57eb249553cce075 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Fri, 10 Jun 2022 11:09:09 -0700 Subject: [PATCH] Fix a11y crash in alt buffer apps (#13250) This fixes the crashes caused by using a screen reader when in an app that uses the alt buffer via two changes: 1. Fix `Terminal::ViewEndIndex()` - `UiaTextRangeBase` receives a coordinate that is outside of the bounds of the text buffer via the following chain of functions... `_getDocumentEnd()` --> `GetLastNonSpaceCharacter()` --> `_getOptimizedBufferSize()` --> `GetTextBufferEndPoisition()` --> `ViewEndIndex()` - Since support for the alt buffer was added recently, `ViewEndIndex()` was recently changed, so that explains why this issue came up recently. We were accidentally setting the view end index to `height` instead of `height-1`. Thanks @j4james for finding this! - The UIA code would get the "exclusive end" of the alt buffer. Since it was using `ViewEndIndex()` to calculate that, it was one more than it should be. The UIA code has explicit allowance for "one past the end of the viewport" in its `IsInBounds()` check. Since the `ViewEndIndex()` is way beyond that, it's not allowed, hitting the fail fast. 2. Replace `FAIL_FAST_IF` with `assert` - These fail fast calls have caused so many issues with our UIA code. Those checks still provide value, but they shouldn't take the whole app down. This change replaces the `Viewport` and `UiaTextRangeBase` fail fasts with asserts to still perform those checks, but not take down the entire app in release builds. Closes #13183 While using Narrator... - opened nano in bash - generated text and scrolled in nano - generated text and scrolled in PowerShell (cherry picked from commit 94e1697a48f08d793a2a852e4f57be790acdeb23) Service-Card-Id: 82972865 Service-Version: 1.14 --- src/cascadia/TerminalCore/Terminal.cpp | 2 +- src/types/UiaTextRangeBase.cpp | 8 ++++---- src/types/viewport.cpp | 7 ++++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 4e20e94e00a..66b3e77bca5 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -990,7 +990,7 @@ int Terminal::ViewStartIndex() const noexcept int Terminal::ViewEndIndex() const noexcept { - return _inAltBuffer() ? _altBufferSize.height : _mutableViewport.BottomInclusive(); + return _inAltBuffer() ? _altBufferSize.height - 1 : _mutableViewport.BottomInclusive(); } // _VisibleStartIndex is the first visible line of the buffer diff --git a/src/types/UiaTextRangeBase.cpp b/src/types/UiaTextRangeBase.cpp index e9dd19912ce..5b431f055d0 100644 --- a/src/types/UiaTextRangeBase.cpp +++ b/src/types/UiaTextRangeBase.cpp @@ -1289,9 +1289,9 @@ try } } - FAIL_FAST_IF(!(newViewport.Top >= topRow)); - FAIL_FAST_IF(!(newViewport.Bottom <= bottomRow)); - FAIL_FAST_IF(!(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport))); + assert(newViewport.Top >= topRow); + assert(newViewport.Bottom <= bottomRow); + assert(_getViewportHeight(oldViewport) == _getViewportHeight(newViewport)); Unlock.reset(); @@ -1338,7 +1338,7 @@ const COORD UiaTextRangeBase::_getScreenFontSize() const noexcept // - The viewport height const unsigned int UiaTextRangeBase::_getViewportHeight(const SMALL_RECT viewport) const noexcept { - FAIL_FAST_IF(!(viewport.Bottom >= viewport.Top)); + assert(viewport.Bottom >= viewport.Top); // + 1 because COORD is inclusive on both sides so subtracting top // and bottom gets rid of 1 more then it should. return viewport.Bottom - viewport.Top + 1; diff --git a/src/types/viewport.cpp b/src/types/viewport.cpp index b71c3d3d892..03826db3c95 100644 --- a/src/types/viewport.cpp +++ b/src/types/viewport.cpp @@ -362,11 +362,12 @@ bool Viewport::DecrementInBoundsCircular(COORD& pos) const noexcept // - This is so you can do s_CompareCoords(first, second) <= 0 for "first is left or the same as second". // (the < looks like a left arrow :D) // - The magnitude of the result is the distance between the two coordinates when typing characters into the buffer (left to right, top to bottom) +#pragma warning(suppress : 4100) int Viewport::CompareInBounds(const COORD& first, const COORD& second, bool allowEndExclusive) const noexcept { // Assert that our coordinates are within the expected boundaries - FAIL_FAST_IF(!IsInBounds(first, allowEndExclusive)); - FAIL_FAST_IF(!IsInBounds(second, allowEndExclusive)); + assert(IsInBounds(first, allowEndExclusive)); + assert(IsInBounds(second, allowEndExclusive)); // First set the distance vertically // If first is on row 4 and second is on row 6, first will be -2 rows behind second * an 80 character row would be -160. @@ -429,7 +430,7 @@ bool Viewport::WalkInBounds(COORD& pos, const WalkDir dir, bool allowEndExclusiv bool Viewport::WalkInBoundsCircular(COORD& pos, const WalkDir dir, bool allowEndExclusive) const noexcept { // Assert that the position given fits inside this viewport. - FAIL_FAST_IF(!IsInBounds(pos, allowEndExclusive)); + assert(IsInBounds(pos, allowEndExclusive)); if (dir.x == XWalk::LeftToRight) {