Skip to content

Commit

Permalink
Apply audit mode to TerminalConnection/Core/Settings and WinCon… (#4016)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
- Enables auditing of some Terminal libraries (Connection, Core, Settings)
- Also audit WinConPTY.LIB since Connection depends on it

## PR Checklist
* [x] Rolls audit out to more things
* [x] I work here
* [x] Tests should still pass
* [x] Am core contributor

## 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.

## Validation Steps Performed
- [x] Built it
- [x] Ran the tests
  • Loading branch information
@miniksa
miniksa authored Jan 3, 2020
1 parent 322989d commit d711d73
Show file tree
Hide file tree
Showing 37 changed files with 553 additions and 392 deletions.
14 changes: 8 additions & 6 deletions OpenConsole.sln
View file
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,8 @@ Global
{48D21369-3D7B-4431-9967-24E81292CF62}.Release|x86.Build.0 = Release|Win32
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|x64.ActiveCfg = Release|x64
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|x64.ActiveCfg = AuditMode|x64
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|x64.Build.0 = AuditMode|x64
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.AuditMode|x86.ActiveCfg = Release|Win32
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.Debug|Any CPU.ActiveCfg = Debug|Win32
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand All @@ -972,8 +973,8 @@ Global
{CA5CAD1A-C46D-4588-B1C0-40F31AE9100B}.Release|x86.Build.0 = Release|Win32
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|x64.ActiveCfg = Release|x64
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|x64.Build.0 = Release|x64
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|x64.ActiveCfg = AuditMode|x64
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|x64.Build.0 = AuditMode|x64
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.AuditMode|x86.ActiveCfg = Release|Win32
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.Debug|Any CPU.ActiveCfg = Debug|Win32
{CA5CAD1A-ABCD-429C-B551-8562EC954746}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand Down Expand Up @@ -1045,8 +1046,8 @@ Global
{CA5CAD1A-44BD-4AC7-AC72-F16E576FDD12}.Release|x86.Build.0 = Release|Win32
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|x64.ActiveCfg = Release|x64
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|x64.Build.0 = Release|x64
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|x64.ActiveCfg = AuditMode|x64
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|x64.Build.0 = AuditMode|x64
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.AuditMode|x86.ActiveCfg = Release|Win32
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.Debug|Any CPU.ActiveCfg = Debug|Win32
{CA5CAD1A-D7EC-4107-B7C6-79CB77AE2907}.Debug|ARM64.ActiveCfg = Debug|ARM64
Expand Down Expand Up @@ -1279,7 +1280,8 @@ Global
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|Any CPU.ActiveCfg = AuditMode|Win32
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|x64.ActiveCfg = Release|x64
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|x64.ActiveCfg = AuditMode|x64
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|x64.Build.0 = AuditMode|x64
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|x86.ActiveCfg = AuditMode|Win32
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.AuditMode|x86.Build.0 = AuditMode|Win32
{58A03BB2-DF5A-4B66-91A0-7EF3BA01269A}.Debug|Any CPU.ActiveCfg = Debug|Win32
Expand Down
24 changes: 12 additions & 12 deletions src/buffer/out/textBuffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -809,23 +809,23 @@ void TextBuffer::Reset()
// - newSize - new size of screen.
// Return Value:
// - Success if successful. Invalid parameter if screen buffer size is unexpected. No memory if allocation failed.
[[nodiscard]] NTSTATUS TextBuffer::ResizeTraditional(const COORD newSize)
[[nodiscard]] NTSTATUS TextBuffer::ResizeTraditional(const COORD newSize) noexcept
{
RETURN_HR_IF(E_INVALIDARG, newSize.X < 0 || newSize.Y < 0);

const auto currentSize = GetSize().Dimensions();
const auto attributes = GetCurrentAttributes();

SHORT TopRow = 0; // new top row of the screen buffer
if (newSize.Y <= GetCursor().GetPosition().Y)
{
TopRow = GetCursor().GetPosition().Y - newSize.Y + 1;
}
const SHORT TopRowIndex = (GetFirstRowIndex() + TopRow) % currentSize.Y;

// rotate rows until the top row is at index 0
try
{
const auto currentSize = GetSize().Dimensions();
const auto attributes = GetCurrentAttributes();

SHORT TopRow = 0; // new top row of the screen buffer
if (newSize.Y <= GetCursor().GetPosition().Y)
{
TopRow = GetCursor().GetPosition().Y - newSize.Y + 1;
}
const SHORT TopRowIndex = (GetFirstRowIndex() + TopRow) % currentSize.Y;

// rotate rows until the top row is at index 0
const ROW& newTopRow = _storage.at(TopRowIndex);
while (&newTopRow != &_storage.front())
{
Expand Down
2 changes: 1 addition & 1 deletion src/buffer/out/textBuffer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class TextBuffer final

void Reset();

[[nodiscard]] HRESULT ResizeTraditional(const COORD newSize);
[[nodiscard]] HRESULT ResizeTraditional(const COORD newSize) noexcept;

const UnicodeStorage& GetUnicodeStorage() const noexcept;
UnicodeStorage& GetUnicodeStorage() noexcept;
Expand Down
4 changes: 1 addition & 3 deletions src/cascadia/TerminalConnection/AzureClientID.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,4 @@

#pragma once

#include <cpprest/details/web_utilities.h>

const utility::string_t AzureClientID = U("0");
static constexpr std::wstring_view AzureClientID = L"0";
27 changes: 17 additions & 10 deletions src/cascadia/TerminalConnection/AzureConnection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,17 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
// This function exists because the clientID only gets added by the release pipelines
// and is not available on local builds, so we want to be able to make sure we don't
// try to make an Azure connection if its a local build
bool AzureConnection::IsAzureConnectionAvailable()
bool AzureConnection::IsAzureConnectionAvailable() noexcept
{
return (AzureClientID != L"0");
}

AzureConnection::AzureConnection(const uint32_t initialRows, const uint32_t initialCols) :
_initialRows{ initialRows },
_initialCols{ initialCols }
_initialCols{ initialCols },
_maxStored{},
_maxSize{},
_expiry{}
{
}

Expand Down Expand Up @@ -186,7 +189,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
const auto str = winrt::to_string(data);
msg.set_utf8_message(str);

_cloudShellSocket.send(msg);
_cloudShellSocket.send(msg).get();
}
default:
return;
Expand Down Expand Up @@ -217,7 +220,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
terminalRequest.set_body(json::value(L""));

// Send the request
_RequestHelper(terminalClient, terminalRequest);
const auto response = _RequestHelper(terminalClient, terminalRequest);
}
}

Expand Down Expand Up @@ -269,8 +272,12 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
// - the exit code of the thread
DWORD WINAPI AzureConnection::StaticOutputThreadProc(LPVOID lpParameter)
{
AzureConnection* const pInstance = (AzureConnection*)lpParameter;
return pInstance->_OutputThread();
AzureConnection* const pInstance = static_cast<AzureConnection*>(lpParameter);
if (pInstance)
{
return pInstance->_OutputThread();
}
return gsl::narrow_cast<DWORD>(E_INVALIDARG);
}

// Method description:
Expand Down Expand Up @@ -703,7 +710,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
http_request commonRequest(L"POST");
commonRequest.set_request_uri(L"common/oauth2/devicecode");
const auto body = L"client_id=" + AzureClientID + L"&resource=" + _wantedResource;
commonRequest.set_body(body, L"application/x-www-form-urlencoded");
commonRequest.set_body(body.c_str(), L"application/x-www-form-urlencoded");

// Send the request and receive the response as a json value
return _RequestHelper(loginClient, commonRequest);
Expand All @@ -724,7 +731,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
http_client pollingClient(_loginUri);

// Continuously send a poll request until the user authenticates
const auto body = L"grant_type=device_code&resource=" + _wantedResource + L"&client_id=" + AzureClientID + L"&code=" + deviceCode;
const auto body = hstring() + L"grant_type=device_code&resource=" + _wantedResource + L"&client_id=" + AzureClientID + L"&code=" + deviceCode;
const auto requestUri = L"common/oauth2/token";
json::value responseJson;
for (int count = 0; count < expiresIn / pollInterval; count++)
Expand All @@ -736,7 +743,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
}
http_request pollRequest(L"POST");
pollRequest.set_request_uri(requestUri);
pollRequest.set_body(body, L"application/x-www-form-urlencoded");
pollRequest.set_body(body.c_str(), L"application/x-www-form-urlencoded");

