Skip to content

Commit 9a07049

Browse files
authored
Fix out-of-bounds exceptions in Set...{Buffer,Screen}Size (#8309)
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
1 parent 6b503ba commit 9a07049

File tree

4 files changed

+41
-7
lines changed

4 files changed

+41
-7
lines changed

src/host/getset.cpp

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,24 @@ void ApiRoutines::GetLargestConsoleWindowSizeImpl(const SCREEN_INFORMATION& cont
524524
COORD const coordScreenBufferSize = screenInfo.GetBufferSize().Dimensions();
525525
if (size.X != coordScreenBufferSize.X || size.Y != coordScreenBufferSize.Y)
526526
{
527-
RETURN_NTSTATUS(screenInfo.ResizeScreenBuffer(size, TRUE));
527+
RETURN_IF_NTSTATUS_FAILED(screenInfo.ResizeScreenBuffer(size, TRUE));
528+
}
529+
530+
// Make sure the viewport doesn't now overflow the buffer dimensions.
531+
auto overflow = screenInfo.GetViewport().EndExclusive() - screenInfo.GetBufferSize().Dimensions();
532+
if (overflow.X > 0 || overflow.Y > 0)
533+
{
534+
overflow = { std::max<SHORT>(overflow.X, 0), std::max<SHORT>(overflow.Y, 0) };
535+
RETURN_IF_NTSTATUS_FAILED(screenInfo.SetViewportOrigin(false, -overflow, false));
536+
}
537+
538+
// And also that the cursor position is clamped within the buffer boundaries.
539+
auto& cursor = screenInfo.GetTextBuffer().GetCursor();
540+
auto clampedCursorPosition = cursor.GetPosition();
541+
screenInfo.GetBufferSize().Clamp(clampedCursorPosition);
542+
if (clampedCursorPosition != cursor.GetPosition())
543+
{
544+
cursor.SetPosition(clampedCursorPosition);
528545
}
529546

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

640+
// However, we do need to make sure the viewport doesn't now overflow the buffer dimensions.
641+
auto overflow = context.GetViewport().EndExclusive() - context.GetBufferSize().Dimensions();
642+
if (overflow.X > 0 || overflow.Y > 0)
643+
{
644+
overflow = { std::max<SHORT>(overflow.X, 0), std::max<SHORT>(overflow.Y, 0) };
645+
RETURN_IF_NTSTATUS_FAILED(context.SetViewportOrigin(false, -overflow, false));
646+
}
647+
648+
// And also that the cursor position is clamped within the buffer boundaries.
649+
auto& cursor = context.GetTextBuffer().GetCursor();
650+
auto clampedCursorPosition = cursor.GetPosition();
651+
context.GetBufferSize().Clamp(clampedCursorPosition);
652+
if (clampedCursorPosition != cursor.GetPosition())
653+
{
654+
cursor.SetPosition(clampedCursorPosition);
655+
}
656+
623657
return S_OK;
624658
}
625659
CATCH_RETURN();

src/host/renderData.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ const std::vector<Microsoft::Console::Render::RenderOverlay> RenderData::GetOver
276276
// - <none>
277277
// Return Value:
278278
// - true if the cursor should be drawn twice as wide as usual
279-
bool RenderData::IsCursorDoubleWidth() const noexcept
279+
bool RenderData::IsCursorDoubleWidth() const
280280
{
281281
const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
282282
return gci.GetActiveOutputBuffer().CursorIsDoubleWidth();

src/host/renderData.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class RenderData final :
4646
CursorType GetCursorStyle() const noexcept override;
4747
ULONG GetCursorPixelWidth() const noexcept override;
4848
COLORREF GetCursorColor() const noexcept override;
49-
bool IsCursorDoubleWidth() const noexcept override;
49+
bool IsCursorDoubleWidth() const override;
5050

5151
bool IsScreenReversed() const noexcept override;
5252

src/host/screenInfo.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2146,7 +2146,7 @@ void SCREEN_INFORMATION::SetViewport(const Viewport& newViewport,
21462146
}
21472147

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

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

21622162
const COORD coordScreenBufferSize = GetBufferSize().Dimensions();
2163-
if (srCorrected.Right >= coordScreenBufferSize.X)
2163+
if (srCorrected.Right > coordScreenBufferSize.X)
21642164
{
21652165
srCorrected.Right = coordScreenBufferSize.X;
21662166
}
2167-
if (srCorrected.Bottom >= coordScreenBufferSize.Y)
2167+
if (srCorrected.Bottom > coordScreenBufferSize.Y)
21682168
{
21692169
srCorrected.Bottom = coordScreenBufferSize.Y;
21702170
}
21712171

2172-
_viewport = Viewport::FromInclusive(srCorrected);
2172+
_viewport = Viewport::FromExclusive(srCorrected);
21732173
if (updateBottom)
21742174
{
21752175
UpdateBottom();

0 commit comments

Comments
 (0)