From 853786ae42198e4fa682dad48d6b2e691d143f8c Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 9 Jun 2021 13:36:32 -0700 Subject: [PATCH 01/18] Make the inbox console wait/stay open forever. --- src/server/IoDispatchers.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index c6009e9d4b1..b87eaf06f92 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -305,6 +305,9 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API // Unlock in case anything tries to spool down as we exit. UnlockConsole(); + // TODO: This works TECHNICALLY but we should likely clean up more stuff. + Sleep(INFINITE); + // We've handed off responsibility. Exit process to clean up any outstanding things we have open. ExitProcess(S_OK); } From f673bd2338260404a6f3b9f1aecd0524f43d1519 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 9 Jun 2021 13:59:02 -0700 Subject: [PATCH 02/18] Make OS wait on open one indefinitely. --- src/host/exe/CConsoleHandoff.cpp | 17 ++++++++++++++++- src/host/exe/CConsoleHandoff.h | 3 ++- src/host/proxy/IConsoleHandoff.idl | 5 +++-- src/server/IoDispatchers.cpp | 11 +++++++---- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/host/exe/CConsoleHandoff.cpp b/src/host/exe/CConsoleHandoff.cpp index 20ac3c6e806..dba77930db6 100644 --- a/src/host/exe/CConsoleHandoff.cpp +++ b/src/host/exe/CConsoleHandoff.cpp @@ -27,11 +27,15 @@ static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) // - server - Console driver server handle // - inputEvent - Event already established that we signal when new input data is available in case the driver is waiting on us // - msg - Portable attach message containing just enough descriptor payload to get us started in servicing it +// - process - Handle to our process for waiting for us to exit HRESULT CConsoleHandoff::EstablishHandoff(HANDLE server, HANDLE inputEvent, - PCCONSOLE_PORTABLE_ATTACH_MSG msg) + PCCONSOLE_PORTABLE_ATTACH_MSG msg, + HANDLE* process) try { + RETURN_HR_IF(E_INVALIDARG, !process); + // Fill the descriptor portion of a fresh api message with the received data. // The descriptor portion is the "received" packet from the last ask of the driver. // The other portions are unnecessary as they track the other buffer state, error codes, @@ -57,6 +61,17 @@ try // Now perform the handoff. RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, &apiMsg)); + // Give back a copy of our own process handle to be tracked. + wil::unique_handle duplicatedHandle; + RETURN_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), + GetCurrentProcess(), + GetCurrentProcess(), + &duplicatedHandle, + SYNCHRONIZE, + FALSE, + 0)); + *process = duplicatedHandle.release(); + return S_OK; } CATCH_RETURN(); diff --git a/src/host/exe/CConsoleHandoff.h b/src/host/exe/CConsoleHandoff.h index e2d2e14d890..7af4f12ee60 100644 --- a/src/host/exe/CConsoleHandoff.h +++ b/src/host/exe/CConsoleHandoff.h @@ -34,7 +34,8 @@ struct __declspec(uuid(__CLSID_CConsoleHandoff)) #pragma region IConsoleHandoff STDMETHODIMP EstablishHandoff(HANDLE server, HANDLE inputEvent, - PCCONSOLE_PORTABLE_ATTACH_MSG msg); + PCCONSOLE_PORTABLE_ATTACH_MSG msg, + HANDLE* process); #pragma endregion }; diff --git a/src/host/proxy/IConsoleHandoff.idl b/src/host/proxy/IConsoleHandoff.idl index 067eb0efa83..639f5a9f52e 100644 --- a/src/host/proxy/IConsoleHandoff.idl +++ b/src/host/proxy/IConsoleHandoff.idl @@ -20,11 +20,12 @@ typedef const CONSOLE_PORTABLE_ATTACH_MSG* PCCONSOLE_PORTABLE_ATTACH_MSG; [ object, - uuid(2B607BC1-43EB-40C3-95AE-2856ADDB7F23) + uuid(E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4) ] interface IConsoleHandoff : IUnknown { HRESULT EstablishHandoff([in, system_handle(sh_file)] HANDLE server, [in, system_handle(sh_event)] HANDLE inputEvent, - [in, ref] PCCONSOLE_PORTABLE_ATTACH_MSG msg); + [in, ref] PCCONSOLE_PORTABLE_ATTACH_MSG msg, + [out, system_handle(sh_process)] HANDLE* process); }; diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index b87eaf06f92..760ea1d5ffb 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -296,19 +296,22 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API HANDLE serverHandle; THROW_IF_FAILED(Globals.pDeviceComm->GetServerHandle(&serverHandle)); + wil::unique_process_handle clientProcess; + // Okay, moment of truth! If they say they successfully took it over, we're going to clean up. // If they fail, we'll throw here and it'll log and we'll just start normally. THROW_IF_FAILED(handoff->EstablishHandoff(serverHandle, Globals.hInputEvent.get(), - &msg)); + &msg, + &clientProcess)); // Unlock in case anything tries to spool down as we exit. UnlockConsole(); - // TODO: This works TECHNICALLY but we should likely clean up more stuff. - Sleep(INFINITE); + // We've handed off responsibility. Wait for child process to exit so we can maintain PID continuity for some clients. + WaitForSingleObject(clientProcess.get(), INFINITE); - // We've handed off responsibility. Exit process to clean up any outstanding things we have open. + // Exit process to clean up any outstanding things we have open. ExitProcess(S_OK); } CATCH_LOG(); // Just log, don't do anything more. We'll move on to launching normally on failure. From aab183b77be0c88a6386cff0dd25ca357dc89942 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 9 Jun 2021 14:22:15 -0700 Subject: [PATCH 03/18] finish interface re-id --- src/cascadia/CascadiaPackage/Package-Dev.appxmanifest | 2 +- src/cascadia/CascadiaPackage/Package-Pre.appxmanifest | 2 +- src/cascadia/CascadiaPackage/Package.appxmanifest | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cascadia/CascadiaPackage/Package-Dev.appxmanifest b/src/cascadia/CascadiaPackage/Package-Dev.appxmanifest index fe8d0bb6fa7..f73f4c62f48 100644 --- a/src/cascadia/CascadiaPackage/Package-Dev.appxmanifest +++ b/src/cascadia/CascadiaPackage/Package-Dev.appxmanifest @@ -98,7 +98,7 @@ - + diff --git a/src/cascadia/CascadiaPackage/Package-Pre.appxmanifest b/src/cascadia/CascadiaPackage/Package-Pre.appxmanifest index edb05aa7bc3..1c493b7edd7 100644 --- a/src/cascadia/CascadiaPackage/Package-Pre.appxmanifest +++ b/src/cascadia/CascadiaPackage/Package-Pre.appxmanifest @@ -99,7 +99,7 @@ - + diff --git a/src/cascadia/CascadiaPackage/Package.appxmanifest b/src/cascadia/CascadiaPackage/Package.appxmanifest index 34c943b3749..ad4472156e1 100644 --- a/src/cascadia/CascadiaPackage/Package.appxmanifest +++ b/src/cascadia/CascadiaPackage/Package.appxmanifest @@ -99,7 +99,7 @@ - + --> From fd1333dcb3099c0e529eaec966f7e028027d79bc Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Thu, 10 Jun 2021 16:21:17 -0700 Subject: [PATCH 04/18] Well... this madness builds. --- src/host/HostSignalInputThread.cpp | 270 ++++++++++++++++++ src/host/HostSignalInputThread.hpp | 49 ++++ src/host/exe/CConsoleHandoff.cpp | 4 +- src/host/exe/CConsoleHandoff.h | 1 + src/host/lib/hostlib.vcxproj | 8 +- src/host/lib/hostlib.vcxproj.filters | 6 + src/host/proxy/IConsoleHandoff.idl | 1 + src/host/sources.inc | 1 + src/host/srvinit.cpp | 5 + src/host/srvinit.h | 1 + src/inc/HostSignals.hpp | 27 ++ .../base/RemoteConsoleControl.cpp | 89 ++++++ .../base/RemoteConsoleControl.hpp | 33 +++ src/interactivity/base/ServiceLocator.cpp | 21 ++ .../base/lib/InteractivityBase.vcxproj | 10 +- .../lib/InteractivityBase.vcxproj.filters | 24 +- src/interactivity/base/sources.inc | 3 +- src/interactivity/inc/ServiceLocator.hpp | 1 + src/interactivity/win32/ConsoleControl.hpp | 2 +- src/server/IoDispatchers.cpp | 14 + 20 files changed, 553 insertions(+), 17 deletions(-) create mode 100644 src/host/HostSignalInputThread.cpp create mode 100644 src/host/HostSignalInputThread.hpp create mode 100644 src/inc/HostSignals.hpp create mode 100644 src/interactivity/base/RemoteConsoleControl.cpp create mode 100644 src/interactivity/base/RemoteConsoleControl.hpp diff --git a/src/host/HostSignalInputThread.cpp b/src/host/HostSignalInputThread.cpp new file mode 100644 index 00000000000..17cdec2871a --- /dev/null +++ b/src/host/HostSignalInputThread.cpp @@ -0,0 +1,270 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" + +#include "HostSignalInputThread.hpp" +#include "../inc/HostSignals.hpp" + +#include "output.h" +#include "handle.h" +#include "../interactivity/inc/ServiceLocator.hpp" +#include "../terminal/adapter/DispatchCommon.hpp" + +using namespace Microsoft::Console; +using namespace Microsoft::Console::Interactivity; +using namespace Microsoft::Console::VirtualTerminal; + +// Constructor Description: +// - Creates the PTY Signal Input Thread. +// Arguments: +// - hPipe - a handle to the file representing the read end of the Host Signal pipe. +HostSignalInputThread::HostSignalInputThread(_In_ wil::unique_hfile hPipe) : + _hFile{ std::move(hPipe) }, + _hThread{}, + _dwThreadId{ 0 }, + _consoleConnected{ false } +{ + THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); +} + +HostSignalInputThread::~HostSignalInputThread() +{ + // Manually terminate our thread during unittesting. Otherwise, the test + // will finish, but TAEF will not manually kill the test. +#ifdef UNIT_TESTING + TerminateThread(_hThread.get(), 0); +#endif +} + +// Function Description: +// - Static function used for initializing an instance's ThreadProc. +// Arguments: +// - lpParameter - A pointer to the HostSignalInputThread instance that should be called. +// Return Value: +// - The return value of the underlying instance's _InputThread +DWORD WINAPI HostSignalInputThread::StaticThreadProc(_In_ LPVOID lpParameter) +{ + HostSignalInputThread* const pInstance = reinterpret_cast(lpParameter); + return pInstance->_InputThread(); +} + +// Method Description: +// - Tell us that there's a client attached to the console, so we can actually +// do something with the messages we receive now. Before this is set, there +// is no guarantee that a client has attached, so most parts of the console +// (in and screen buffers) haven't yet been initialized. +// Arguments: +// - +// Return Value: +// - +void HostSignalInputThread::ConnectConsole() noexcept +{ + _consoleConnected = true; +} + +// Method Description: +// - The ThreadProc for the Host Signal Input Thread. +// Return Value: +// - S_OK if the thread runs to completion. +// - Otherwise it may cause an application termination another route and never return. +[[nodiscard]] HRESULT HostSignalInputThread::_InputThread() +{ + unsigned short signalId; + while (_GetData(&signalId, sizeof(signalId))) + { + switch (signalId) + { + case HOST_SIGNAL_NOTIFY_APP: + { + HOST_SIGNAL_NOTIFY_APP_DATA msg = { 0 }; + if (_GetData(&msg, sizeof(msg))) + { + if (msg.cbSize >= sizeof(msg)) + { + _AdvanceReader(msg.cbSize - sizeof(msg)); + } + else + { + THROW_HR(E_ILLEGAL_METHOD_CALL); + } + + LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.dwProcessId)); + } + else + { + return E_ABORT; + } + + break; + } + case HOST_SIGNAL_SET_FOREGROUND: + { + HOST_SIGNAL_SET_FOREGROUND_DATA msg = { 0 }; + if (_GetData(&msg, sizeof(msg))) + { + if (msg.cbSize >= sizeof(msg)) + { + _AdvanceReader(msg.cbSize - sizeof(msg)); + } + else + { + THROW_HR(E_ILLEGAL_METHOD_CALL); + } + + LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.dwProcessId), msg.fForeground)); + } + else + { + return E_ABORT; + } + break; + } + case HOST_SIGNAL_END_TASK: + { + HOST_SIGNAL_END_TASK_DATA msg = { 0 }; + if (_GetData(&msg, sizeof(msg))) + { + if (msg.cbSize >= sizeof(msg)) + { + _AdvanceReader(msg.cbSize - sizeof(msg)); + } + else + { + THROW_HR(E_ILLEGAL_METHOD_CALL); + } + + LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.dwProcessId), msg.dwEventType, msg.ulCtrlFlags)); + } + else + { + return E_ABORT; + } + break; + } + default: + { + THROW_HR(E_UNEXPECTED); + break; + } + } + } + return S_OK; +} + +// Method Description: +// - Skips the file stream forward by the specified number of bytes.zs +// Arguments: +// - cbBytes - Count of bytes to skip forward +// Return Value: +// - True if we could skip forward successfully. False otherwise. +bool HostSignalInputThread::_AdvanceReader(const DWORD cbBytes) +{ + std::array buffer; + auto bytesRemaining = cbBytes; + + while (bytesRemaining > 0) + { + const auto advance = std::min(bytesRemaining, gsl::narrow_cast(buffer.max_size())); + + if (!_GetData(buffer.data(), advance)) + { + return false; + } + + bytesRemaining -= advance; + } + + return true; +} + +// Method Description: +// - Retrieves bytes from the file stream and exits or throws errors should the pipe state +// be compromised. +// Arguments: +// - pBuffer - Buffer to fill with data. +// - cbBuffer - Count of bytes in the given buffer. +// Return Value: +// - True if data was retrieved successfully. False otherwise. +bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, + const DWORD cbBuffer) +{ + DWORD dwRead = 0; + // If we failed to read because the terminal broke our pipe (usually due + // to dying itself), close gracefully with ERROR_BROKEN_PIPE. + // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that + // we want to gracefully close in. + if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) + { + DWORD lastError = GetLastError(); + if (lastError == ERROR_BROKEN_PIPE) + { + _Shutdown(); + return false; + } + else + { + THROW_WIN32(lastError); + } + } + else if (dwRead != cbBuffer) + { + _Shutdown(); + return false; + } + + return true; +} + +// Method Description: +// - Starts the PTY Signal input thread. +[[nodiscard]] HRESULT HostSignalInputThread::Start() noexcept +{ + RETURN_LAST_ERROR_IF(!_hFile); + + HANDLE hThread = nullptr; + // 0 is the right value, https://blogs.msdn.microsoft.com/oldnewthing/20040223-00/?p=40503 + DWORD dwThreadId = 0; + + hThread = CreateThread(nullptr, + 0, + HostSignalInputThread::StaticThreadProc, + this, + 0, + &dwThreadId); + + RETURN_LAST_ERROR_IF_NULL(hThread); + _hThread.reset(hThread); + _dwThreadId = dwThreadId; + LOG_IF_FAILED(SetThreadDescription(hThread, L"Host Signal Handler Thread")); + + return S_OK; +} + +// Method Description: +// - Perform a shutdown of the console. This happens when the signal pipe is +// broken, which means either the parent terminal process has died, or they +// called ClosePseudoConsole. +// CloseConsoleProcessState is not enough by itself - it will disconnect +// clients as if the X was pressed, but then we need to actually still die, +// so then call RundownAndExit. +// Arguments: +// - +// Return Value: +// - +void HostSignalInputThread::_Shutdown() +{ + // Trigger process shutdown. + CloseConsoleProcessState(); + + // If we haven't terminated by now, that's because there's a client that's still attached. + // Force the handling of the control events by the attached clients. + // As of MSFT:19419231, CloseConsoleProcessState will make sure this + // happens if this method is called outside of lock, but if we're + // currently locked, we want to make sure ctrl events are handled + // _before_ we RundownAndExit. + ProcessCtrlEvents(); + + // Make sure we terminate. + ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); +} diff --git a/src/host/HostSignalInputThread.hpp b/src/host/HostSignalInputThread.hpp new file mode 100644 index 00000000000..334eddcdee7 --- /dev/null +++ b/src/host/HostSignalInputThread.hpp @@ -0,0 +1,49 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- HostSignalInputThread.hpp + +Abstract: +- Defines methods that wrap the thread that will wait for signals + from a delegated console host to this "owner" console. + +Author(s): +- Michael Niksa (miniksa) 10 Jun 2021 + +Notes: +- Sourced from `PtySignalInputThread` +--*/ +#pragma once + +namespace Microsoft::Console +{ + class HostSignalInputThread final + { + public: + HostSignalInputThread(_In_ wil::unique_hfile hPipe); + ~HostSignalInputThread(); + + [[nodiscard]] HRESULT Start() noexcept; + static DWORD WINAPI StaticThreadProc(_In_ LPVOID lpParameter); + + // Prevent copying and assignment. + HostSignalInputThread(const HostSignalInputThread&) = delete; + HostSignalInputThread& operator=(const HostSignalInputThread&) = delete; + + void ConnectConsole() noexcept; + + private: + [[nodiscard]] HRESULT _InputThread(); + + bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); + bool _AdvanceReader(const DWORD cbBytes); + void _Shutdown(); + + wil::unique_hfile _hFile; + wil::unique_handle _hThread; + DWORD _dwThreadId; + bool _consoleConnected; + }; +} diff --git a/src/host/exe/CConsoleHandoff.cpp b/src/host/exe/CConsoleHandoff.cpp index dba77930db6..09262edac52 100644 --- a/src/host/exe/CConsoleHandoff.cpp +++ b/src/host/exe/CConsoleHandoff.cpp @@ -31,6 +31,7 @@ static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) HRESULT CConsoleHandoff::EstablishHandoff(HANDLE server, HANDLE inputEvent, PCCONSOLE_PORTABLE_ATTACH_MSG msg, + HANDLE signalPipe, HANDLE* process) try { @@ -57,9 +58,10 @@ try // Making our own duplicate copy ensures they hang around in our lifetime. RETURN_IF_FAILED(_duplicateHandle(server, server)); RETURN_IF_FAILED(_duplicateHandle(inputEvent, inputEvent)); + RETURN_IF_FAILED(_duplicateHandle(signalPipe, signalPipe)); // Now perform the handoff. - RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, &apiMsg)); + RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, signalPipe, &apiMsg)); // Give back a copy of our own process handle to be tracked. wil::unique_handle duplicatedHandle; diff --git a/src/host/exe/CConsoleHandoff.h b/src/host/exe/CConsoleHandoff.h index 7af4f12ee60..05e056e913b 100644 --- a/src/host/exe/CConsoleHandoff.h +++ b/src/host/exe/CConsoleHandoff.h @@ -35,6 +35,7 @@ struct __declspec(uuid(__CLSID_CConsoleHandoff)) STDMETHODIMP EstablishHandoff(HANDLE server, HANDLE inputEvent, PCCONSOLE_PORTABLE_ATTACH_MSG msg, + HANDLE signalPipe, HANDLE* process); #pragma endregion diff --git a/src/host/lib/hostlib.vcxproj b/src/host/lib/hostlib.vcxproj index fb83977af9c..d900c0453b9 100644 --- a/src/host/lib/hostlib.vcxproj +++ b/src/host/lib/hostlib.vcxproj @@ -8,9 +8,15 @@ ConhostV2Lib StaticLibrary + + + + + + - + \ No newline at end of file diff --git a/src/host/lib/hostlib.vcxproj.filters b/src/host/lib/hostlib.vcxproj.filters index e8877f96c70..59c4da62cd9 100644 --- a/src/host/lib/hostlib.vcxproj.filters +++ b/src/host/lib/hostlib.vcxproj.filters @@ -183,6 +183,9 @@ Source Files + + Source Files + @@ -356,6 +359,9 @@ Header Files + + Header Files + diff --git a/src/host/proxy/IConsoleHandoff.idl b/src/host/proxy/IConsoleHandoff.idl index 639f5a9f52e..391bc726fde 100644 --- a/src/host/proxy/IConsoleHandoff.idl +++ b/src/host/proxy/IConsoleHandoff.idl @@ -26,6 +26,7 @@ typedef const CONSOLE_PORTABLE_ATTACH_MSG* PCCONSOLE_PORTABLE_ATTACH_MSG; HRESULT EstablishHandoff([in, system_handle(sh_file)] HANDLE server, [in, system_handle(sh_event)] HANDLE inputEvent, [in, ref] PCCONSOLE_PORTABLE_ATTACH_MSG msg, + [in, system_handle(sh_pipe)] HANDLE signalPipe, [out, system_handle(sh_process)] HANDLE* process); }; diff --git a/src/host/sources.inc b/src/host/sources.inc index a50292e4821..4145290e694 100644 --- a/src/host/sources.inc +++ b/src/host/sources.inc @@ -59,6 +59,7 @@ SOURCES = \ ..\VtIo.cpp \ ..\VtInputThread.cpp \ ..\PtySignalInputThread.cpp \ + ..\HostSignalInputThread.cpp \ ..\consoleInformation.cpp \ ..\directio.cpp \ ..\getset.cpp \ diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index c5283dca447..3b857450e1e 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -20,6 +20,7 @@ #include "../interactivity/inc/ServiceLocator.hpp" #include "../interactivity/base/ApiDetector.hpp" +#include "../interactivity/base/RemoteConsoleControl.hpp" #include "renderData.hpp" #include "../renderer/base/renderer.hpp" @@ -364,6 +365,7 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server, // from the driver... or an S_OK success. [[nodiscard]] HRESULT ConsoleEstablishHandoff([[maybe_unused]] _In_ HANDLE Server, [[maybe_unused]] HANDLE driverInputEvent, + [[maybe_unused]] HANDLE hostSignalPipe, [[maybe_unused]] PCONSOLE_API_MSG connectMessage) try { @@ -388,6 +390,9 @@ try return E_NOT_SET; } + std::unique_ptr remoteControl = std::make_unique(hostSignalPipe); + RETURN_IF_NTSTATUS_FAILED(ServiceLocator::SetConsoleControlInstance(remoteControl)); + wil::unique_handle signalPipeTheirSide; wil::unique_handle signalPipeOurSide; diff --git a/src/host/srvinit.h b/src/host/srvinit.h index 3b2725a390e..ea660ecb2b4 100644 --- a/src/host/srvinit.h +++ b/src/host/srvinit.h @@ -30,6 +30,7 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand, [[nodiscard]] HRESULT ConsoleEstablishHandoff(_In_ HANDLE Server, HANDLE driverInputEvent, + HANDLE hostSignalPipe, PCONSOLE_API_MSG connectMessage); void ConsoleCheckDebug(); diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp new file mode 100644 index 00000000000..4c32c47aa89 --- /dev/null +++ b/src/inc/HostSignals.hpp @@ -0,0 +1,27 @@ +// These values match the enumeration values of `ControlType` for the `ConsoleControl` class +// but are defined here similarly to not pollute other projects.zs +// They don't *have* to be the same values, but matching them seemed to make sense. +#define HOST_SIGNAL_NOTIFY_APP 1u +struct HOST_SIGNAL_NOTIFY_APP_DATA +{ + DWORD cbSize; + DWORD dwProcessId; +}; + +#define HOST_SIGNAL_SET_FOREGROUND 5u +struct HOST_SIGNAL_SET_FOREGROUND_DATA +{ + DWORD cbSize; + ULONG dwProcessId; + BOOL fForeground; +}; + +#define HOST_SIGNAL_END_TASK 7u +struct HOST_SIGNAL_END_TASK_DATA +{ + + DWORD cbSize; + ULONG dwProcessId; + DWORD dwEventType; + ULONG ulCtrlFlags; +}; diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp new file mode 100644 index 00000000000..e1c53133abe --- /dev/null +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -0,0 +1,89 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +#include "precomp.h" + +#include "RemoteConsoleControl.hpp" + +#include "../../inc/HostSignals.hpp" + +using namespace Microsoft::Console::Interactivity; + +RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : + _pipe{ signalPipe } +{ + +} + +#pragma region IConsoleControl Members + +[[nodiscard]] NTSTATUS RemoteConsoleControl::NotifyConsoleApplication(_In_ DWORD dwProcessId) +{ + DWORD dwWritten = 0; + + unsigned short code = HOST_SIGNAL_NOTIFY_APP; + if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + HOST_SIGNAL_NOTIFY_APP_DATA data; + data.cbSize = sizeof(data); + data.dwProcessId = dwProcessId; + + if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + return STATUS_SUCCESS; +} + +[[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) +{ + DWORD dwWritten = 0; + + unsigned short code = HOST_SIGNAL_SET_FOREGROUND; + if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + HOST_SIGNAL_SET_FOREGROUND_DATA data; + data.cbSize = sizeof(data); + data.dwProcessId = HandleToULong(hProcess); + data.fForeground = fForeground; + + if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + return STATUS_SUCCESS; +} + +[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +{ + DWORD dwWritten = 0; + + unsigned short code = HOST_SIGNAL_END_TASK; + if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + HOST_SIGNAL_END_TASK_DATA data; + data.cbSize = sizeof(data); + data.dwProcessId = HandleToULong(hProcessId); + data.dwEventType = dwEventType; + data.ulCtrlFlags = ulCtrlFlags; + + if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) + { + return NTSTATUS_FROM_WIN32(::GetLastError()); + } + + return STATUS_SUCCESS; +} + +#pragma endregion diff --git a/src/interactivity/base/RemoteConsoleControl.hpp b/src/interactivity/base/RemoteConsoleControl.hpp new file mode 100644 index 00000000000..52de8db1eb3 --- /dev/null +++ b/src/interactivity/base/RemoteConsoleControl.hpp @@ -0,0 +1,33 @@ +/*++ +Copyright (c) Microsoft Corporation +Licensed under the MIT license. + +Module Name: +- RemoteConsoleControl.hpp + +Abstract: +- This module is used for remoting console control calls to a different host owner process. + +Author(s): +- Michael Niksa (MiNiksa) 10-Jun-2021 +--*/ +#pragma once + +#include "../inc/IConsoleControl.hpp" + +namespace Microsoft::Console::Interactivity +{ + class RemoteConsoleControl final : public IConsoleControl + { + public: + RemoteConsoleControl(HANDLE signalPipe); + + // IConsoleControl Members + [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId); + [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground); + [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); + + private: + wil::unique_handle _pipe; + }; +} diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 14d088368af..0d247f60342 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -104,6 +104,27 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) #pragma region Set Methods +[[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr& control) +{ + NTSTATUS status = STATUS_SUCCESS; + + if (s_consoleControl) + { + status = STATUS_INVALID_HANDLE; + } + else if (!control) + { + status = STATUS_INVALID_PARAMETER; + } + else + { + s_consoleControl.swap(control); + control.release(); + } + + return status; +} + [[nodiscard]] NTSTATUS ServiceLocator::SetConsoleWindowInstance(_In_ IConsoleWindow* window) { NTSTATUS status = STATUS_SUCCESS; diff --git a/src/interactivity/base/lib/InteractivityBase.vcxproj b/src/interactivity/base/lib/InteractivityBase.vcxproj index bf64e6cf45d..ed4fa2698b5 100644 --- a/src/interactivity/base/lib/InteractivityBase.vcxproj +++ b/src/interactivity/base/lib/InteractivityBase.vcxproj @@ -6,7 +6,7 @@ Base InteractivityBase ConInteractivityBaseLib - StaticLibrary + StaticLibrary @@ -21,11 +21,12 @@ - + Create + @@ -40,12 +41,13 @@ - + + - + \ No newline at end of file diff --git a/src/interactivity/base/lib/InteractivityBase.vcxproj.filters b/src/interactivity/base/lib/InteractivityBase.vcxproj.filters index 58825ba6f04..6686fedc573 100644 --- a/src/interactivity/base/lib/InteractivityBase.vcxproj.filters +++ b/src/interactivity/base/lib/InteractivityBase.vcxproj.filters @@ -27,9 +27,12 @@ Source Files - - Source Files - + + Source Files + + + Source Files + @@ -41,9 +44,6 @@ Header Files - - Source Files - Header Files @@ -77,9 +77,15 @@ Header Files - - Header Files - + + Header Files + + + Header Files + + + Header Files + diff --git a/src/interactivity/base/sources.inc b/src/interactivity/base/sources.inc index 4881f791184..a3f82480ed1 100644 --- a/src/interactivity/base/sources.inc +++ b/src/interactivity/base/sources.inc @@ -43,7 +43,8 @@ SOURCES = \ ..\InteractivityFactory.cpp \ ..\ServiceLocator.cpp \ ..\VtApiRedirection.cpp \ - ..\EventSynthesis.cpp \ + ..\EventSynthesis.cpp \ + ..\RemoteConsoleControl.cpp \ INCLUDES = \ $(INCLUDES); \ diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index 888855b4a3a..3682483ce74 100644 --- a/src/interactivity/inc/ServiceLocator.hpp +++ b/src/interactivity/inc/ServiceLocator.hpp @@ -39,6 +39,7 @@ namespace Microsoft::Console::Interactivity static IAccessibilityNotifier* LocateAccessibilityNotifier(); + [[nodiscard]] static NTSTATUS SetConsoleControlInstance(_In_ std::unique_ptr& control); static IConsoleControl* LocateConsoleControl(); template static T* LocateConsoleControl() diff --git a/src/interactivity/win32/ConsoleControl.hpp b/src/interactivity/win32/ConsoleControl.hpp index aa8b10916b9..9a8ab927fc0 100644 --- a/src/interactivity/win32/ConsoleControl.hpp +++ b/src/interactivity/win32/ConsoleControl.hpp @@ -3,7 +3,7 @@ Copyright (c) Microsoft Corporation Licensed under the MIT license. Module Name: -- userdpiapi.hpp +- ConsoleControl.hpp Abstract: - This module is used for abstracting calls to private user32 DLL APIs to break the build system dependency. diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 760ea1d5ffb..42118d64fcc 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -13,6 +13,7 @@ #include "../host/handle.h" #include "../host/srvinit.h" #include "../host/telemetry.hpp" +#include "../host/HostSignalInputThread.hpp" #include "../interactivity/inc/ServiceLocator.hpp" @@ -296,6 +297,11 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API HANDLE serverHandle; THROW_IF_FAILED(Globals.pDeviceComm->GetServerHandle(&serverHandle)); + wil::unique_hfile signalPipeTheirSide; + wil::unique_hfile signalPipeOurSide; + + THROW_IF_WIN32_BOOL_FALSE(CreatePipe(signalPipeOurSide.addressof(), signalPipeTheirSide.addressof(), nullptr, 0)); + wil::unique_process_handle clientProcess; // Okay, moment of truth! If they say they successfully took it over, we're going to clean up. @@ -303,8 +309,16 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API THROW_IF_FAILED(handoff->EstablishHandoff(serverHandle, Globals.hInputEvent.get(), &msg, + signalPipeTheirSide.get(), &clientProcess)); + signalPipeTheirSide.release(); + + auto hostSignalThread = std::make_unique(std::move(signalPipeOurSide)); + + // Start it if it was successfully created. + THROW_IF_FAILED(hostSignalThread->Start()); + // Unlock in case anything tries to spool down as we exit. UnlockConsole(); From 08ad342cb8df666745e71a4b9530e807cff92c4a Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 13:18:19 -0700 Subject: [PATCH 05/18] use correct macros, add a few comments --- src/host/HostSignalInputThread.cpp | 8 ++++---- src/server/IoDispatchers.cpp | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/host/HostSignalInputThread.cpp b/src/host/HostSignalInputThread.cpp index 17cdec2871a..7acecd5accc 100644 --- a/src/host/HostSignalInputThread.cpp +++ b/src/host/HostSignalInputThread.cpp @@ -88,8 +88,8 @@ void HostSignalInputThread::ConnectConsole() noexcept { THROW_HR(E_ILLEGAL_METHOD_CALL); } - - LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.dwProcessId)); + + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.dwProcessId)); } else { @@ -112,7 +112,7 @@ void HostSignalInputThread::ConnectConsole() noexcept THROW_HR(E_ILLEGAL_METHOD_CALL); } - LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.dwProcessId), msg.fForeground)); + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.dwProcessId), msg.fForeground)); } else { @@ -134,7 +134,7 @@ void HostSignalInputThread::ConnectConsole() noexcept THROW_HR(E_ILLEGAL_METHOD_CALL); } - LOG_NTSTATUS(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.dwProcessId), msg.dwEventType, msg.ulCtrlFlags)); + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.dwProcessId), msg.dwEventType, msg.ulCtrlFlags)); } else { diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 42118d64fcc..144a87bcfbe 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -312,8 +312,10 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API signalPipeTheirSide.get(), &clientProcess)); + // Detach the end of the pipe we gave them. signalPipeTheirSide.release(); + // Start a thread to listen for signals from their side that we must relay to the OS. auto hostSignalThread = std::make_unique(std::move(signalPipeOurSide)); // Start it if it was successfully created. From f99ff517cd7bf1f6870353606e61d205faf6fba2 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 14:56:17 -0700 Subject: [PATCH 06/18] Properly reset some of the handles we have handed off. Establish appropriate watching between the processes in the chain so they clean up correctly when any one of them exits (appropriately or otherwise.) --- src/host/exe/CConsoleHandoff.cpp | 5 ++++- src/host/exe/CConsoleHandoff.h | 1 + src/host/globals.h | 2 ++ src/host/proxy/IConsoleHandoff.idl | 1 + src/host/srvinit.cpp | 29 +++++++++++++++++------------ src/host/srvinit.h | 1 + src/server/IoDispatchers.cpp | 17 +++++++++++++++-- 7 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/host/exe/CConsoleHandoff.cpp b/src/host/exe/CConsoleHandoff.cpp index 09262edac52..d04cde821a8 100644 --- a/src/host/exe/CConsoleHandoff.cpp +++ b/src/host/exe/CConsoleHandoff.cpp @@ -27,11 +27,13 @@ static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) // - server - Console driver server handle // - inputEvent - Event already established that we signal when new input data is available in case the driver is waiting on us // - msg - Portable attach message containing just enough descriptor payload to get us started in servicing it +// - inboxProcess - Handle to the inbox process so we can watch it to see if it disappears on us. // - process - Handle to our process for waiting for us to exit HRESULT CConsoleHandoff::EstablishHandoff(HANDLE server, HANDLE inputEvent, PCCONSOLE_PORTABLE_ATTACH_MSG msg, HANDLE signalPipe, + HANDLE inboxProcess, HANDLE* process) try { @@ -59,9 +61,10 @@ try RETURN_IF_FAILED(_duplicateHandle(server, server)); RETURN_IF_FAILED(_duplicateHandle(inputEvent, inputEvent)); RETURN_IF_FAILED(_duplicateHandle(signalPipe, signalPipe)); + RETURN_IF_FAILED(_duplicateHandle(inboxProcess, inboxProcess)); // Now perform the handoff. - RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, signalPipe, &apiMsg)); + RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, signalPipe, inboxProcess, &apiMsg)); // Give back a copy of our own process handle to be tracked. wil::unique_handle duplicatedHandle; diff --git a/src/host/exe/CConsoleHandoff.h b/src/host/exe/CConsoleHandoff.h index 05e056e913b..36ad564f620 100644 --- a/src/host/exe/CConsoleHandoff.h +++ b/src/host/exe/CConsoleHandoff.h @@ -36,6 +36,7 @@ struct __declspec(uuid(__CLSID_CConsoleHandoff)) HANDLE inputEvent, PCCONSOLE_PORTABLE_ATTACH_MSG msg, HANDLE signalPipe, + HANDLE inboxProcess, HANDLE* process); #pragma endregion diff --git a/src/host/globals.h b/src/host/globals.h index c99794845fa..a955e991f87 100644 --- a/src/host/globals.h +++ b/src/host/globals.h @@ -74,6 +74,8 @@ class Globals std::optional handoffConsoleClsid; std::optional handoffTerminalClsid; + wil::unique_hfile handoffInboxConsoleHandle; + wil::unique_threadpool_wait handoffInboxConsoleExitWait; #ifdef UNIT_TESTING void EnableConptyModeForTests(std::unique_ptr vtRenderEngine); diff --git a/src/host/proxy/IConsoleHandoff.idl b/src/host/proxy/IConsoleHandoff.idl index 391bc726fde..d7e8d63bc53 100644 --- a/src/host/proxy/IConsoleHandoff.idl +++ b/src/host/proxy/IConsoleHandoff.idl @@ -27,6 +27,7 @@ typedef const CONSOLE_PORTABLE_ATTACH_MSG* PCCONSOLE_PORTABLE_ATTACH_MSG; [in, system_handle(sh_event)] HANDLE inputEvent, [in, ref] PCCONSOLE_PORTABLE_ATTACH_MSG msg, [in, system_handle(sh_pipe)] HANDLE signalPipe, + [in, system_handle(sh_process)] HANDLE inboxProcess, [out, system_handle(sh_process)] HANDLE* process); }; diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 3b857450e1e..219a267e4c3 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -366,6 +366,7 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server, [[nodiscard]] HRESULT ConsoleEstablishHandoff([[maybe_unused]] _In_ HANDLE Server, [[maybe_unused]] HANDLE driverInputEvent, [[maybe_unused]] HANDLE hostSignalPipe, + [[maybe_unused]] HANDLE hostProcessHandle, [[maybe_unused]] PCONSOLE_API_MSG connectMessage) try { @@ -390,6 +391,19 @@ try return E_NOT_SET; } + // Capture handle to the inbox process into a unique handle holder. + g.handoffInboxConsoleHandle.reset(hostProcessHandle); + + // Set up a threadpool waiter to shutdown everything if the inbox process disappears. + g.handoffInboxConsoleExitWait.reset(CreateThreadpoolWait( + [](PTP_CALLBACK_INSTANCE /*callbackInstance*/, PVOID /*context*/, PTP_WAIT /*wait*/, TP_WAIT_RESULT /*waitResult*/) noexcept { + ServiceLocator::RundownAndExit(E_APPLICATION_MANAGER_NOT_RUNNING); + }, + nullptr, + nullptr)); + + SetThreadpoolWait(g.handoffInboxConsoleExitWait.get(), g.handoffInboxConsoleHandle.get(), nullptr); + std::unique_ptr remoteControl = std::make_unique(hostSignalPipe); RETURN_IF_NTSTATUS_FAILED(ServiceLocator::SetConsoleControlInstance(remoteControl)); @@ -402,20 +416,11 @@ try wil::unique_handle outPipeTheirSide; wil::unique_handle outPipeOurSide; - SECURITY_ATTRIBUTES sa; - sa.nLength = sizeof(sa); - // Mark inheritable for signal handle when creating. It'll have the same value on the other side. - sa.bInheritHandle = TRUE; - sa.lpSecurityDescriptor = nullptr; - RETURN_IF_WIN32_BOOL_FALSE(CreatePipe(signalPipeOurSide.addressof(), signalPipeTheirSide.addressof(), nullptr, 0)); - RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(signalPipeTheirSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)); RETURN_IF_WIN32_BOOL_FALSE(CreatePipe(inPipeOurSide.addressof(), inPipeTheirSide.addressof(), nullptr, 0)); - RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(inPipeTheirSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)); RETURN_IF_WIN32_BOOL_FALSE(CreatePipe(outPipeTheirSide.addressof(), outPipeOurSide.addressof(), nullptr, 0)); - RETURN_IF_WIN32_BOOL_FALSE(SetHandleInformation(outPipeTheirSide.get(), HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT)); wil::unique_handle clientProcess{ OpenProcess(PROCESS_QUERY_INFORMATION | SYNCHRONIZE, TRUE, static_cast(connectMessage->Descriptor.Process)) }; RETURN_LAST_ERROR_IF_NULL(clientProcess.get()); @@ -439,9 +444,9 @@ try serverProcess, clientProcess.get())); - inPipeTheirSide.release(); - outPipeTheirSide.release(); - signalPipeTheirSide.release(); + inPipeTheirSide.reset(); + outPipeTheirSide.reset(); + signalPipeTheirSide.reset(); const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --signal {:#x}"), (int64_t)signalPipeOurSide.release()); diff --git a/src/host/srvinit.h b/src/host/srvinit.h index ea660ecb2b4..6fac6771897 100644 --- a/src/host/srvinit.h +++ b/src/host/srvinit.h @@ -31,6 +31,7 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand, [[nodiscard]] HRESULT ConsoleEstablishHandoff(_In_ HANDLE Server, HANDLE driverInputEvent, HANDLE hostSignalPipe, + HANDLE hostProcessHandle, PCONSOLE_API_MSG connectMessage); void ConsoleCheckDebug(); diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 144a87bcfbe..e72eeea6513 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -302,6 +302,16 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API THROW_IF_WIN32_BOOL_FALSE(CreatePipe(signalPipeOurSide.addressof(), signalPipeTheirSide.addressof(), nullptr, 0)); + // Give a copy of our own process handle to be tracked. + wil::unique_process_handle ourProcess; + THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), + GetCurrentProcess(), + GetCurrentProcess(), + &ourProcess, + SYNCHRONIZE, + FALSE, + 0)); + wil::unique_process_handle clientProcess; // Okay, moment of truth! If they say they successfully took it over, we're going to clean up. @@ -310,10 +320,13 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API Globals.hInputEvent.get(), &msg, signalPipeTheirSide.get(), + ourProcess.get(), &clientProcess)); - // Detach the end of the pipe we gave them. - signalPipeTheirSide.release(); + // Close handles for the things we gave to them + signalPipeTheirSide.reset(); + ourProcess.reset(); + Globals.hInputEvent.reset(); // Start a thread to listen for signals from their side that we must relay to the OS. auto hostSignalThread = std::make_unique(std::move(signalPipeOurSide)); From 14fd5f9134abdd612197a8ee7ede56b740b8952c Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 15:09:14 -0700 Subject: [PATCH 07/18] move host signal input thread to interactivity base since it's dispatching to interactivity anyway. that helps it link better with existing tests. --- src/host/lib/hostlib.vcxproj | 6 ------ src/host/lib/hostlib.vcxproj.filters | 6 ------ src/host/sources.inc | 1 - .../base}/HostSignalInputThread.cpp | 15 --------------- .../base}/HostSignalInputThread.hpp | 0 .../base/lib/InteractivityBase.vcxproj | 2 ++ .../base/lib/InteractivityBase.vcxproj.filters | 6 ++++++ src/interactivity/base/sources.inc | 3 ++- 8 files changed, 10 insertions(+), 29 deletions(-) rename src/{host => interactivity/base}/HostSignalInputThread.cpp (89%) rename src/{host => interactivity/base}/HostSignalInputThread.hpp (100%) diff --git a/src/host/lib/hostlib.vcxproj b/src/host/lib/hostlib.vcxproj index d900c0453b9..44598d01ebe 100644 --- a/src/host/lib/hostlib.vcxproj +++ b/src/host/lib/hostlib.vcxproj @@ -8,12 +8,6 @@ ConhostV2Lib StaticLibrary - - - - - - diff --git a/src/host/lib/hostlib.vcxproj.filters b/src/host/lib/hostlib.vcxproj.filters index 59c4da62cd9..e8877f96c70 100644 --- a/src/host/lib/hostlib.vcxproj.filters +++ b/src/host/lib/hostlib.vcxproj.filters @@ -183,9 +183,6 @@ Source Files - - Source Files - @@ -359,9 +356,6 @@ Header Files - - Header Files - diff --git a/src/host/sources.inc b/src/host/sources.inc index 4145290e694..a50292e4821 100644 --- a/src/host/sources.inc +++ b/src/host/sources.inc @@ -59,7 +59,6 @@ SOURCES = \ ..\VtIo.cpp \ ..\VtInputThread.cpp \ ..\PtySignalInputThread.cpp \ - ..\HostSignalInputThread.cpp \ ..\consoleInformation.cpp \ ..\directio.cpp \ ..\getset.cpp \ diff --git a/src/host/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp similarity index 89% rename from src/host/HostSignalInputThread.cpp rename to src/interactivity/base/HostSignalInputThread.cpp index 7acecd5accc..1278e70d541 100644 --- a/src/host/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -6,14 +6,10 @@ #include "HostSignalInputThread.hpp" #include "../inc/HostSignals.hpp" -#include "output.h" -#include "handle.h" #include "../interactivity/inc/ServiceLocator.hpp" -#include "../terminal/adapter/DispatchCommon.hpp" using namespace Microsoft::Console; using namespace Microsoft::Console::Interactivity; -using namespace Microsoft::Console::VirtualTerminal; // Constructor Description: // - Creates the PTY Signal Input Thread. @@ -254,17 +250,6 @@ bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pB // - void HostSignalInputThread::_Shutdown() { - // Trigger process shutdown. - CloseConsoleProcessState(); - - // If we haven't terminated by now, that's because there's a client that's still attached. - // Force the handling of the control events by the attached clients. - // As of MSFT:19419231, CloseConsoleProcessState will make sure this - // happens if this method is called outside of lock, but if we're - // currently locked, we want to make sure ctrl events are handled - // _before_ we RundownAndExit. - ProcessCtrlEvents(); - // Make sure we terminate. ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); } diff --git a/src/host/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp similarity index 100% rename from src/host/HostSignalInputThread.hpp rename to src/interactivity/base/HostSignalInputThread.hpp diff --git a/src/interactivity/base/lib/InteractivityBase.vcxproj b/src/interactivity/base/lib/InteractivityBase.vcxproj index ed4fa2698b5..0986a133f5a 100644 --- a/src/interactivity/base/lib/InteractivityBase.vcxproj +++ b/src/interactivity/base/lib/InteractivityBase.vcxproj @@ -22,6 +22,7 @@ + Create @@ -43,6 +44,7 @@ + diff --git a/src/interactivity/base/lib/InteractivityBase.vcxproj.filters b/src/interactivity/base/lib/InteractivityBase.vcxproj.filters index 6686fedc573..b7f9e8e1890 100644 --- a/src/interactivity/base/lib/InteractivityBase.vcxproj.filters +++ b/src/interactivity/base/lib/InteractivityBase.vcxproj.filters @@ -33,6 +33,9 @@ Source Files + + Source Files + @@ -86,6 +89,9 @@ Header Files + + Header Files + diff --git a/src/interactivity/base/sources.inc b/src/interactivity/base/sources.inc index a3f82480ed1..9521ea7c0ed 100644 --- a/src/interactivity/base/sources.inc +++ b/src/interactivity/base/sources.inc @@ -44,7 +44,8 @@ SOURCES = \ ..\ServiceLocator.cpp \ ..\VtApiRedirection.cpp \ ..\EventSynthesis.cpp \ - ..\RemoteConsoleControl.cpp \ + ..\RemoteConsoleControl.cpp \ + ..\HostSignalInputThread.cpp \ INCLUDES = \ $(INCLUDES); \ From a8bfc9c96a376791429d65d666c62a0a3f475e52 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 15:45:45 -0700 Subject: [PATCH 08/18] spellcheck --- .github/actions/spelling/expect/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 722631b26ea..a7f124f39d4 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -1941,6 +1941,7 @@ REGSTR reingest Relayout RELBINPATH +remoting Remoting renamer renderengine From a6ef19a615880fc9d4ac48d5ecb0aced0fd4764c Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 15:46:24 -0700 Subject: [PATCH 09/18] codeformat! --- src/inc/HostSignals.hpp | 1 - src/interactivity/base/HostSignalInputThread.cpp | 4 ++-- src/interactivity/base/RemoteConsoleControl.cpp | 1 - src/server/IoDispatchers.cpp | 12 ++++++------ 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index 4c32c47aa89..d9032ed21e9 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -19,7 +19,6 @@ struct HOST_SIGNAL_SET_FOREGROUND_DATA #define HOST_SIGNAL_END_TASK 7u struct HOST_SIGNAL_END_TASK_DATA { - DWORD cbSize; ULONG dwProcessId; DWORD dwEventType; diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 1278e70d541..0b0e7650f74 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -84,7 +84,7 @@ void HostSignalInputThread::ConnectConsole() noexcept { THROW_HR(E_ILLEGAL_METHOD_CALL); } - + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.dwProcessId)); } else @@ -183,7 +183,7 @@ bool HostSignalInputThread::_AdvanceReader(const DWORD cbBytes) // Return Value: // - True if data was retrieved successfully. False otherwise. bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, - const DWORD cbBuffer) + const DWORD cbBuffer) { DWORD dwRead = 0; // If we failed to read because the terminal broke our pipe (usually due diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index e1c53133abe..a65902551ee 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -12,7 +12,6 @@ using namespace Microsoft::Console::Interactivity; RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : _pipe{ signalPipe } { - } #pragma region IConsoleControl Members diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index e72eeea6513..8d1aa018121 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -305,12 +305,12 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API // Give a copy of our own process handle to be tracked. wil::unique_process_handle ourProcess; THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), - GetCurrentProcess(), - GetCurrentProcess(), - &ourProcess, - SYNCHRONIZE, - FALSE, - 0)); + GetCurrentProcess(), + GetCurrentProcess(), + &ourProcess, + SYNCHRONIZE, + FALSE, + 0)); wil::unique_process_handle clientProcess; From 579be98a671a30415bf81df336af807f2d362da7 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 15:47:11 -0700 Subject: [PATCH 10/18] restore otherwise unchanged file --- src/host/lib/hostlib.vcxproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/host/lib/hostlib.vcxproj b/src/host/lib/hostlib.vcxproj index 44598d01ebe..fb83977af9c 100644 --- a/src/host/lib/hostlib.vcxproj +++ b/src/host/lib/hostlib.vcxproj @@ -13,4 +13,4 @@ - \ No newline at end of file + From d811b33d6f0c5d9331a020eaefd69f83673ba34b Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 11 Jun 2021 16:01:26 -0700 Subject: [PATCH 11/18] stale header reference --- src/server/IoDispatchers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 8d1aa018121..53e6d753869 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -13,8 +13,8 @@ #include "../host/handle.h" #include "../host/srvinit.h" #include "../host/telemetry.hpp" -#include "../host/HostSignalInputThread.hpp" +#include "../interactivity/base/HostSignalInputThread.hpp" #include "../interactivity/inc/ServiceLocator.hpp" #include "../types/inc/utils.hpp" From 46406901c8e01dcacca82bfbd3cc37ec4b2758b1 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 14 Jun 2021 11:34:03 -0700 Subject: [PATCH 12/18] Enums and classes and templates and namespaces and move semantics and grammar issues in comments... oh my. --- src/host/PtySignalInputThread.cpp | 2 +- src/host/srvinit.cpp | 2 +- src/inc/HostSignals.hpp | 39 +++--- .../base/HostSignalInputThread.cpp | 124 ++++++++---------- .../base/HostSignalInputThread.hpp | 3 +- .../base/RemoteConsoleControl.cpp | 76 ++++------- src/interactivity/base/ServiceLocator.cpp | 5 +- src/interactivity/inc/ServiceLocator.hpp | 2 +- 8 files changed, 113 insertions(+), 140 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index c563adc6b6f..62468a4b0d9 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -76,7 +76,7 @@ void PtySignalInputThread::ConnectConsole() noexcept // - The ThreadProc for the PTY Signal Input Thread. // Return Value: // - S_OK if the thread runs to completion. -// - Otherwise it may cause an application termination another route and never return. +// - Otherwise it may cause an application termination and never return. [[nodiscard]] HRESULT PtySignalInputThread::_InputThread() { unsigned short signalId; diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 219a267e4c3..ad7716b499d 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -405,7 +405,7 @@ try SetThreadpoolWait(g.handoffInboxConsoleExitWait.get(), g.handoffInboxConsoleHandle.get(), nullptr); std::unique_ptr remoteControl = std::make_unique(hostSignalPipe); - RETURN_IF_NTSTATUS_FAILED(ServiceLocator::SetConsoleControlInstance(remoteControl)); + RETURN_IF_NTSTATUS_FAILED(ServiceLocator::SetConsoleControlInstance(std::move(remoteControl))); wil::unique_handle signalPipeTheirSide; wil::unique_handle signalPipeOurSide; diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index d9032ed21e9..620bc8c263a 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -1,26 +1,33 @@ +namespace Microsoft::Console +{ // These values match the enumeration values of `ControlType` for the `ConsoleControl` class -// but are defined here similarly to not pollute other projects.zs +// but are defined here similarly to not pollute other projects. // They don't *have* to be the same values, but matching them seemed to make sense. -#define HOST_SIGNAL_NOTIFY_APP 1u -struct HOST_SIGNAL_NOTIFY_APP_DATA +enum class HostSignals : uint8_t +{ + NotifyApp = 1u, + SetForeground = 5u, + EndTask = 7u +}; + +struct HostSignalNotifyAppData { - DWORD cbSize; - DWORD dwProcessId; + uint32_t sizeInBytes; + uint32_t processId; }; -#define HOST_SIGNAL_SET_FOREGROUND 5u -struct HOST_SIGNAL_SET_FOREGROUND_DATA +struct HostSignalSetForegroundData { - DWORD cbSize; - ULONG dwProcessId; - BOOL fForeground; + uint32_t sizeInBytes; + uint32_t processId; + bool isForeground; }; -#define HOST_SIGNAL_END_TASK 7u -struct HOST_SIGNAL_END_TASK_DATA +struct HostSignalEndTaskData { - DWORD cbSize; - ULONG dwProcessId; - DWORD dwEventType; - ULONG ulCtrlFlags; + uint32_t sizeInBytes; + uint32_t processId; + uint32_t eventType; + uint32_t ctrlFlags; }; +}; \ No newline at end of file diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 0b0e7650f74..8ce367edb18 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -15,7 +15,7 @@ using namespace Microsoft::Console::Interactivity; // - Creates the PTY Signal Input Thread. // Arguments: // - hPipe - a handle to the file representing the read end of the Host Signal pipe. -HostSignalInputThread::HostSignalInputThread(_In_ wil::unique_hfile hPipe) : +HostSignalInputThread::HostSignalInputThread(_In_ wil::unique_hfile&& hPipe) : _hFile{ std::move(hPipe) }, _hThread{}, _dwThreadId{ 0 }, @@ -59,83 +59,70 @@ void HostSignalInputThread::ConnectConsole() noexcept _consoleConnected = true; } +// Method Description: +// - Attempts to retrieve a given type T off of the internal +// pipe channel and return it. +// Return Value: +// - A structure filled with the specified data off the byte stream +// - EXCEPTIONS may be thrown if the packet size mismatches +// or if we fail to read for another reason. +template T HostSignalInputThread::_ReaderHelper() +{ + T msg = {0}; + THROW_HR_IF(E_ABORT, !_GetData(&msg, sizeof(msg))); + + // If the message size was stated to be larger, we + // want to seek forward to the next message code. + // If it's equal, we'll seek forward by 0 and + // do nothing. + if (msg.sizeInBytes >= sizeof(msg)) + { + _AdvanceReader(msg.sizeInBytes - sizeof(msg)); + } + // If the message is smaller than what we expected + // then it was malformed and we need to throw. + else + { + THROW_HR(E_ILLEGAL_METHOD_CALL); + } + + return msg; +} + // Method Description: // - The ThreadProc for the Host Signal Input Thread. // Return Value: // - S_OK if the thread runs to completion. -// - Otherwise it may cause an application termination another route and never return. +// - Otherwise it may cause an application termination and never return. [[nodiscard]] HRESULT HostSignalInputThread::_InputThread() { - unsigned short signalId; + HostSignals signalId; while (_GetData(&signalId, sizeof(signalId))) { switch (signalId) { - case HOST_SIGNAL_NOTIFY_APP: + case HostSignals::NotifyApp: { - HOST_SIGNAL_NOTIFY_APP_DATA msg = { 0 }; - if (_GetData(&msg, sizeof(msg))) - { - if (msg.cbSize >= sizeof(msg)) - { - _AdvanceReader(msg.cbSize - sizeof(msg)); - } - else - { - THROW_HR(E_ILLEGAL_METHOD_CALL); - } - - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.dwProcessId)); - } - else - { - return E_ABORT; - } + auto msg = _ReaderHelper(); + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.processId)); + break; } - case HOST_SIGNAL_SET_FOREGROUND: + case HostSignals::SetForeground: { - HOST_SIGNAL_SET_FOREGROUND_DATA msg = { 0 }; - if (_GetData(&msg, sizeof(msg))) - { - if (msg.cbSize >= sizeof(msg)) - { - _AdvanceReader(msg.cbSize - sizeof(msg)); - } - else - { - THROW_HR(E_ILLEGAL_METHOD_CALL); - } - - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.dwProcessId), msg.fForeground)); - } - else - { - return E_ABORT; - } + auto msg = _ReaderHelper(); + + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); + break; } - case HOST_SIGNAL_END_TASK: + case HostSignals::EndTask: { - HOST_SIGNAL_END_TASK_DATA msg = { 0 }; - if (_GetData(&msg, sizeof(msg))) - { - if (msg.cbSize >= sizeof(msg)) - { - _AdvanceReader(msg.cbSize - sizeof(msg)); - } - else - { - THROW_HR(E_ILLEGAL_METHOD_CALL); - } - - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.dwProcessId), msg.dwEventType, msg.ulCtrlFlags)); - } - else - { - return E_ABORT; - } + HostSignalEndTaskData msg = { 0 }; + + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.processId), msg.eventType, msg.ctrlFlags)); + break; } default: @@ -149,7 +136,7 @@ void HostSignalInputThread::ConnectConsole() noexcept } // Method Description: -// - Skips the file stream forward by the specified number of bytes.zs +// - Skips the file stream forward by the specified number of bytes. // Arguments: // - cbBytes - Count of bytes to skip forward // Return Value: @@ -218,21 +205,18 @@ bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pB { RETURN_LAST_ERROR_IF(!_hFile); - HANDLE hThread = nullptr; - // 0 is the right value, https://blogs.msdn.microsoft.com/oldnewthing/20040223-00/?p=40503 - DWORD dwThreadId = 0; + // 0 is the right value, https://devblogs.microsoft.com/oldnewthing/20040223-00/?p=40503 + _dwThreadId = 0; - hThread = CreateThread(nullptr, + _hThread.reset(CreateThread(nullptr, 0, HostSignalInputThread::StaticThreadProc, this, 0, - &dwThreadId); + &_dwThreadId)); - RETURN_LAST_ERROR_IF_NULL(hThread); - _hThread.reset(hThread); - _dwThreadId = dwThreadId; - LOG_IF_FAILED(SetThreadDescription(hThread, L"Host Signal Handler Thread")); + RETURN_LAST_ERROR_IF_NULL(_hThread.get()); + LOG_IF_FAILED(SetThreadDescription(_hThread.get(), L"Host Signal Handler Thread")); return S_OK; } diff --git a/src/interactivity/base/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp index 334eddcdee7..9d9f38c0a7f 100644 --- a/src/interactivity/base/HostSignalInputThread.hpp +++ b/src/interactivity/base/HostSignalInputThread.hpp @@ -22,7 +22,7 @@ namespace Microsoft::Console class HostSignalInputThread final { public: - HostSignalInputThread(_In_ wil::unique_hfile hPipe); + HostSignalInputThread(_In_ wil::unique_hfile&& hPipe); ~HostSignalInputThread(); [[nodiscard]] HRESULT Start() noexcept; @@ -35,6 +35,7 @@ namespace Microsoft::Console void ConnectConsole() noexcept; private: + template T _ReaderHelper(); [[nodiscard]] HRESULT _InputThread(); bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index a65902551ee..c31a1f0d2b1 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -16,21 +16,20 @@ RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : #pragma region IConsoleControl Members -[[nodiscard]] NTSTATUS RemoteConsoleControl::NotifyConsoleApplication(_In_ DWORD dwProcessId) +template [[nodiscard]] NTSTATUS _PacketSender(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) { - DWORD dwWritten = 0; - - unsigned short code = HOST_SIGNAL_NOTIFY_APP; - if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) + struct HostSignalPacket { - return NTSTATUS_FROM_WIN32(::GetLastError()); - } + ::Microsoft::Console::HostSignals code; + T data; + }; - HOST_SIGNAL_NOTIFY_APP_DATA data; - data.cbSize = sizeof(data); - data.dwProcessId = dwProcessId; + HostSignalPacket packet; + packet.code = signalCode; + packet.data = payload; - if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) + DWORD dwWritten = 0; + if (!WriteFile(pipe, &packet, sizeof(packet), &dwWritten, nullptr)) { return NTSTATUS_FROM_WIN32(::GetLastError()); } @@ -38,51 +37,34 @@ RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : return STATUS_SUCCESS; } -[[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) +[[nodiscard]] NTSTATUS RemoteConsoleControl::NotifyConsoleApplication(_In_ DWORD dwProcessId) { - DWORD dwWritten = 0; - - unsigned short code = HOST_SIGNAL_SET_FOREGROUND; - if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) - { - return NTSTATUS_FROM_WIN32(::GetLastError()); - } + HostSignalNotifyAppData data = { 0 }; + data.sizeInBytes = sizeof(data); + data.processId = dwProcessId; - HOST_SIGNAL_SET_FOREGROUND_DATA data; - data.cbSize = sizeof(data); - data.dwProcessId = HandleToULong(hProcess); - data.fForeground = fForeground; + return _PacketSender(_pipe.get(), HostSignals::NotifyApp, data); +} - if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) - { - return NTSTATUS_FROM_WIN32(::GetLastError()); - } +[[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) +{ + HostSignalSetForegroundData data = { 0 }; + data.sizeInBytes = sizeof(data); + data.processId = HandleToULong(hProcess); + data.isForeground = fForeground; - return STATUS_SUCCESS; + return _PacketSender(_pipe.get(), HostSignals::SetForeground, data); } [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { - DWORD dwWritten = 0; + HostSignalEndTaskData data = { 0 }; + data.sizeInBytes = sizeof(data); + data.processId = HandleToULong(hProcessId); + data.eventType = dwEventType; + data.ctrlFlags = ulCtrlFlags; - unsigned short code = HOST_SIGNAL_END_TASK; - if (!WriteFile(_pipe.get(), &code, sizeof(code), &dwWritten, nullptr)) - { - return NTSTATUS_FROM_WIN32(::GetLastError()); - } - - HOST_SIGNAL_END_TASK_DATA data; - data.cbSize = sizeof(data); - data.dwProcessId = HandleToULong(hProcessId); - data.dwEventType = dwEventType; - data.ulCtrlFlags = ulCtrlFlags; - - if (!WriteFile(_pipe.get(), &data, sizeof(data), &dwWritten, nullptr)) - { - return NTSTATUS_FROM_WIN32(::GetLastError()); - } - - return STATUS_SUCCESS; + return _PacketSender(_pipe.get(), HostSignals::EndTask, data); } #pragma endregion diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 0d247f60342..1e67be8b602 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -104,7 +104,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) #pragma region Set Methods -[[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr& control) +[[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr&& control) { NTSTATUS status = STATUS_SUCCESS; @@ -118,8 +118,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) } else { - s_consoleControl.swap(control); - control.release(); + s_consoleControl = std::move(control); } return status; diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index 3682483ce74..a7b0dc89a86 100644 --- a/src/interactivity/inc/ServiceLocator.hpp +++ b/src/interactivity/inc/ServiceLocator.hpp @@ -39,7 +39,7 @@ namespace Microsoft::Console::Interactivity static IAccessibilityNotifier* LocateAccessibilityNotifier(); - [[nodiscard]] static NTSTATUS SetConsoleControlInstance(_In_ std::unique_ptr& control); + [[nodiscard]] static NTSTATUS SetConsoleControlInstance(_In_ std::unique_ptr&& control); static IConsoleControl* LocateConsoleControl(); template static T* LocateConsoleControl() From c65380f7dbfea22d3e57d7dd9eb1f58d3eb99561 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 14 Jun 2021 11:34:49 -0700 Subject: [PATCH 13/18] Code format --- src/inc/HostSignals.hpp | 54 +++++++++---------- .../base/HostSignalInputThread.cpp | 23 ++++---- .../base/HostSignalInputThread.hpp | 3 +- .../base/RemoteConsoleControl.cpp | 3 +- 4 files changed, 43 insertions(+), 40 deletions(-) diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index 620bc8c263a..05e1d0f3f72 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -1,33 +1,33 @@ namespace Microsoft::Console { -// These values match the enumeration values of `ControlType` for the `ConsoleControl` class -// but are defined here similarly to not pollute other projects. -// They don't *have* to be the same values, but matching them seemed to make sense. -enum class HostSignals : uint8_t -{ - NotifyApp = 1u, - SetForeground = 5u, - EndTask = 7u -}; + // These values match the enumeration values of `ControlType` for the `ConsoleControl` class + // but are defined here similarly to not pollute other projects. + // They don't *have* to be the same values, but matching them seemed to make sense. + enum class HostSignals : uint8_t + { + NotifyApp = 1u, + SetForeground = 5u, + EndTask = 7u + }; -struct HostSignalNotifyAppData -{ - uint32_t sizeInBytes; - uint32_t processId; -}; + struct HostSignalNotifyAppData + { + uint32_t sizeInBytes; + uint32_t processId; + }; -struct HostSignalSetForegroundData -{ - uint32_t sizeInBytes; - uint32_t processId; - bool isForeground; -}; + struct HostSignalSetForegroundData + { + uint32_t sizeInBytes; + uint32_t processId; + bool isForeground; + }; -struct HostSignalEndTaskData -{ - uint32_t sizeInBytes; - uint32_t processId; - uint32_t eventType; - uint32_t ctrlFlags; -}; + struct HostSignalEndTaskData + { + uint32_t sizeInBytes; + uint32_t processId; + uint32_t eventType; + uint32_t ctrlFlags; + }; }; \ No newline at end of file diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 8ce367edb18..c338f5c6131 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -66,14 +66,15 @@ void HostSignalInputThread::ConnectConsole() noexcept // - A structure filled with the specified data off the byte stream // - EXCEPTIONS may be thrown if the packet size mismatches // or if we fail to read for another reason. -template T HostSignalInputThread::_ReaderHelper() +template +T HostSignalInputThread::_ReaderHelper() { - T msg = {0}; + T msg = { 0 }; THROW_HR_IF(E_ABORT, !_GetData(&msg, sizeof(msg))); // If the message size was stated to be larger, we // want to seek forward to the next message code. - // If it's equal, we'll seek forward by 0 and + // If it's equal, we'll seek forward by 0 and // do nothing. if (msg.sizeInBytes >= sizeof(msg)) { @@ -106,7 +107,7 @@ template T HostSignalInputThread::_ReaderHelper() auto msg = _ReaderHelper(); LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.processId)); - + break; } case HostSignals::SetForeground: @@ -120,9 +121,9 @@ template T HostSignalInputThread::_ReaderHelper() case HostSignals::EndTask: { HostSignalEndTaskData msg = { 0 }; - + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.processId), msg.eventType, msg.ctrlFlags)); - + break; } default: @@ -209,11 +210,11 @@ bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pB _dwThreadId = 0; _hThread.reset(CreateThread(nullptr, - 0, - HostSignalInputThread::StaticThreadProc, - this, - 0, - &_dwThreadId)); + 0, + HostSignalInputThread::StaticThreadProc, + this, + 0, + &_dwThreadId)); RETURN_LAST_ERROR_IF_NULL(_hThread.get()); LOG_IF_FAILED(SetThreadDescription(_hThread.get(), L"Host Signal Handler Thread")); diff --git a/src/interactivity/base/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp index 9d9f38c0a7f..f8a2ecd9a2c 100644 --- a/src/interactivity/base/HostSignalInputThread.hpp +++ b/src/interactivity/base/HostSignalInputThread.hpp @@ -35,7 +35,8 @@ namespace Microsoft::Console void ConnectConsole() noexcept; private: - template T _ReaderHelper(); + template + T _ReaderHelper(); [[nodiscard]] HRESULT _InputThread(); bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index c31a1f0d2b1..4c1a4eb1bfb 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -16,7 +16,8 @@ RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : #pragma region IConsoleControl Members -template [[nodiscard]] NTSTATUS _PacketSender(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) +template +[[nodiscard]] NTSTATUS _PacketSender(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) { struct HostSignalPacket { From 712f9e5ef3bc1ae179b1efba01167271e9c35d55 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 14 Jun 2021 14:37:07 -0700 Subject: [PATCH 14/18] improve naming, actually save the file with the third definition. --- src/interactivity/base/HostSignalInputThread.cpp | 8 ++++---- src/interactivity/base/HostSignalInputThread.hpp | 2 +- src/interactivity/base/RemoteConsoleControl.cpp | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index c338f5c6131..c74fb889d0e 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -67,7 +67,7 @@ void HostSignalInputThread::ConnectConsole() noexcept // - EXCEPTIONS may be thrown if the packet size mismatches // or if we fail to read for another reason. template -T HostSignalInputThread::_ReaderHelper() +T HostSignalInputThread::_ReceiveTypedPacket() { T msg = { 0 }; THROW_HR_IF(E_ABORT, !_GetData(&msg, sizeof(msg))); @@ -104,7 +104,7 @@ T HostSignalInputThread::_ReaderHelper() { case HostSignals::NotifyApp: { - auto msg = _ReaderHelper(); + auto msg = _ReceiveTypedPacket(); LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->NotifyConsoleApplication(msg.processId)); @@ -112,7 +112,7 @@ T HostSignalInputThread::_ReaderHelper() } case HostSignals::SetForeground: { - auto msg = _ReaderHelper(); + auto msg = _ReceiveTypedPacket(); LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); @@ -120,7 +120,7 @@ T HostSignalInputThread::_ReaderHelper() } case HostSignals::EndTask: { - HostSignalEndTaskData msg = { 0 }; + auto msg = _ReceiveTypedPacket(); LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.processId), msg.eventType, msg.ctrlFlags)); diff --git a/src/interactivity/base/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp index f8a2ecd9a2c..49ebf45d844 100644 --- a/src/interactivity/base/HostSignalInputThread.hpp +++ b/src/interactivity/base/HostSignalInputThread.hpp @@ -36,7 +36,7 @@ namespace Microsoft::Console private: template - T _ReaderHelper(); + T _ReceiveTypedPacket(); [[nodiscard]] HRESULT _InputThread(); bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 4c1a4eb1bfb..57b78af198e 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -17,7 +17,7 @@ RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : #pragma region IConsoleControl Members template -[[nodiscard]] NTSTATUS _PacketSender(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) +[[nodiscard]] NTSTATUS _SendTypedPacket(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) { struct HostSignalPacket { @@ -44,7 +44,7 @@ template data.sizeInBytes = sizeof(data); data.processId = dwProcessId; - return _PacketSender(_pipe.get(), HostSignals::NotifyApp, data); + return _SendTypedPacket(_pipe.get(), HostSignals::NotifyApp, data); } [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) @@ -54,7 +54,7 @@ template data.processId = HandleToULong(hProcess); data.isForeground = fForeground; - return _PacketSender(_pipe.get(), HostSignals::SetForeground, data); + return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); } [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) @@ -65,7 +65,7 @@ template data.eventType = dwEventType; data.ctrlFlags = ulCtrlFlags; - return _PacketSender(_pipe.get(), HostSignals::EndTask, data); + return _SendTypedPacket(_pipe.get(), HostSignals::EndTask, data); } #pragma endregion From d46a67aada54f80aae916f1df4abd2e1538a3b8d Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Mon, 14 Jun 2021 15:02:49 -0700 Subject: [PATCH 15/18] pack the structure to ensure there aren't 0s between the code and packet --- src/interactivity/base/RemoteConsoleControl.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 57b78af198e..bf816436509 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -19,11 +19,14 @@ RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : template [[nodiscard]] NTSTATUS _SendTypedPacket(HANDLE pipe, ::Microsoft::Console::HostSignals signalCode, T& payload) { + // To ensure it's a happy wire format, pack it tight at 1. +#pragma pack(push, 1) struct HostSignalPacket { ::Microsoft::Console::HostSignals code; T data; }; +#pragma pack(pop) HostSignalPacket packet; packet.code = signalCode; From ed5cb2c6bf7050bb78eaacdb5114c7039e2b9796 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Tue, 15 Jun 2021 16:48:01 -0700 Subject: [PATCH 16/18] Assorted feedback for modernizing thread and style --- src/host/PtySignalInputThread.cpp | 2 +- src/host/exe/CConsoleHandoff.cpp | 4 +- src/host/srvinit.cpp | 1 + src/inc/LibraryIncludes.h | 1 + .../base/HostSignalInputThread.cpp | 80 +++++++------------ .../base/HostSignalInputThread.hpp | 13 ++- .../base/RemoteConsoleControl.cpp | 17 ++-- src/interactivity/base/ServiceLocator.cpp | 8 +- src/server/IoDispatchers.cpp | 2 +- 9 files changed, 52 insertions(+), 76 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 62468a4b0d9..12197c8bc85 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -26,7 +26,7 @@ using namespace Microsoft::Console::VirtualTerminal; // - Creates the PTY Signal Input Thread. // Arguments: // - hPipe - a handle to the file representing the read end of the VT pipe. -PtySignalInputThread::PtySignalInputThread(_In_ wil::unique_hfile hPipe) : +PtySignalInputThread::PtySignalInputThread(wil::unique_hfile hPipe) : _hFile{ std::move(hPipe) }, _hThread{}, _pConApi{ std::make_unique(ServiceLocator::LocateGlobals().getConsoleInformation()) }, diff --git a/src/host/exe/CConsoleHandoff.cpp b/src/host/exe/CConsoleHandoff.cpp index d04cde821a8..eaf28088a22 100644 --- a/src/host/exe/CConsoleHandoff.cpp +++ b/src/host/exe/CConsoleHandoff.cpp @@ -67,15 +67,13 @@ try RETURN_IF_FAILED(ConsoleEstablishHandoff(server, inputEvent, signalPipe, inboxProcess, &apiMsg)); // Give back a copy of our own process handle to be tracked. - wil::unique_handle duplicatedHandle; RETURN_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), GetCurrentProcess(), GetCurrentProcess(), - &duplicatedHandle, + process, SYNCHRONIZE, FALSE, 0)); - *process = duplicatedHandle.release(); return S_OK; } diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index ed1fd93731b..c1ed6020d33 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -401,6 +401,7 @@ try }, nullptr, nullptr)); + RETURN_LAST_ERROR_IF_NULL(g.handoffInboxConsoleExitWait.get()); SetThreadpoolWait(g.handoffInboxConsoleExitWait.get(), g.handoffInboxConsoleHandle.get(), nullptr); diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h index 996f68d247d..b19b42f16a6 100644 --- a/src/inc/LibraryIncludes.h +++ b/src/inc/LibraryIncludes.h @@ -53,6 +53,7 @@ // WIL #include #include +#include #include #include #include diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index c74fb889d0e..eaf640248cc 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -15,13 +15,13 @@ using namespace Microsoft::Console::Interactivity; // - Creates the PTY Signal Input Thread. // Arguments: // - hPipe - a handle to the file representing the read end of the Host Signal pipe. -HostSignalInputThread::HostSignalInputThread(_In_ wil::unique_hfile&& hPipe) : +HostSignalInputThread::HostSignalInputThread(wil::unique_hfile&& hPipe) : _hFile{ std::move(hPipe) }, _hThread{}, - _dwThreadId{ 0 }, - _consoleConnected{ false } + _dwThreadId{} { THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); + THROW_HR_IF(E_HANDLE, _hFile.get() == nullptr); } HostSignalInputThread::~HostSignalInputThread() @@ -39,26 +39,12 @@ HostSignalInputThread::~HostSignalInputThread() // - lpParameter - A pointer to the HostSignalInputThread instance that should be called. // Return Value: // - The return value of the underlying instance's _InputThread -DWORD WINAPI HostSignalInputThread::StaticThreadProc(_In_ LPVOID lpParameter) +DWORD WINAPI HostSignalInputThread::StaticThreadProc(LPVOID lpParameter) { - HostSignalInputThread* const pInstance = reinterpret_cast(lpParameter); + HostSignalInputThread* const pInstance = static_cast(lpParameter); return pInstance->_InputThread(); } -// Method Description: -// - Tell us that there's a client attached to the console, so we can actually -// do something with the messages we receive now. Before this is set, there -// is no guarantee that a client has attached, so most parts of the console -// (in and screen buffers) haven't yet been initialized. -// Arguments: -// - -// Return Value: -// - -void HostSignalInputThread::ConnectConsole() noexcept -{ - _consoleConnected = true; -} - // Method Description: // - Attempts to retrieve a given type T off of the internal // pipe channel and return it. @@ -70,22 +56,17 @@ template T HostSignalInputThread::_ReceiveTypedPacket() { T msg = { 0 }; - THROW_HR_IF(E_ABORT, !_GetData(&msg, sizeof(msg))); + THROW_HR_IF(E_ABORT, !_GetData(gsl::as_writable_bytes(gsl::span{ &msg, 1 }))); + + // If the message is smaller than what we expected + // then it was malformed and we need to throw. + THROW_HR_IF(E_ILLEGAL_METHOD_CALL, msg.sizeInBytes < sizeof(msg)); // If the message size was stated to be larger, we // want to seek forward to the next message code. // If it's equal, we'll seek forward by 0 and // do nothing. - if (msg.sizeInBytes >= sizeof(msg)) - { - _AdvanceReader(msg.sizeInBytes - sizeof(msg)); - } - // If the message is smaller than what we expected - // then it was malformed and we need to throw. - else - { - THROW_HR(E_ILLEGAL_METHOD_CALL); - } + _AdvanceReader(msg.sizeInBytes - sizeof(msg)); return msg; } @@ -98,7 +79,8 @@ T HostSignalInputThread::_ReceiveTypedPacket() [[nodiscard]] HRESULT HostSignalInputThread::_InputThread() { HostSignals signalId; - while (_GetData(&signalId, sizeof(signalId))) + + while (_GetData(gsl::as_writable_bytes(gsl::span{ &signalId, 1 }))) { switch (signalId) { @@ -139,24 +121,23 @@ T HostSignalInputThread::_ReceiveTypedPacket() // Method Description: // - Skips the file stream forward by the specified number of bytes. // Arguments: -// - cbBytes - Count of bytes to skip forward +// - byteCount - Count of bytes to skip forward // Return Value: // - True if we could skip forward successfully. False otherwise. -bool HostSignalInputThread::_AdvanceReader(const DWORD cbBytes) +bool HostSignalInputThread::_AdvanceReader(DWORD byteCount) { - std::array buffer; - auto bytesRemaining = cbBytes; + std::array buffer; - while (bytesRemaining > 0) + while (byteCount > 0) { - const auto advance = std::min(bytesRemaining, gsl::narrow_cast(buffer.max_size())); + const auto advance = std::min(byteCount, gsl::narrow_cast(buffer.max_size())); - if (!_GetData(buffer.data(), advance)) + if (!_GetData(buffer)) { return false; } - bytesRemaining -= advance; + byteCount -= advance; } return true; @@ -166,19 +147,17 @@ bool HostSignalInputThread::_AdvanceReader(const DWORD cbBytes) // - Retrieves bytes from the file stream and exits or throws errors should the pipe state // be compromised. // Arguments: -// - pBuffer - Buffer to fill with data. -// - cbBuffer - Count of bytes in the given buffer. +// - buffer - Buffer to fill with data. // Return Value: // - True if data was retrieved successfully. False otherwise. -bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, - const DWORD cbBuffer) +bool HostSignalInputThread::_GetData(gsl::span buffer) { - DWORD dwRead = 0; + DWORD bytesRead = 0; // If we failed to read because the terminal broke our pipe (usually due // to dying itself), close gracefully with ERROR_BROKEN_PIPE. // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that // we want to gracefully close in. - if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) + if (FALSE == ReadFile(_hFile.get(), buffer.data(), gsl::narrow_cast(buffer.size()), &bytesRead, nullptr)) { DWORD lastError = GetLastError(); if (lastError == ERROR_BROKEN_PIPE) @@ -186,12 +165,11 @@ bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pB _Shutdown(); return false; } - else - { - THROW_WIN32(lastError); - } + + THROW_WIN32(lastError); } - else if (dwRead != cbBuffer) + + if (bytesRead != buffer.size()) { _Shutdown(); return false; @@ -204,8 +182,6 @@ bool HostSignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pB // - Starts the PTY Signal input thread. [[nodiscard]] HRESULT HostSignalInputThread::Start() noexcept { - RETURN_LAST_ERROR_IF(!_hFile); - // 0 is the right value, https://devblogs.microsoft.com/oldnewthing/20040223-00/?p=40503 _dwThreadId = 0; diff --git a/src/interactivity/base/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp index 49ebf45d844..ba5c7f95680 100644 --- a/src/interactivity/base/HostSignalInputThread.hpp +++ b/src/interactivity/base/HostSignalInputThread.hpp @@ -22,30 +22,27 @@ namespace Microsoft::Console class HostSignalInputThread final { public: - HostSignalInputThread(_In_ wil::unique_hfile&& hPipe); + HostSignalInputThread(wil::unique_hfile&& hPipe); ~HostSignalInputThread(); [[nodiscard]] HRESULT Start() noexcept; - static DWORD WINAPI StaticThreadProc(_In_ LPVOID lpParameter); + static DWORD WINAPI StaticThreadProc(LPVOID lpParameter); // Prevent copying and assignment. HostSignalInputThread(const HostSignalInputThread&) = delete; HostSignalInputThread& operator=(const HostSignalInputThread&) = delete; - void ConnectConsole() noexcept; - private: template T _ReceiveTypedPacket(); [[nodiscard]] HRESULT _InputThread(); - bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); - bool _AdvanceReader(const DWORD cbBytes); + bool _GetData(gsl::span buffer); + bool _AdvanceReader(DWORD byteCount); void _Shutdown(); + DWORD _dwThreadId; wil::unique_hfile _hFile; wil::unique_handle _hThread; - DWORD _dwThreadId; - bool _consoleConnected; }; } diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index bf816436509..21b8155a79f 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -32,10 +32,15 @@ template packet.code = signalCode; packet.data = payload; - DWORD dwWritten = 0; - if (!WriteFile(pipe, &packet, sizeof(packet), &dwWritten, nullptr)) + DWORD bytesWritten = 0; + if (!WriteFile(pipe, &packet, sizeof(packet), &bytesWritten, nullptr)) { - return NTSTATUS_FROM_WIN32(::GetLastError()); + NT_RETURN_NTSTATUS(static_cast(NTSTATUS_FROM_WIN32(::GetLastError()))); + } + + if (bytesWritten != sizeof(packet)) + { + NT_RETURN_NTSTATUS(static_cast(NTSTATUS_FROM_WIN32(E_UNEXPECTED))); } return STATUS_SUCCESS; @@ -43,7 +48,7 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::NotifyConsoleApplication(_In_ DWORD dwProcessId) { - HostSignalNotifyAppData data = { 0 }; + HostSignalNotifyAppData data{}; data.sizeInBytes = sizeof(data); data.processId = dwProcessId; @@ -52,7 +57,7 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) { - HostSignalSetForegroundData data = { 0 }; + HostSignalSetForegroundData data{}; data.sizeInBytes = sizeof(data); data.processId = HandleToULong(hProcess); data.isForeground = fForeground; @@ -62,7 +67,7 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { - HostSignalEndTaskData data = { 0 }; + HostSignalEndTaskData data{}; data.sizeInBytes = sizeof(data); data.processId = HandleToULong(hProcessId); data.eventType = dwEventType; diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 1e67be8b602..40fad8e9cbf 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -106,22 +106,20 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) [[nodiscard]] NTSTATUS ServiceLocator::SetConsoleControlInstance(_In_ std::unique_ptr&& control) { - NTSTATUS status = STATUS_SUCCESS; - if (s_consoleControl) { - status = STATUS_INVALID_HANDLE; + NT_RETURN_NTSTATUS(STATUS_INVALID_HANDLE); } else if (!control) { - status = STATUS_INVALID_PARAMETER; + NT_RETURN_NTSTATUS(STATUS_INVALID_PARAMETER); } else { s_consoleControl = std::move(control); } - return status; + return STATUS_SUCCESS; } [[nodiscard]] NTSTATUS ServiceLocator::SetConsoleWindowInstance(_In_ IConsoleWindow* window) diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index 20f891f1c31..1d76c6d4c30 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -307,7 +307,7 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), GetCurrentProcess(), GetCurrentProcess(), - &ourProcess, + ourProcess.addressof(), SYNCHRONIZE, FALSE, 0)); From b54983dc7acb3a714ce6447c34179e6001678783 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Tue, 15 Jun 2021 16:48:59 -0700 Subject: [PATCH 17/18] code format --- src/interactivity/base/HostSignalInputThread.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index eaf640248cc..00d25c9fca0 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -79,7 +79,7 @@ T HostSignalInputThread::_ReceiveTypedPacket() [[nodiscard]] HRESULT HostSignalInputThread::_InputThread() { HostSignals signalId; - + while (_GetData(gsl::as_writable_bytes(gsl::span{ &signalId, 1 }))) { switch (signalId) From c276eda28ae84037c8af52f4c162b94517bfbd93 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Wed, 16 Jun 2021 11:57:38 -0700 Subject: [PATCH 18/18] For reasons I cannot discern, I must use gsl::byte here or the OS build won't build this. --- src/interactivity/base/HostSignalInputThread.cpp | 4 ++-- src/interactivity/base/HostSignalInputThread.hpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 00d25c9fca0..2dc71ccca20 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -126,7 +126,7 @@ T HostSignalInputThread::_ReceiveTypedPacket() // - True if we could skip forward successfully. False otherwise. bool HostSignalInputThread::_AdvanceReader(DWORD byteCount) { - std::array buffer; + std::array buffer; while (byteCount > 0) { @@ -150,7 +150,7 @@ bool HostSignalInputThread::_AdvanceReader(DWORD byteCount) // - buffer - Buffer to fill with data. // Return Value: // - True if data was retrieved successfully. False otherwise. -bool HostSignalInputThread::_GetData(gsl::span buffer) +bool HostSignalInputThread::_GetData(gsl::span buffer) { DWORD bytesRead = 0; // If we failed to read because the terminal broke our pipe (usually due diff --git a/src/interactivity/base/HostSignalInputThread.hpp b/src/interactivity/base/HostSignalInputThread.hpp index ba5c7f95680..429e492955b 100644 --- a/src/interactivity/base/HostSignalInputThread.hpp +++ b/src/interactivity/base/HostSignalInputThread.hpp @@ -37,7 +37,7 @@ namespace Microsoft::Console T _ReceiveTypedPacket(); [[nodiscard]] HRESULT _InputThread(); - bool _GetData(gsl::span buffer); + bool _GetData(gsl::span buffer); bool _AdvanceReader(DWORD byteCount); void _Shutdown();