Skip to content

Commit

Permalink
Fix crash when window width and height are too high (#1134)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

Currently, the program crashes with a window width or height greater than 32767 (accounting for window decorations). This can be caused when the `initialRows` and `initialColumns` settings are set too high (also depends on the font width and height). This fixes the issue by not allowing the window to expand beyond 32767x32767.

## References
#843 - relocated the ClampToShortMax helper for reuse elsewhere
  • Loading branch information
dkter authored and miniksa committed Jun 4, 2019
1 parent 30a579e commit 7ede378
Show file tree
Hide file tree
Showing 7 changed files with 79 additions and 15 deletions.
20 changes: 9 additions & 11 deletions src/cascadia/TerminalCore/Terminal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,11 @@

using namespace winrt::Microsoft::Terminal::Settings;
using namespace Microsoft::Terminal::Core;
using namespace Microsoft::Console;
using namespace Microsoft::Console::Render;
using namespace Microsoft::Console::Types;
using namespace Microsoft::Console::VirtualTerminal;

static constexpr short _ClampToShortMax(int value, short min)
{
return static_cast<short>(std::clamp(value, static_cast<int>(min), SHRT_MAX));
}

static std::wstring _KeyEventsToText(std::deque<std::unique_ptr<IInputEvent>>& inEventsToWrite)
{
std::wstring wstr = L"";
Expand Down Expand Up @@ -70,7 +66,8 @@ void Terminal::Create(COORD viewportSize, SHORT scrollbackLines, IRenderTarget&
{
_mutableViewport = Viewport::FromDimensions({ 0,0 }, viewportSize);
_scrollbackLines = scrollbackLines;
const COORD bufferSize { viewportSize.X, _ClampToShortMax(viewportSize.Y + scrollbackLines, 1) };
const COORD bufferSize { viewportSize.X,
Utils::ClampToShortMax(viewportSize.Y + scrollbackLines, 1) };
const TextAttribute attr{};
const UINT cursorSize = 12;
_buffer = std::make_unique<TextBuffer>(bufferSize, attr, cursorSize, renderTarget);
Expand All @@ -84,9 +81,10 @@ void Terminal::Create(COORD viewportSize, SHORT scrollbackLines, IRenderTarget&
void Terminal::CreateFromSettings(winrt::Microsoft::Terminal::Settings::ICoreSettings settings,
Microsoft::Console::Render::IRenderTarget& renderTarget)
{
const COORD viewportSize{ _ClampToShortMax(settings.InitialCols(), 1), _ClampToShortMax(settings.InitialRows(), 1) };
const COORD viewportSize{ Utils::ClampToShortMax(settings.InitialCols(), 1),
Utils::ClampToShortMax(settings.InitialRows(), 1) };
// TODO:MSFT:20642297 - Support infinite scrollback here, if HistorySize is -1
Create(viewportSize, _ClampToShortMax(settings.HistorySize(), 0), renderTarget);
Create(viewportSize, Utils::ClampToShortMax(settings.HistorySize(), 0), renderTarget);

UpdateSettings(settings);
}
Expand Down Expand Up @@ -499,11 +497,11 @@ void Terminal::_InitializeColorTable()
{
gsl::span<COLORREF> tableView = { &_colorTable[0], gsl::narrow<ptrdiff_t>(_colorTable.size()) };
// First set up the basic 256 colors
::Microsoft::Console::Utils::Initialize256ColorTable(tableView);
Utils::Initialize256ColorTable(tableView);
// Then use fill the first 16 values with the Campbell scheme
::Microsoft::Console::Utils::InitializeCampbellColorTable(tableView);
Utils::InitializeCampbellColorTable(tableView);
// Then make sure all the values have an alpha of 255
::Microsoft::Console::Utils::SetColorTableAlpha(tableView, 0xff);
Utils::SetColorTableAlpha(tableView, 0xff);
}

// Method Description:
Expand Down
12 changes: 8 additions & 4 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
#include "pch.h"
#include "AppHost.h"
#include "../types/inc/Viewport.hpp"
#include "../types/inc/Utils.hpp"

using namespace winrt::Windows::UI;
using namespace winrt::Windows::UI::Composition;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Hosting;
using namespace winrt::Windows::Foundation::Numerics;
using namespace ::Microsoft::Console;
using namespace ::Microsoft::Console::Types;

// The tabs are 34.8px tall. This is their default height - we're not
Expand Down Expand Up @@ -131,8 +133,10 @@ void AppHost::_HandleCreateWindow(const HWND hwnd, const RECT proposedRect)

auto initialSize = _app.GetLaunchDimensions(dpix);

const short _currentWidth = gsl::narrow<short>(ceil(initialSize.X));
const short _currentHeight = gsl::narrow<short>(ceil(initialSize.Y));
const short _currentWidth = Utils::ClampToShortMax(
static_cast<long>(ceil(initialSize.X)), 1);
const short _currentHeight = Utils::ClampToShortMax(
static_cast<long>(ceil(initialSize.Y)), 1);

// Create a RECT from our requested client size
auto nonClient = Viewport::FromDimensions({ _currentWidth,
Expand Down Expand Up @@ -172,8 +176,8 @@ void AppHost::_HandleCreateWindow(const HWND hwnd, const RECT proposedRect)

const COORD origin{ gsl::narrow<short>(proposedRect.left),
gsl::narrow<short>(proposedRect.top) };
const COORD dimensions{ gsl::narrow<short>(adjustedWidth),
gsl::narrow<short>(adjustedHeight) };
const COORD dimensions{ Utils::ClampToShortMax(adjustedWidth, 1),
Utils::ClampToShortMax(adjustedHeight, 1) };

const auto newPos = Viewport::FromDimensions(origin, dimensions);

Expand Down
2 changes: 2 additions & 0 deletions src/types/inc/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ namespace Microsoft::Console::Utils
{
bool IsValidHandle(const HANDLE handle) noexcept;

short ClampToShortMax(const long value, const short min);

std::wstring GuidToString(const GUID guid);
GUID GuidFromString(const std::wstring wstr);
GUID CreateGuid();
Expand Down
1 change: 1 addition & 0 deletions src/types/ut_types/Types.Unit.Tests.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<Project DefaultTargets="Build" ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$(SolutionDir)src\common.build.pre.props" />
<ItemGroup>
<ClCompile Include="UtilsTests.cpp" />
<ClCompile Include="UuidTests.cpp" />
<ClCompile Include="..\precomp.cpp">
<PrecompiledHeader>Create</PrecompiledHeader>
Expand Down
44 changes: 44 additions & 0 deletions src/types/ut_types/UtilsTests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"
#include "WexTestClass.h"
#include "..\..\inc\consoletaeftemplates.hpp"

#include "..\inc\utils.hpp"

using namespace WEX::Common;
using namespace WEX::Logging;
using namespace WEX::TestExecution;

using namespace Microsoft::Console::Utils;

class UtilsTests
{
TEST_CLASS(UtilsTests);

TEST_METHOD(TestClampToShortMax)
{
const short min = 1;

// Test outside the lower end of the range
const short minExpected = min;
auto minActual = ClampToShortMax(0, min);
VERIFY_ARE_EQUAL(minExpected, minActual);

// Test negative numbers
const short negativeExpected = min;
auto negativeActual = ClampToShortMax(-1, min);
VERIFY_ARE_EQUAL(negativeExpected, negativeActual);

// Test outside the upper end of the range
const short maxExpected = SHRT_MAX;
auto maxActual = ClampToShortMax(50000, min);
VERIFY_ARE_EQUAL(maxExpected, maxActual);

// Test within the range
const short withinRangeExpected = 100;
auto withinRangeActual = ClampToShortMax(withinRangeExpected, min);
VERIFY_ARE_EQUAL(withinRangeExpected, withinRangeActual);
}
};
1 change: 1 addition & 0 deletions src/types/ut_types/sources
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ DLLDEF =
SOURCES = \
$(SOURCES) \
UuidTests.cpp \
UtilsTests.cpp \
DefaultResource.rc \

INCLUDES = \
Expand Down
14 changes: 14 additions & 0 deletions src/types/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@

using namespace Microsoft::Console;

// Function Description:
// - Clamps a long in between `min` and `SHRT_MAX`
// Arguments:
// - value: the value to clamp
// - min: the minimum value to clamp to
// Return Value:
// - The clamped value as a short.
short Utils::ClampToShortMax(const long value, const short min)
{
return static_cast<short>(std::clamp(value,
static_cast<long>(min),
static_cast<long>(SHRT_MAX)));
}

// Function Description:
// - Creates a String representation of a guid, in the format
// "{12345678-ABCD-EF12-3456-7890ABCDEF12}"
Expand Down

0 comments on commit 7ede378

Please sign in to comment.