Fix SetConsoleWindowInfo being able to crash ConPTY #13212

lhecker · 2022-05-31T23:36:29Z

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);

lhecker · 2022-05-31T23:43:23Z

src/host/screenInfo.cpp

+    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));


This function needs to correct the newViewport argument to be within the valid bounds, while retaining the given size as much as possible. If you pass a newViewport with the top/bottom coordinates -130 and -100, it should still result in a viewport that's 30 high, but with a top/bottom of 0 and 30.

I feel like this change increases the robustness of the code by splitting the viewport into its size (cx/cy) and origin (x/y). That way we can first ensure that the size doesn't exceed our limits (just like the old code seemingly intended) and then adjust the origin to be within the smaller rectangle of valid coordinates.

The gsl::narrow_cast<SHORT>s will be removed once my 32-bit coord PR has been merged, but it shouldn't pose a problem here either, as GetBufferSize().Dimensions() is a COORD which can't exceed a SHORT.

carlos-zamora · 2022-06-01T00:19:14Z

src/host/screenInfo.cpp

+    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));


Suggested change

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 cx = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.width(), 0, coordScreenBufferSize.width));

const auto cy = gsl::narrow_cast<SHORT>(std::clamp(viewportRect.height(), 0, coordScreenBufferSize.height));

curious, why not allow a width/height of 0?

The minimum size the TextBuffer allows is (1,1), so I thought it'd be reasonable if we did the same here.

A text buffer with a zero dimension would not be a particularly useful text buffer :)

ghost · 2022-06-01T17:47:04Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

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

ghost · 2022-07-06T18:06:04Z

🎉Windows Terminal v1.14.186 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-07-06T18:07:00Z

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Severity-Crash Crashes are real bad news. labels May 31, 2022

lhecker added this to To Cherry Pick in 1.13 Servicing Pipeline via automation May 31, 2022

lhecker added this to To Cherry Pick in 1.14 Servicing Pipeline via automation May 31, 2022

lhecker added this to To Cherry Pick in Inbox Servicing Pipeline via automation May 31, 2022

lhecker commented May 31, 2022

View reviewed changes

Fix SetConsoleWindowInfo being able to crash ConPTY

02ab51d

lhecker force-pushed the user/lhecker/13193-fail-fast-crash branch from 35deadc to 02ab51d Compare May 31, 2022 23:45

carlos-zamora reviewed Jun 1, 2022

View reviewed changes

carlos-zamora approved these changes Jun 1, 2022

View reviewed changes

Fix confusion between inclusive/exclusive SMALL_RECT

84ccf12

carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Jun 1, 2022

ghost requested review from zadjii-msft, PankajBhojwani and DHowett June 1, 2022 17:36

DHowett approved these changes Jun 1, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 1, 2022

ghost merged commit 7dbe741 into main Jun 1, 2022

ghost deleted the user/lhecker/13193-fail-fast-crash branch June 1, 2022 17:47

DHowett moved this from To Cherry Pick to Rejected in Inbox Servicing Pipeline Jun 7, 2022

DHowett moved this from To Cherry Pick to Cherry Picked in 1.14 Servicing Pipeline Jun 30, 2022

ghost mentioned this pull request Jul 6, 2022

Exception 0xc0000409 in ucrtbase.dll #13193

Closed

zadjii-msft mentioned this pull request Jul 7, 2022

Scrambled prompt/input area #10130

Open

DHowett moved this from Cherry Picked to Shipped in 1.14 Servicing Pipeline Aug 10, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SetConsoleWindowInfo being able to crash ConPTY #13212

Fix SetConsoleWindowInfo being able to crash ConPTY #13212

lhecker commented May 31, 2022 •

edited

lhecker May 31, 2022 •

edited

carlos-zamora Jun 1, 2022

lhecker Jun 1, 2022 •

edited

DHowett Jun 1, 2022

ghost commented Jun 1, 2022

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

		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));

Fix SetConsoleWindowInfo being able to crash ConPTY #13212

Fix SetConsoleWindowInfo being able to crash ConPTY #13212

Conversation

lhecker commented May 31, 2022 • edited

Validation Steps Performed

lhecker May 31, 2022 • edited

Choose a reason for hiding this comment

carlos-zamora Jun 1, 2022

Choose a reason for hiding this comment

lhecker Jun 1, 2022 • edited

Choose a reason for hiding this comment

DHowett Jun 1, 2022

Choose a reason for hiding this comment

ghost commented Jun 1, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

lhecker commented May 31, 2022 •

edited

lhecker May 31, 2022 •

edited

lhecker Jun 1, 2022 •

edited

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.