From 7dedf50b14a58b597201684eab827ccb40d3ab84 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 17 Jan 2022 17:57:10 +0100 Subject: [PATCH 1/5] Fix unbalanced unlock in PtySignalInputThread::_Shutdown --- src/host/PtySignalInputThread.cpp | 8 -------- src/interactivity/base/ServiceLocator.cpp | 4 ++-- src/interactivity/inc/ServiceLocator.hpp | 2 +- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 3855a467ab5..2b34b09e5e9 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -227,14 +227,6 @@ void PtySignalInputThread::_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/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 577caad3617..3bc691abdc6 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -34,7 +34,7 @@ wil::unique_hwnd ServiceLocator::s_pseudoWindow = nullptr; #pragma region Public Methods -void ServiceLocator::RundownAndExit(const HRESULT hr) +[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr) { // MSFT:15506250 // In VT I/O Mode, a client application might die before we've rendered @@ -67,7 +67,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) s_inputServices.reset(nullptr); } - TerminateProcess(GetCurrentProcess(), hr); + ExitProcess(hr); } #pragma region Creation Methods diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index 583efb2540c..41e27bcdf9f 100644 --- a/src/interactivity/inc/ServiceLocator.hpp +++ b/src/interactivity/inc/ServiceLocator.hpp @@ -30,7 +30,7 @@ namespace Microsoft::Console::Interactivity class ServiceLocator final { public: - static void RundownAndExit(const HRESULT hr); + [[noreturn]] static void RundownAndExit(const HRESULT hr); // N.B.: Location methods without corresponding creation methods // automatically create the singleton object on demand. From 169a78dadd978f01785a1a985b70780769a9ed64 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 18 Jan 2022 15:02:08 +0100 Subject: [PATCH 2/5] Improve robustness of CloseConsoleProcessState --- src/host/VtIo.cpp | 8 -------- src/host/output.cpp | 12 ++++-------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index f7ac039bc5c..130b06c2023 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -377,14 +377,6 @@ void VtIo::_ShutdownIfNeeded() // if we don't, this will cause us to rundown and exit. 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/output.cpp b/src/host/output.cpp index c07cb566e10..ecca89139fc 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -512,13 +512,9 @@ void CloseConsoleProcessState() HandleCtrlEvent(CTRL_CLOSE_EVENT); - // Jiggle the handle: (see MSFT:19419231) - // When we call this function, we'll only actually close the console once - // we're totally unlocked. If our caller has the console locked, great, - // we'll dispatch the ctrl event once they unlock. However, if they're - // not running under lock (eg PtySignalInputThread::_GetData), then the - // ctrl event will never actually get dispatched. - // So, lock and unlock here, to make sure the ctrl event gets handled. + // MSFT:19419231 + // Ensure ctrl events are processed before exiting, + // no matter whether the console is locked or not. LockConsole(); - auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); + ProcessCtrlEvents(); } From 609497f89e4b74c494246884667a3e627340529a Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 18 Jan 2022 18:50:41 +0100 Subject: [PATCH 3/5] Fix audit checks --- src/host/PtySignalInputThread.cpp | 2 -- src/interactivity/base/HostSignalInputThread.cpp | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 2b34b09e5e9..4c5cf3d81ba 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -170,7 +170,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu if (lastError == ERROR_BROKEN_PIPE) { _Shutdown(); - return false; } else { @@ -180,7 +179,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu else if (dwRead != cbBuffer) { _Shutdown(); - return false; } return true; diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 2dc71ccca20..1c2abeb471a 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -163,7 +163,6 @@ bool HostSignalInputThread::_GetData(gsl::span buffer) if (lastError == ERROR_BROKEN_PIPE) { _Shutdown(); - return false; } THROW_WIN32(lastError); @@ -172,7 +171,6 @@ bool HostSignalInputThread::_GetData(gsl::span buffer) if (bytesRead != buffer.size()) { _Shutdown(); - return false; } return true; From 6fe9843f728a935fb350abeeeda3d9c3d13cfa4e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 21 Jan 2022 18:40:53 +0100 Subject: [PATCH 4/5] Simplify pull request --- src/host/PtySignalInputThread.cpp | 9 +++++++++ src/host/VtIo.cpp | 9 +++++++++ src/host/output.cpp | 12 ++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 4c5cf3d81ba..16b330085e6 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -225,6 +225,15 @@ void PtySignalInputThread::_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. + LockConsole(); + ProcessCtrlEvents(); + // Make sure we terminate. ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); } diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 130b06c2023..70ad6ab6975 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -377,6 +377,15 @@ void VtIo::_ShutdownIfNeeded() // if we don't, this will cause us to rundown and exit. 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. + LockConsole(); + ProcessCtrlEvents(); + // Make sure we terminate. ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); } diff --git a/src/host/output.cpp b/src/host/output.cpp index ecca89139fc..a5f539ccc77 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -512,9 +512,13 @@ void CloseConsoleProcessState() HandleCtrlEvent(CTRL_CLOSE_EVENT); - // MSFT:19419231 - // Ensure ctrl events are processed before exiting, - // no matter whether the console is locked or not. + // Jiggle the handle: (see MSFT:19419231) + // When we call this function, we'll only actually close the console once + // we're totally unlocked. If our caller has the console locked, great, + // we'll dispatch the ctrl event once they unlock. However, if they're + // not running under lock (eg PtySignalInputThread::_GetData), then the + // ctrl event will never actually get dispatched. + // So, lock and unlock here, to make sure the ctrl event gets handled. LockConsole(); - ProcessCtrlEvents(); + UnlockConsole(); } From c12b27ac5adf812e86673d5e74221d3b20f18062 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 21 Jan 2022 22:03:39 +0100 Subject: [PATCH 5/5] Fix build --- src/host/VtIo.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 70ad6ab6975..5b482ecd185 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -10,6 +10,7 @@ #include "../renderer/base/renderer.hpp" #include "../types/inc/utils.hpp" +#include "handle.h" // LockConsole #include "input.h" // ProcessCtrlEvents #include "output.h" // CloseConsoleProcessState