Skip to content

Commit

Permalink
Apply audit mode to TerminalInput/Adapter/Parser libraries (#4005)
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
- Enables auditing of Virtual Terminal libraries (input, adapter, parser)

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Rolls audit out to more things
* [x] I work here
* [x] Tests should still pass
* [x] Am core contributor
* [x] Closes #3957

<!-- 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
This is turning on the auditing of these projects (as enabled by the heavier lifting in the other refactor) and then cleaning up the remaining warnings.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
- [x] Built it
- [x] Ran the tests
  • Loading branch information
miniksa authored and msftbot[bot] committed Jan 3, 2020
1 parent f467422 commit 322989d
Show file tree
Hide file tree
Showing 36 changed files with 1,162 additions and 1,271 deletions.
11 changes: 6 additions & 5 deletions OpenConsole.sln
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,8 @@ Global
{2FD12FBB-1DDB-46D8-B818-1023C624CACA}.Release|x86.Build.0 = Release|Win32
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|x64.ActiveCfg = Release|x64
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|x64.Build.0 = Release|x64
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|x64.ActiveCfg = AuditMode|x64
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|x64.Build.0 = AuditMode|x64
{3AE13314-1939-4DFA-9C14-38CA0834050C}.AuditMode|x86.ActiveCfg = Release|Win32
{3AE13314-1939-4DFA-9C14-38CA0834050C}.Debug|Any CPU.ActiveCfg = Debug|Win32
{3AE13314-1939-4DFA-9C14-38CA0834050C}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand All @@ -372,7 +372,8 @@ Global
{3AE13314-1939-4DFA-9C14-38CA0834050C}.Release|x86.Build.0 = Release|Win32
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.ActiveCfg = Release|x64
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.ActiveCfg = AuditMode|x64
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.Build.0 = AuditMode|x64
{DCF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x86.ActiveCfg = Release|Win32
{DCF55140-EF6A-4736-A403-957E4F7430BB}.Debug|Any CPU.ActiveCfg = Debug|Win32
{DCF55140-EF6A-4736-A403-957E4F7430BB}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand All @@ -390,8 +391,8 @@ Global
{DCF55140-EF6A-4736-A403-957E4F7430BB}.Release|x86.Build.0 = Release|Win32
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.ActiveCfg = Release|x64
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.Build.0 = Release|x64
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.ActiveCfg = AuditMode|x64
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x64.Build.0 = AuditMode|x64
{1CF55140-EF6A-4736-A403-957E4F7430BB}.AuditMode|x86.ActiveCfg = Release|Win32
{1CF55140-EF6A-4736-A403-957E4F7430BB}.Debug|Any CPU.ActiveCfg = Debug|Win32
{1CF55140-EF6A-4736-A403-957E4F7430BB}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand Down
28 changes: 14 additions & 14 deletions src/cascadia/TerminalCore/TerminalDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ void TerminalDispatch::PrintString(const std::wstring_view string)
}

bool TerminalDispatch::CursorPosition(const size_t line,
const size_t column)
const size_t column) noexcept
{
const auto columnInBufferSpace = column - 1;
const auto lineInBufferSpace = line - 1;
Expand All @@ -40,33 +40,33 @@ bool TerminalDispatch::CursorPosition(const size_t line,
return _terminalApi.SetCursorPosition(x, y);
}

bool TerminalDispatch::CursorForward(const size_t distance)
bool TerminalDispatch::CursorForward(const size_t distance) noexcept
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X + gsl::narrow<short>(distance), cursorPos.Y };
return _terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
}

bool TerminalDispatch::CursorBackward(const size_t distance)
bool TerminalDispatch::CursorBackward(const size_t distance) noexcept
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X - gsl::narrow<short>(distance), cursorPos.Y };
return _terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
}

bool TerminalDispatch::CursorUp(const size_t distance)
bool TerminalDispatch::CursorUp(const size_t distance) noexcept
{
const auto cursorPos = _terminalApi.GetCursorPosition();
const COORD newCursorPos{ cursorPos.X, cursorPos.Y + gsl::narrow<short>(distance) };
return _terminalApi.SetCursorPosition(newCursorPos.X, newCursorPos.Y);
}

bool TerminalDispatch::EraseCharacters(const size_t numChars)
bool TerminalDispatch::EraseCharacters(const size_t numChars) noexcept
{
return _terminalApi.EraseCharacters(numChars);
}

