Skip to content

Commit

Permalink
Fix SetColorTableEntry off-by-one error (#3938)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Uses the verification in `at` to ensure the index is correct (as @j4james suggests). If `at` throws, then returns false.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #3720
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] ~~I've~~ discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #3720

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Can no longer repro the issue after the fix.
  • Loading branch information
mkitzan authored and msftbot[bot] committed Dec 14, 2019
1 parent a3f3cc8 commit 19babc0
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 10 deletions.
15 changes: 9 additions & 6 deletions src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -382,15 +382,18 @@ bool Terminal::SetWindowTitle(std::wstring_view title)
// - true iff we successfully updated the color table entry.
bool Terminal::SetColorTableEntry(const size_t tableIndex, const COLORREF dwColor)
{
if (tableIndex > _colorTable.size())
try
{
_colorTable.at(tableIndex) = dwColor;

// Repaint everything - the colors might have changed
_buffer->GetRenderTarget().TriggerRedrawAll();
return true;
}
catch (std::out_of_range&)
{
return false;
}
_colorTable.at(tableIndex) = dwColor;

// Repaint everything - the colors might have changed
_buffer->GetRenderTarget().TriggerRedrawAll();
return true;
}

// Method Description:
Expand Down
41 changes: 41 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#include "precomp.h"
#include <WexTestClass.h>

#include "../cascadia/TerminalCore/Terminal.hpp"
#include "MockTermSettings.h"
#include "../renderer/inc/DummyRenderTarget.hpp"
#include "consoletaeftemplates.hpp"

using namespace winrt::Microsoft::Terminal::Settings;
using namespace Microsoft::Terminal::Core;

namespace TerminalCoreUnitTests
{
#define WCS(x) WCSHELPER(x)
#define WCSHELPER(x) L#x

class TerminalApiTest
{
TEST_CLASS(TerminalApiTest);

TEST_METHOD(SetColorTableEntry)
{
Terminal term;
DummyRenderTarget emptyRT;
term.Create({ 100, 100 }, 0, emptyRT);

auto settings = winrt::make<MockTermSettings>(100, 100, 100);
term.UpdateSettings(settings);

VERIFY_IS_TRUE(term.SetColorTableEntry(0, 100));
VERIFY_IS_TRUE(term.SetColorTableEntry(128, 100));
VERIFY_IS_TRUE(term.SetColorTableEntry(255, 100));

VERIFY_IS_FALSE(term.SetColorTableEntry(256, 100));
VERIFY_IS_FALSE(term.SetColorTableEntry(512, 100));
}
};
}
7 changes: 3 additions & 4 deletions src/cascadia/UnitTests_TerminalCore/UnitTests.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,18 @@
<RootNamespace>TerminalCoreUnitTests</RootNamespace>
<ProjectName>UnitTests_TerminalCore</ProjectName>
<TargetName>Terminal.Core.Unit.Tests</TargetName>
<ConfigurationType>DynamicLibrary</ConfigurationType>
<ConfigurationType>DynamicLibrary</ConfigurationType>
</PropertyGroup>

<Import Project="$(SolutionDir)\common.openconsole.props" Condition="'$(OpenConsoleDir)'==''" />
<Import Project="$(SolutionDir)\src\common.build.pre.props" />

<ItemGroup>
<ClCompile Include="ScreenSizeLimitsTest.cpp" />
<ClCompile Include="SelectionTest.cpp" />
<ClCompile Include="InputTest.cpp" />
<ClCompile Include="precomp.cpp">
<PrecompiledHeader>Create</PrecompiledHeader>
</ClCompile>
<ClCompile Include="TerminalApiTest.cpp" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\buffer\out\lib\bufferout.vcxproj">
Expand Down Expand Up @@ -56,4 +55,4 @@
<!-- Careful reordering these. Some default props (contained in these files) are order sensitive. -->
<Import Project="$(SolutionDir)src\common.build.post.props" />
<Import Project="$(SolutionDir)src\common.build.tests.props" />
</Project>
</Project>

0 comments on commit 19babc0

Please sign in to comment.