responseJson = _RequestHelper(pollingClient, pollRequest);

Expand Down Expand Up @@ -789,7 +796,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
http_request refreshRequest(L"POST");
refreshRequest.set_request_uri(_tenantID + L"/oauth2/token");
const auto body = L"client_id=" + AzureClientID + L"&resource=" + _wantedResource + L"&grant_type=refresh_token" + L"&refresh_token=" + _refreshToken;
refreshRequest.set_body(body, L"application/x-www-form-urlencoded");
refreshRequest.set_body(body.c_str(), L"application/x-www-form-urlencoded");
refreshRequest.headers().add(L"User-Agent", HttpUserAgent);

// Send the request and return the response as a json value
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/AzureConnection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
{
struct AzureConnection : AzureConnectionT<AzureConnection>, ConnectionStateHolder<AzureConnection>
{
static bool IsAzureConnectionAvailable();
static bool IsAzureConnectionAvailable() noexcept;
AzureConnection(const uint32_t rows, const uint32_t cols);

void Start();
Expand Down
12 changes: 11 additions & 1 deletion src/cascadia/TerminalConnection/ConnectionStateHolder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
TYPED_EVENT(StateChanged, ITerminalConnection, winrt::Windows::Foundation::IInspectable);

protected:
#pragma warning(push)
#pragma warning(disable : 26447) // Analyzer is still upset about noexcepts throwing even with function level try.
// Method Description:
// - Attempt to transition to and signal the specified connection state.
// The transition will only be effected if the state is "beyond" the current state.
Expand All @@ -21,6 +23,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
// Return Value:
// Whether we've successfully transitioned to the new state.
bool _transitionToState(const ConnectionState state) noexcept
try
{
{
std::lock_guard<std::mutex> stateLock{ _stateMutex };
Expand All @@ -32,9 +35,11 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
_connectionState = state;
}
// Dispatch the event outside of lock.
#pragma warning(suppress : 26491) // We can't avoid static_cast downcast because this is template magic.
_StateChangedHandlers(*static_cast<T*>(this), nullptr);
return true;
}
CATCH_FAIL_FAST()

// Method Description:
// - Returns whether the state is one of the N specified states.
Expand All @@ -43,7 +48,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
// Return Value:
// Whether we're in one of the states.
template<typename... Args>
bool _isStateOneOf(Args&&... args) const noexcept
bool _isStateOneOf(const Args&&... args) const noexcept
try
{
// Dark magic! This function uses C++17 fold expressions. These fold expressions roughly expand as follows:
// (... OP (expression_using_args))
Expand All @@ -56,6 +62,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
std::lock_guard<std::mutex> stateLock{ _stateMutex };
return (... || (_connectionState == args));
}
CATCH_FAIL_FAST()

// Method Description:
// - Returns whether the state has reached or surpassed the specified state.
Expand All @@ -64,10 +71,13 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
// Return Value:
// Whether we're at or beyond the specified state
bool _isStateAtOrBeyond(const ConnectionState state) const noexcept
try
{
std::lock_guard<std::mutex> stateLock{ _stateMutex };
return _connectionState >= state;
}
CATCH_FAIL_FAST()
#pragma warning(pop)

// Method Description:
// - (Convenience:) Returns whether we're "connected".
Expand Down
Loading

0 comments on commit d711d73

Please sign in to comment.