Skip to content

Commit

Permalink
Fix SetConsoleWindowInfo being able to crash ConPTY (#13212)
Browse files Browse the repository at this point in the history
MSFT-33471786 is one of the most common crashes we have right now.
Memory dumps suggest that `VtEngine::UpdateViewport` is called with a rectangle
like `(0, 46, 119, 29)` (left, top, right, bottom), which is a rectangle of
negative height. When the `_invalidMap` is resized the negative size gets
turned into a very large unsigned integer, which results in an OOM exception,
crashing OpenConsole.

`VtEngine::UpdateViewport` is called by `Renderer::_CheckViewportAndScroll`
which holds a (cached) old and a new viewport. The old viewport was
`(0, 46, 119, 75)` which is exceedingly similar to the invalid, new viewport.
It's bottom coordinate is also coincidentally larger by exactly 46 (top).

The viewport comes from the `SCREEN_INFORMATION` class whose `SetViewport`
function was highly suspicious as it has a branch which updates the bottom
to be the buffer height, but leaves the top unmodified.

`SCREEN_INFORMATION::SetViewport` is called by `SetConsoleWindowInfo` which
processes user-provided data. A repro of the crash can be constructed with:
```
SMALL_RECT rect{0, 46, 119, 75};
SetConsoleWindowInfo(GetStdHandle(STD_OUTPUT_HANDLE), TRUE, &rect);
```

Closes #13193
Closes MSFT-33471786

## Validation Steps Performed
Ensured the following code doesn't crash when run under Windows Terminal:
```
SMALL_RECT rect{0, 46, 119, 75};
SetConsoleWindowInfo(GetStdHandle(STD_OUTPUT_HANDLE), TRUE, &rect);
```

(cherry picked from commit 7dbe741)
Service-Card-Id: 82642053
Service-Version: 1.14
  • Loading branch information
lhecker authored and DHowett committed Jun 30, 2022
1 parent 93b5dff commit 9165c00
Showing 1 changed file with 10 additions and 21 deletions.
31 changes: 10 additions & 21 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2268,30 +2268,19 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
}

// do adjustments on a copy that's easily manipulated.
auto srCorrected = newViewport.ToExclusive();
const til::rect viewportRect{ newViewport.ToInclusive() };
const til::size coordScreenBufferSize{ GetBufferSize().Dimensions() };

if (srCorrected.Left < 0)
{
srCorrected.Right -= srCorrected.Left;
srCorrected.Left = 0;
}
if (srCorrected.Top < 0)
{
srCorrected.Bottom -= srCorrected.Top;
srCorrected.Top = 0;
}
// MSFT-33471786, GH#13193:
// newViewport may reside anywhere outside of the valid coordScreenBufferSize.
// For instance it might be scrolled down more than our text buffer allows to be scrolled.
const auto cx = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.width(), 1, coordScreenBufferSize.width));
const auto cy = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.height(), 1, coordScreenBufferSize.height));
const auto x = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.left, 0, coordScreenBufferSize.width - cx));
const auto y = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.top, 0, coordScreenBufferSize.height - cy));

const auto coordScreenBufferSize = GetBufferSize().Dimensions();
if (srCorrected.Right > coordScreenBufferSize.X)
{
srCorrected.Right = coordScreenBufferSize.X;
}
if (srCorrected.Bottom > coordScreenBufferSize.Y)
{
srCorrected.Bottom = coordScreenBufferSize.Y;
}
_viewport = Viewport::FromExclusive({ x, y, x + cx, y + cy });

_viewport = Viewport::FromExclusive(srCorrected);
if (updateBottom)
{
UpdateBottom();
Expand Down

0 comments on commit 9165c00

Please sign in to comment.