Skip to content

Commit

Permalink
Fix out-of-bounds exceptions in Set...{Buffer,Screen}Size (#8309)
Browse files Browse the repository at this point in the history
This fixes a number of exceptions that can cause conhost to crash when
the buffer is resized in such a way that the viewport or cursor position
end up out of bounds.

Technically this is a fix for issue #256, although that has been closed
as "needs-repro".

The main fix was to add checks in the `SetConsoleScreenBufferSizeImpl`
and `SetConsoleScreenBufferInfoExImpl` methods, to make sure the
viewport doesn't extend past the bottom or right of the buffer after a
resize. If it has overflowed, we move the viewport back up or left until
it's back within the buffer boundaries. We also check if the cursor
position has ended up out of bounds, and if so, clamp it back inside the
buffer.

The `SCREEN_INFORMATION::SetViewport` was also a source of viewport
overflow problems, because it was mistakenly using inclusive coordinates
in its range checks, which resulted in them being off by one. That has
now been corrected to use exclusive coordinates.

Finally, the `IsCursorDoubleWidth` method was incorrectly marked as
`noexcept`, which was preventing its exceptions from being caught.
Ideally it shouldn't be throwing exceptions at all any more, but I've
removed the `noexcept` specifier, so if it does throw an exception,
it'll at least have more chance of recovering without a crash.

## Validation Steps Performed

I put together a few test cases (based on the reports in issues #276 and
#1976) which consistently caused conhost to crash, or to generate an
exception visible in the debug output. With this PR applied, those test
cases are no longer crashing or triggering exceptions.

Closes #1976
  • Loading branch information
j4james authored Nov 17, 2020
1 parent 6b503ba commit 9a07049
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 7 deletions.
36 changes: 35 additions & 1 deletion src/host/getset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,24 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
COORD const coordScreenBufferSize = screenInfo.GetBufferSize().Dimensions();
if (size.X != coordScreenBufferSize.X || size.Y != coordScreenBufferSize.Y)
{
RETURN_NTSTATUS(screenInfo.ResizeScreenBuffer(size, TRUE));
RETURN_IF_NTSTATUS_FAILED(screenInfo.ResizeScreenBuffer(size, TRUE));
}

// Make sure the viewport doesn't now overflow the buffer dimensions.
auto overflow = screenInfo.GetViewport().EndExclusive() - screenInfo.GetBufferSize().Dimensions();
if (overflow.X > 0 || overflow.Y > 0)
{
overflow = { std::max<SHORT>(overflow.X, 0), std::max<SHORT>(overflow.Y, 0) };
RETURN_IF_NTSTATUS_FAILED(screenInfo.SetViewportOrigin(false, -overflow, false));
}

// And also that the cursor position is clamped within the buffer boundaries.
auto& cursor = screenInfo.GetTextBuffer().GetCursor();
auto clampedCursorPosition = cursor.GetPosition();
screenInfo.GetBufferSize().Clamp(clampedCursorPosition);
if (clampedCursorPosition != cursor.GetPosition())
{
cursor.SetPosition(clampedCursorPosition);
}

return S_OK;
Expand Down Expand Up @@ -620,6 +637,23 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
// (see https://msdn.microsoft.com/en-us/library/windows/desktop/ms686125(v=vs.85).aspx and DoSrvSetConsoleWindowInfo)
// Note that it also doesn't set cursor position.

// However, we do need to make sure the viewport doesn't now overflow the buffer dimensions.
auto overflow = context.GetViewport().EndExclusive() - context.GetBufferSize().Dimensions();
if (overflow.X > 0 || overflow.Y > 0)
{
overflow = { std::max<SHORT>(overflow.X, 0), std::max<SHORT>(overflow.Y, 0) };
RETURN_IF_NTSTATUS_FAILED(context.SetViewportOrigin(false, -overflow, false));
}

// And also that the cursor position is clamped within the buffer boundaries.
auto& cursor = context.GetTextBuffer().GetCursor();
auto clampedCursorPosition = cursor.GetPosition();
context.GetBufferSize().Clamp(clampedCursorPosition);
if (clampedCursorPosition != cursor.GetPosition())
{
cursor.SetPosition(clampedCursorPosition);
}

return S_OK;
}
CATCH_RETURN();
Expand Down
2 changes: 1 addition & 1 deletion src/host/renderData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ const std::vector<Microsoft::Console::Render::RenderOverlay> RenderData::GetOver
// - <none>
// Return Value:
// - true if the cursor should be drawn twice as wide as usual
bool RenderData::IsCursorDoubleWidth() const noexcept
bool RenderData::IsCursorDoubleWidth() const
{
const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
return gci.GetActiveOutputBuffer().CursorIsDoubleWidth();
Expand Down
2 changes: 1 addition & 1 deletion src/host/renderData.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class RenderData final :
CursorType GetCursorStyle() const noexcept override;
ULONG GetCursorPixelWidth() const noexcept override;
COLORREF GetCursorColor() const noexcept override;
bool IsCursorDoubleWidth() const noexcept override;
bool IsCursorDoubleWidth() const override;

bool IsScreenReversed() const noexcept override;

Expand Down
8 changes: 4 additions & 4 deletions src/host/screenInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2146,7 +2146,7 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
}

// do adjustments on a copy that's easily manipulated.
SMALL_RECT srCorrected = newViewport.ToInclusive();
SMALL_RECT srCorrected = newViewport.ToExclusive();

if (srCorrected.Left < 0)
{
Expand All @@ -2160,16 +2160,16 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
}

const COORD coordScreenBufferSize = GetBufferSize().Dimensions();
if (srCorrected.Right >= coordScreenBufferSize.X)
if (srCorrected.Right > coordScreenBufferSize.X)
{
srCorrected.Right = coordScreenBufferSize.X;
}
if (srCorrected.Bottom >= coordScreenBufferSize.Y)
if (srCorrected.Bottom > coordScreenBufferSize.Y)
{
srCorrected.Bottom = coordScreenBufferSize.Y;
}

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

0 comments on commit 9a07049

Please sign in to comment.