bool TerminalDispatch::SetWindowTitle(std::wstring_view title)
bool TerminalDispatch::SetWindowTitle(std::wstring_view title) noexcept
{
return _terminalApi.SetWindowTitle(title);
}
Expand All @@ -79,12 +79,12 @@ bool TerminalDispatch::SetWindowTitle(std::wstring_view title)
// Return Value:
// True if handled successfully. False otherwise.
bool TerminalDispatch::SetColorTableEntry(const size_t tableIndex,
const DWORD color)
const DWORD color) noexcept
{
return _terminalApi.SetColorTableEntry(tableIndex, color);
}

bool TerminalDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle)
bool TerminalDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorStyle) noexcept
{
return _terminalApi.SetCursorStyle(cursorStyle);
}
Expand All @@ -95,7 +95,7 @@ bool TerminalDispatch::SetCursorStyle(const DispatchTypes::CursorStyle cursorSty
// - color: The new RGB color value to use, in 0x00BBGGRR form
// Return Value:
// True if handled successfully. False otherwise.
bool TerminalDispatch::SetDefaultForeground(const DWORD color)
bool TerminalDispatch::SetDefaultForeground(const DWORD color) noexcept
{
return _terminalApi.SetDefaultForeground(color);
}
Expand All @@ -106,7 +106,7 @@ bool TerminalDispatch::SetDefaultForeground(const DWORD color)
// - color: The new RGB color value to use, in 0x00BBGGRR form
// Return Value:
// True if handled successfully. False otherwise.
bool TerminalDispatch::SetDefaultBackground(const DWORD color)
bool TerminalDispatch::SetDefaultBackground(const DWORD color) noexcept
{
return _terminalApi.SetDefaultBackground(color);
}
Expand All @@ -117,7 +117,7 @@ bool TerminalDispatch::SetDefaultBackground(const DWORD color)
// - eraseType: the erase type (from beginning, to end, or all)
// Return Value:
// True if handled successfully. False otherwise.
bool TerminalDispatch::EraseInLine(const DispatchTypes::EraseType eraseType)
bool TerminalDispatch::EraseInLine(const DispatchTypes::EraseType eraseType) noexcept
{
return _terminalApi.EraseInLine(eraseType);
}
Expand All @@ -128,7 +128,7 @@ bool TerminalDispatch::EraseInLine(const DispatchTypes::EraseType eraseType)
// - count, the number of characters to delete
// Return Value:
// True if handled successfully. False otherwise.
bool TerminalDispatch::DeleteCharacter(const size_t count)
bool TerminalDispatch::DeleteCharacter(const size_t count) noexcept
{
return _terminalApi.DeleteCharacter(count);
}
Expand All @@ -139,7 +139,7 @@ bool TerminalDispatch::DeleteCharacter(const size_t count)
// - count, the number of spaces to add
// Return Value:
// True if handled successfully, false otherwise
bool TerminalDispatch::InsertCharacter(const size_t count)
bool TerminalDispatch::InsertCharacter(const size_t count) noexcept
{
return _terminalApi.InsertCharacter(count);
}
Expand All @@ -150,7 +150,7 @@ bool TerminalDispatch::InsertCharacter(const size_t count)
// - eraseType: the desired erase type
// Return Value:
// True if handled successfully. False otherwise
bool TerminalDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType)
bool TerminalDispatch::EraseInDisplay(const DispatchTypes::EraseType eraseType) noexcept
{
return _terminalApi.EraseInDisplay(eraseType);
}
30 changes: 15 additions & 15 deletions src/cascadia/TerminalCore/TerminalDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,27 @@ class TerminalDispatch : public Microsoft::Console::VirtualTerminal::TermDispatc
virtual void Print(const wchar_t wchPrintable) override;
virtual void PrintString(const std::wstring_view string) override;

bool SetGraphicsRendition(const std::basic_string_view<::Microsoft::Console::VirtualTerminal::DispatchTypes::GraphicsOptions> options) override;
bool SetGraphicsRendition(const std::basic_string_view<::Microsoft::Console::VirtualTerminal::DispatchTypes::GraphicsOptions> options) noexcept override;

virtual bool CursorPosition(const size_t line,
const size_t column) override; // CUP
const size_t column) noexcept override; // CUP

bool CursorForward(const size_t distance) override;
bool CursorBackward(const size_t distance) override;
bool CursorUp(const size_t distance) override;
bool CursorForward(const size_t distance) noexcept override;
bool CursorBackward(const size_t distance) noexcept override;
bool CursorUp(const size_t distance) noexcept override;

