From 863cdd44f2663c7afad922a9e63c3b0e87ea46d3 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 21 Feb 2025 14:57:37 -0800 Subject: [PATCH] ConPTY: Fix shutdown if killed during startup (#18588) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During startup we relinquish ownership of the console lock to wait for the DA1 response of the hosting terminal. The problem occurs if the hosting terminal disconnects during that time. The broken pipe will cause `VtIo` to send out `CTRL_CLOSE_EVENT` messages, but those won't achieve anything, because the first and only client hasn't even finished connecting yet. What we need to do instead is to return an error code. In order to not use a bunch of booleans to control this behavior, I gave `VtIo` a state enum. This however required restructuring the calling code in order to not have a dozen states. ## Validation Steps Performed * Launch cmd.exe with ConPTY * ...but leave the stdin pipe unbound (which will hang the DA1 request) * Immediately kill the ConPTY session * cmd.exe exits after clicking away the error message ✅ (cherry picked from commit 733a5e7becd86400323c0536a86d88021b3748fb) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXjzIg Service-Version: 1.23 --- src/host/VtIo.cpp | 197 ++++++++++++++++++++----------------------- src/host/VtIo.hpp | 15 +++- src/host/srvinit.cpp | 27 ++---- 3 files changed, 109 insertions(+), 130 deletions(-) diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 1b8a8591d2..be1d8bd330 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -79,7 +79,11 @@ using namespace Microsoft::Console::Interactivity; const HANDLE OutHandle, _In_opt_ const HANDLE SignalHandle) { - FAIL_FAST_IF_MSG(_initialized, "Someone attempted to double-_Initialize VtIo"); + if (_state != State::Uninitialized) + { + assert(false); // Don't call initialize twice. + return E_UNEXPECTED; + } _hInput.reset(InHandle); _hOutput.reset(OutHandle); @@ -95,47 +99,33 @@ using namespace Microsoft::Console::Interactivity; } } + // - Create and start the signal thread. The signal thread can be created + // independent of the i/o threads, and doesn't require a client first + // attaching to the console. We need to create it first and foremost, + // because it's possible that a terminal application could + // CreatePseudoConsole, then ClosePseudoConsole without ever attaching a + // client. Should that happen, we still need to exit. + if (IsValidHandle(_hSignal.get())) + { + try + { + _pPtySignalInputThread = std::make_unique(std::move(_hSignal)); + + // Start it if it was successfully created. + RETURN_IF_FAILED(_pPtySignalInputThread->Start()); + } + CATCH_RETURN(); + } + // The only way we're initialized is if the args said we're in conpty mode. // If the args say so, then at least one of in, out, or signal was specified - _initialized = true; - return S_OK; -} - -// Method Description: -// - Create the VtEngine and the VtInputThread for this console. -// MUST BE DONE AFTER CONSOLE IS INITIALIZED, to make sure we've gotten the -// buffer size from the attached client application. -// Arguments: -// - -// Return Value: -// S_OK if we initialized successfully, -// S_FALSE if VtIo hasn't been initialized (or we're not in conpty mode) -// otherwise an appropriate HRESULT indicating failure. -[[nodiscard]] HRESULT VtIo::CreateIoHandlers() noexcept -{ - if (!_initialized) - { - return S_FALSE; - } - - // SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine. - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); - - try - { - if (IsValidHandle(_hInput.get())) - { - _pVtInputThread = std::make_unique(std::move(_hInput), _lookingForCursorPosition); - } - } - CATCH_RETURN(); - + _state = State::Initialized; return S_OK; } bool VtIo::IsUsingVt() const { - return _initialized; + return _state != State::Uninitialized; } // Routine Description: @@ -151,50 +141,64 @@ bool VtIo::IsUsingVt() const [[nodiscard]] HRESULT VtIo::StartIfNeeded() { // If we haven't been set up, do nothing (because there's nothing to start) - if (!_initialized) + if (_state != State::Initialized) { return S_FALSE; } + _state = State::Starting; + + // SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine. + assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + + try + { + if (IsValidHandle(_hInput.get())) + { + _pVtInputThread = std::make_unique(std::move(_hInput), _lookingForCursorPosition); + } + } + CATCH_RETURN(); + if (_pVtInputThread) { LOG_IF_FAILED(_pVtInputThread->Start()); - } - { - Writer writer{ this }; - - // MSFT: 15813316 - // If the terminal application wants us to inherit the cursor position, - // we're going to emit a VT sequence to ask for the cursor position. - // If we get a response, the InteractDispatch will call SetCursorPosition, - // which will call to our VtIo::SetCursorPosition method. - // - // By sending the request before sending the DA1 one, we can simply - // wait for the DA1 response below and effectively wait for both. - if (_lookingForCursorPosition) { - writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR) + Writer writer{ this }; + + // MSFT: 15813316 + // If the terminal application wants us to inherit the cursor position, + // we're going to emit a VT sequence to ask for the cursor position. + // If we get a response, the InteractDispatch will call SetCursorPosition, + // which will call to our VtIo::SetCursorPosition method. + // + // By sending the request before sending the DA1 one, we can simply + // wait for the DA1 response below and effectively wait for both. + if (_lookingForCursorPosition) + { + writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR) + } + + // GH#4999 - Send a sequence to the connected terminal to request + // win32-input-mode from them. This will enable the connected terminal to + // send us full INPUT_RECORDs as input. If the terminal doesn't understand + // this sequence, it'll just ignore it. + writer.WriteUTF8( + "\x1b[c" // DA1 Report (Primary Device Attributes) + "\x1b[?1004h" // Focus Event Mode + "\x1b[?9001h" // Win32 Input Mode + ); + + writer.Submit(); } - // GH#4999 - Send a sequence to the connected terminal to request - // win32-input-mode from them. This will enable the connected terminal to - // send us full INPUT_RECORDs as input. If the terminal doesn't understand - // this sequence, it'll just ignore it. - writer.WriteUTF8( - "\x1b[c" // DA1 Report (Primary Device Attributes) - "\x1b[?1004h" // Focus Event Mode - "\x1b[?9001h" // Win32 Input Mode - ); - - writer.Submit(); - } - - { - // Allow the input thread to momentarily gain the console lock. - auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - const auto suspension = gci.SuspendLock(); - _deviceAttributes = _pVtInputThread->WaitUntilDA1(3000); + { + // Allow the input thread to momentarily gain the console lock. + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + const auto suspension = gci.SuspendLock(); + _deviceAttributes = _pVtInputThread->WaitUntilDA1(3000); + } } if (_pPtySignalInputThread) @@ -208,6 +212,17 @@ bool VtIo::IsUsingVt() const _pPtySignalInputThread->ConnectConsole(); } + if (_state != State::Starting) + { + // Here's where we _could_ call CloseConsoleProcessState(), but this function + // only gets called once when the first client connects and CONSOLE_INITIALIZED + // is not set yet. The process list may already contain that first client, + // but since it hasn't finished connecting yet, it won't react to a CTRL_CLOSE_EVENT. + // Instead, we return an error here which will abort the connection setup. + return E_FAIL; + } + + _state = State::Running; return S_OK; } @@ -244,47 +259,21 @@ void VtIo::CreatePseudoWindow() } } -// Method Description: -// - Create and start the signal thread. The signal thread can be created -// independent of the i/o threads, and doesn't require a client first -// attaching to the console. We need to create it first and foremost, -// because it's possible that a terminal application could -// CreatePseudoConsole, then ClosePseudoConsole without ever attaching a -// client. Should that happen, we still need to exit. -// Arguments: -// - -// Return Value: -// - S_FALSE if we're not in VtIo mode, -// S_OK if we succeeded, -// otherwise an appropriate HRESULT indicating failure. -[[nodiscard]] HRESULT VtIo::CreateAndStartSignalThread() noexcept -{ - if (!_initialized) - { - return S_FALSE; - } - - // If we were passed a signal handle, try to open it and make a signal reading thread. - if (IsValidHandle(_hSignal.get())) - { - try - { - _pPtySignalInputThread = std::make_unique(std::move(_hSignal)); - - // Start it if it was successfully created. - RETURN_IF_FAILED(_pPtySignalInputThread->Start()); - } - CATCH_RETURN(); - } - - return S_OK; -} - void VtIo::SendCloseEvent() { LockConsole(); const auto unlock = wil::scope_exit([] { UnlockConsole(); }); + // If we're still in the process of starting up, and we're asked to shut down + // (broken pipe), `VtIo::StartIfNeeded()` will handle the cleanup for us. + // This can happen during the call to `WaitUntilDA1`, because we relinquish + // ownership of the console lock. + if (_state == State::Starting) + { + _state = State::StartupFailed; + return; + } + // This function is called when the ConPTY signal pipe is closed (PtySignalInputThread) and when the input // pipe is closed (VtIo). Usually these two happen at about the same time. This if condition is a bit of // a premature optimization and prevents us from sending out a CTRL_CLOSE_EVENT right after another. diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index 4e9d250c4c..6805fbc730 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -57,8 +57,6 @@ namespace Microsoft::Console::VirtualTerminal static wchar_t SanitizeUCS2(wchar_t ch); [[nodiscard]] HRESULT Initialize(const ConsoleArguments* const pArgs); - [[nodiscard]] HRESULT CreateAndStartSignalThread() noexcept; - [[nodiscard]] HRESULT CreateIoHandlers() noexcept; bool IsUsingVt() const; [[nodiscard]] HRESULT StartIfNeeded(); @@ -69,6 +67,15 @@ namespace Microsoft::Console::VirtualTerminal void CreatePseudoWindow(); private: + enum class State : uint8_t + { + Uninitialized, + Initialized, + Starting, + StartupFailed, + Running, + }; + [[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, _In_opt_ const HANDLE SignalHandle); void _uncork(); @@ -77,7 +84,7 @@ namespace Microsoft::Console::VirtualTerminal // After CreateIoHandlers is called, these will be invalid. wil::unique_hfile _hInput; wil::unique_hfile _hOutput; - // After CreateAndStartSignalThread is called, this will be invalid. + // After Initialize is called, this will be invalid. wil::unique_hfile _hSignal; std::unique_ptr _pVtInputThread; @@ -96,7 +103,7 @@ namespace Microsoft::Console::VirtualTerminal bool _writerRestoreCursor = false; bool _writerTainted = false; - bool _initialized = false; + State _state = State::Uninitialized; bool _lookingForCursorPosition = false; bool _closeEventSent = false; int _corked = 0; diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 5b10705a63..97bcd8f276 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -381,7 +381,6 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server, // can start, so they're started below, in ConsoleAllocateConsole auto& gci = g.getConsoleInformation(); RETURN_IF_FAILED(gci.GetVtIo()->Initialize(args)); - RETURN_IF_FAILED(gci.GetVtIo()->CreateAndStartSignalThread()); return S_OK; } @@ -945,27 +944,11 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand, // We'll need the size of the screen buffer in the vt i/o initialization if (SUCCEEDED_NTSTATUS(Status)) { - auto hr = gci.GetVtIo()->CreateIoHandlers(); - if (hr == S_FALSE) - { - // We're not in VT I/O mode, this is fine. - } - else if (SUCCEEDED(hr)) - { - // Actually start the VT I/O threads - hr = gci.GetVtIo()->StartIfNeeded(); - // Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS - // is treated as an error - if (hr != S_FALSE) - { - Status = NTSTATUS_FROM_HRESULT(hr); - } - else - { - Status = ERROR_SUCCESS; - } - } - else + // Actually start the VT I/O threads + auto hr = gci.GetVtIo()->StartIfNeeded(); + // Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS + // is treated as an error + if (FAILED(hr)) { Status = NTSTATUS_FROM_HRESULT(hr); }