bool EraseCharacters(const size_t numChars) override;
bool SetWindowTitle(std::wstring_view title) override;
bool EraseCharacters(const size_t numChars) noexcept override;
bool SetWindowTitle(std::wstring_view title) noexcept override;

bool SetColorTableEntry(const size_t tableIndex, const DWORD color) override;
bool SetCursorStyle(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::CursorStyle cursorStyle) override;
bool SetColorTableEntry(const size_t tableIndex, const DWORD color) noexcept override;
bool SetCursorStyle(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::CursorStyle cursorStyle) noexcept override;

bool SetDefaultForeground(const DWORD color) override;
bool SetDefaultBackground(const DWORD color) override;
bool EraseInLine(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) override; // ED
bool DeleteCharacter(const size_t count) override;
bool InsertCharacter(const size_t count) override;
bool EraseInDisplay(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) override;
bool SetDefaultForeground(const DWORD color) noexcept override;
bool SetDefaultBackground(const DWORD color) noexcept override;
bool EraseInLine(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) noexcept override; // ED
bool DeleteCharacter(const size_t count) noexcept override;
bool InsertCharacter(const size_t count) noexcept override;
bool EraseInDisplay(const ::Microsoft::Console::VirtualTerminal::DispatchTypes::EraseType eraseType) noexcept override;

private:
::Microsoft::Terminal::Core::ITerminalApi& _terminalApi;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalCore/TerminalDispatchGraphics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ void TerminalDispatch::_SetGraphicsOptionHelper(const DispatchTypes::GraphicsOpt
}
}

bool TerminalDispatch::SetGraphicsRendition(const std::basic_string_view<DispatchTypes::GraphicsOptions> options)
bool TerminalDispatch::SetGraphicsRendition(const std::basic_string_view<DispatchTypes::GraphicsOptions> options) noexcept
{
bool success = false;
// Run through the graphics options and apply them
Expand Down
3 changes: 3 additions & 0 deletions src/inc/ITerminalOutputConnection.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,14 @@ namespace Microsoft::Console
class ITerminalOutputConnection
{
public:
#pragma warning(push)
#pragma warning(disable : 26432) // suppress rule of 5 violation on interface because tampering with this is fraught with peril
virtual ~ITerminalOutputConnection() = 0;

[[nodiscard]] virtual HRESULT WriteTerminalUtf8(const std::string_view str) = 0;
[[nodiscard]] virtual HRESULT WriteTerminalW(const std::wstring_view wstr) = 0;
};

inline Microsoft::Console::ITerminalOutputConnection::~ITerminalOutputConnection() {}
#pragma warning(pop)
}
3 changes: 3 additions & 0 deletions src/inc/LibraryIncludes.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@
// WRL
#include <wrl.h>

// TIL - Terminal Implementation Library
#include "til.h"

#pragma warning(pop)

// clang-format on
45 changes: 45 additions & 0 deletions src/inc/til.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
// The at function declares that you've already sufficiently checked that your array access
// is in range before retrieving an item inside it at an offset.
// This is to save double/triple/quadruple testing in circumstances where you are already
// pivoting on the length of a set and now want to pull elements out of it by offset
// without checking again.
// gsl::at will do the check again. As will .at(). And using [] will have a warning in audit.
template<class T>
constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
{
#pragma warning(suppress : 26482) // Suppress bounds.2 check for indexing with constant expressions
#pragma warning(suppress : 26446) // Suppress bounds.4 check for subscript operator.
return cont[i];
}
}

// These sit outside the namespace because they sit outside for WIL too.

// Inspired from RETURN_IF_WIN32_BOOL_FALSE
// WIL doesn't include a RETURN_BOOL_IF_FALSE, and RETURN_IF_WIN32_BOOL_FALSE
// will actually return the value of GLE.
#define RETURN_BOOL_IF_FALSE(b) \
do \
{ \
const bool __boolRet = wil::verify_bool(b); \
if (!__boolRet) \
{ \
return __boolRet; \
} \
} while (0, 0)

// Due to a bug (DevDiv 441931), Warning 4297 (function marked noexcept throws exception) is detected even when the throwing code is unreachable, such as the end of scope after a return, in function-level catch.
#define CATCH_LOG_RETURN_FALSE() \
catch (...) \
{ \
__pragma(warning(suppress : 4297)); \
LOG_CAUGHT_EXCEPTION(); \
return false; \
}
24 changes: 12 additions & 12 deletions src/terminal/adapter/DispatchCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@ bool DispatchCommon::s_ResizeWindow(ConGetSet& conApi,
SHORT sRows = 0;

// We should do nothing if 0 is passed in for a size.
bool fSuccess = SUCCEEDED(SizeTToShort(width, &sColumns)) &&
SUCCEEDED(SizeTToShort(height, &sRows)) &&
(width > 0 && height > 0);
bool success = SUCCEEDED(SizeTToShort(width, &sColumns)) &&
SUCCEEDED(SizeTToShort(height, &sRows)) &&
(width > 0 && height > 0);

if (fSuccess)
if (success)
{
CONSOLE_SCREEN_BUFFER_INFOEX csbiex = { 0 };
csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
fSuccess = !!conApi.GetConsoleScreenBufferInfoEx(csbiex);
success = conApi.GetConsoleScreenBufferInfoEx(csbiex);

if (fSuccess)
if (success)
{
const Viewport oldViewport = Viewport::FromInclusive(csbiex.srWindow);
const Viewport newViewport = Viewport::FromDimensions(oldViewport.Origin(),
Expand All @@ -51,20 +51,20 @@ bool DispatchCommon::s_ResizeWindow(ConGetSet& conApi,
}

// SetConsoleWindowInfo expect inclusive rects
SMALL_RECT sri = newViewport.ToInclusive();
const auto sri = newViewport.ToInclusive();

// SetConsoleScreenBufferInfoEx however expects exclusive rects
SMALL_RECT sre = newViewport.ToExclusive();
const auto sre = newViewport.ToExclusive();
csbiex.srWindow = sre;

fSuccess = conApi.SetConsoleScreenBufferInfoEx(csbiex);
if (fSuccess)
success = conApi.SetConsoleScreenBufferInfoEx(csbiex);
if (success)
{
fSuccess = conApi.SetConsoleWindowInfo(true, sri);
success = conApi.SetConsoleWindowInfo(true, sri);
}
}
}
return fSuccess;
return success;
}

// Routine Description:
Expand Down
3 changes: 3 additions & 0 deletions src/terminal/adapter/IInteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ namespace Microsoft::Console::VirtualTerminal
class IInteractDispatch
{
public:
#pragma warning(push)
#pragma warning(disable : 26432) // suppress rule of 5 violation on interface because tampering with this is fraught with peril
virtual ~IInteractDispatch() = default;
#pragma warning(pop)

virtual bool WriteInput(std::deque<std::unique_ptr<IInputEvent>>& inputEvents) = 0;

Expand Down
4 changes: 4 additions & 0 deletions src/terminal/adapter/ITermDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ namespace Microsoft::Console::VirtualTerminal
class Microsoft::Console::VirtualTerminal::ITermDispatch
{
public:
#pragma warning(push)
#pragma warning(disable : 26432) // suppress rule of 5 violation on interface because tampering with this is fraught with peril
virtual ~ITermDispatch() = 0;

virtual void Execute(const wchar_t wchControl) = 0;
virtual void Print(const wchar_t wchPrintable) = 0;
virtual void PrintString(const std::wstring_view string) = 0;
Expand Down Expand Up @@ -97,3 +100,4 @@ class Microsoft::Console::VirtualTerminal::ITermDispatch
const std::basic_string_view<size_t> parameters) = 0;
};
inline Microsoft::Console::VirtualTerminal::ITermDispatch::~ITermDispatch() {}
#pragma warning(pop)
2 changes: 1 addition & 1 deletion src/terminal/adapter/InteractDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ bool InteractDispatch::WindowManipulation(const DispatchTypes::WindowManipulatio
// the ConGetSet interface, that specifically handles a conpty resize.
if (parameters.size() == 2)
{
success = DispatchCommon::s_ResizeWindow(*_pConApi, parameters[1], parameters[0]);
success = DispatchCommon::s_ResizeWindow(*_pConApi, til::at(parameters, 1), til::at(parameters, 0));
if (success)
{
DispatchCommon::s_SuppressResizeRepaint(*_pConApi);
Expand Down
2 changes: 0 additions & 2 deletions src/terminal/adapter/InteractDispatch.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ namespace Microsoft::Console::VirtualTerminal
public:
InteractDispatch(std::unique_ptr<ConGetSet> pConApi);

~InteractDispatch() = default;

bool WriteInput(std::deque<std::unique_ptr<IInputEvent>>& inputEvents) override;
bool WriteCtrlC() override;
bool WriteString(const std::wstring_view string) override;
Expand Down
Loading

0 comments on commit 322989d

Please sign in to comment.