From 1ac221a7a5fa2f5a24b9fa8955367fe89469a67b Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 17 Jul 2024 03:02:59 +0200 Subject: [PATCH] Implement a timeout for PSEUDOCONSOLE_INHERIT_CURSOR (#17574) This implements a 3s timeout for cursor inheritance which prevents ConPTY from being deadlocked at startup, if the terminal misbehaves. It serves another purpose, however, in that it prepares the code for the introduction of overlapped IO in #17510. Closes #11213 --- src/host/VtInputThread.cpp | 150 ++++++++---------- src/host/VtInputThread.hpp | 11 +- src/host/VtIo.cpp | 11 +- .../parser/InputStateMachineEngine.cpp | 23 ++- .../parser/InputStateMachineEngine.hpp | 5 +- 5 files changed, 89 insertions(+), 111 deletions(-) diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 9b2f435e19..2df7ee1498 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -2,17 +2,15 @@ // Licensed under the MIT license. #include "precomp.h" - #include "VtInputThread.hpp" -#include "../interactivity/inc/ServiceLocator.hpp" -#include "input.h" -#include "../terminal/parser/InputStateMachineEngine.hpp" -#include "../terminal/adapter/InteractDispatch.hpp" -#include "../types/inc/convert.hpp" -#include "server.h" -#include "output.h" #include "handle.h" +#include "output.h" +#include "server.h" +#include "../interactivity/inc/ServiceLocator.hpp" +#include "../terminal/adapter/InteractDispatch.hpp" +#include "../terminal/parser/InputStateMachineEngine.hpp" +#include "../types/inc/utils.hpp" using namespace Microsoft::Console; using namespace Microsoft::Console::Interactivity; @@ -23,30 +21,18 @@ using namespace Microsoft::Console::VirtualTerminal; // - hPipe - a handle to the file representing the read end of the VT pipe. // - inheritCursor - a bool indicating if the state machine should expect a // cursor positioning sequence. See MSFT:15681311. -VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, - const bool inheritCursor) : - _hFile{ std::move(hPipe) }, - _hThread{}, - _u8State{}, - _dwThreadId{ 0 }, - _pfnSetLookingForDSR{} +VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) : + _hFile{ std::move(hPipe) } { THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); auto dispatch = std::make_unique(); - auto engine = std::make_unique(std::move(dispatch), inheritCursor); - auto engineRef = engine.get(); + // we need this callback to be able to flush an unknown input sequence to the app + engine->SetFlushToInputQueueCallback([this] { return _pInputStateMachine->FlushToTerminal(); }); _pInputStateMachine = std::make_unique(std::move(engine)); - - // we need this callback to be able to flush an unknown input sequence to the app - auto flushCallback = [capture0 = _pInputStateMachine.get()] { return capture0->FlushToTerminal(); }; - engineRef->SetFlushToInputQueueCallback(flushCallback); - - // we need this callback to capture the reply if someone requests a status from the terminal - _pfnSetLookingForDSR = [engineRef](auto&& PH1) { engineRef->SetLookingForDSR(std::forward(PH1)); }; } // Function Description: @@ -57,79 +43,67 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, // - The return value of the underlying instance's _InputThread DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) { - const auto pInstance = reinterpret_cast(lpParameter); + const auto pInstance = static_cast(lpParameter); pInstance->_InputThread(); return S_OK; } -// Method Description: -// - Do a single ReadFile from our pipe, and try and handle it. If handling -// failed, throw or log, depending on what the caller wants. -// Return Value: -// - true if you should continue reading -bool VtInputThread::DoReadInput() -{ - char buffer[4096]; - DWORD dwRead = 0; - const auto ok = ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr); - - // The ReadFile() documentations calls out that: - // > If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a pipe, the other - // > end of the pipe called the WriteFile function with nNumberOfBytesToWrite set to zero. - // But I was unable to replicate any such behavior. I'm not sure it's true anymore. - // - // However, what the documentations fails to mention is that winsock2 (WSA) handles of the \Device\Afd type are - // transparently compatible with ReadFile() and the WSARecv() documentations contains this important information: - // > For byte streams, zero bytes having been read [..] indicates graceful closure and that no more bytes will ever be read. - // In other words, for pipe HANDLE of unknown type you should consider `lpNumberOfBytesRead == 0` as an exit indicator. - // - // Here, `dwRead == 0` fixes a deadlock when exiting conhost while being in use by WSL whose hypervisor pipes are WSA. - if (!ok || dwRead == 0) - { - return false; - } - - // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it. - if (FAILED_LOG(til::u8u16({ buffer, gsl::narrow_cast(dwRead) }, _wstr, _u8State))) - { - return true; - } - - try - { - // Make sure to call the GLOBAL Lock/Unlock, not the gci's lock/unlock. - // Only the global unlock attempts to dispatch ctrl events. If you use the - // gci's unlock, when you press C-c, it won't be dispatched until the - // next console API call. For something like `powershell sleep 60`, - // that won't happen for 60s - LockConsole(); - const auto unlock = wil::scope_exit([&] { UnlockConsole(); }); - - _pInputStateMachine->ProcessString(_wstr); - } - CATCH_LOG(); - - return true; -} - -void VtInputThread::SetLookingForDSR(const bool looking) noexcept -{ - if (_pfnSetLookingForDSR) - { - _pfnSetLookingForDSR(looking); - } -} - // Method Description: // - The ThreadProc for the VT Input Thread. Reads input from the pipe, and // passes it to _HandleRunInput to be processed by the // InputStateMachineEngine. void VtInputThread::_InputThread() { - while (DoReadInput()) + const auto cleanup = wil::scope_exit([this]() { + ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput(); + }); + + char buffer[4096]; + DWORD dwRead = 0; + + til::u8state u8State; + std::wstring wstr; + + for (;;) { + const auto ok = ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr); + + // The ReadFile() documentations calls out that: + // > If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a pipe, the other + // > end of the pipe called the WriteFile function with nNumberOfBytesToWrite set to zero. + // But I was unable to replicate any such behavior. I'm not sure it's true anymore. + // + // However, what the documentations fails to mention is that winsock2 (WSA) handles of the \Device\Afd type are + // transparently compatible with ReadFile() and the WSARecv() documentations contains this important information: + // > For byte streams, zero bytes having been read [..] indicates graceful closure and that no more bytes will ever be read. + // In other words, for pipe HANDLE of unknown type you should consider `lpNumberOfBytesRead == 0` as an exit indicator. + // + // Here, `dwRead == 0` fixes a deadlock when exiting conhost while being in use by WSL whose hypervisor pipes are WSA. + if (!ok || dwRead == 0) + { + break; + } + + // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it. + if (FAILED_LOG(til::u8u16({ buffer, gsl::narrow_cast(dwRead) }, wstr, u8State))) + { + continue; + } + + try + { + // Make sure to call the GLOBAL Lock/Unlock, not the gci's lock/unlock. + // Only the global unlock attempts to dispatch ctrl events. If you use the + // gci's unlock, when you press C-c, it won't be dispatched until the + // next console API call. For something like `powershell sleep 60`, + // that won't happen for 60s + LockConsole(); + const auto unlock = wil::scope_exit([&] { UnlockConsole(); }); + + _pInputStateMachine->ProcessString(wstr); + } + CATCH_LOG(); } - ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput(); } // Method Description: @@ -156,3 +130,9 @@ void VtInputThread::_InputThread() return S_OK; } + +void VtInputThread::WaitUntilDSR(DWORD timeout) const noexcept +{ + const auto& engine = static_cast(_pInputStateMachine->Engine()); + engine.WaitUntilDSR(timeout); +} diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index d058c425bc..e6f15e2ab6 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -24,21 +24,16 @@ namespace Microsoft::Console VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor); [[nodiscard]] HRESULT Start(); - static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter); - bool DoReadInput(); - void SetLookingForDSR(const bool looking) noexcept; + void WaitUntilDSR(DWORD timeout) const noexcept; private: + static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter); void _InputThread(); wil::unique_hfile _hFile; wil::unique_handle _hThread; - DWORD _dwThreadId; - - std::function _pfnSetLookingForDSR; + DWORD _dwThreadId = 0; std::unique_ptr _pInputStateMachine; - til::u8state _u8State; - std::wstring _wstr; }; } diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 7b0c9af37f..1511b7e34b 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -186,19 +186,14 @@ bool VtIo::IsUsingVt() const // 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, then - // read input until we get a response. Terminals who request this behavior - // but don't respond will hang. + // wait 3s until we get a response. // If we get a response, the InteractDispatch will call SetCursorPosition, // which will call to our VtIo::SetCursorPosition method. - // We need both handles for this initialization to work. If we don't have - // both, we'll skip it. They either aren't going to be reading output - // (so they can't get the DSR) or they can't write the response to us. if (_lookingForCursorPosition && _pVtRenderEngine && _pVtInputThread) { + _lookingForCursorPosition = false; LOG_IF_FAILED(_pVtRenderEngine->RequestCursor()); - while (_lookingForCursorPosition && _pVtInputThread->DoReadInput()) - { - } + _pVtInputThread->WaitUntilDSR(3000); } // GH#4999 - Send a sequence to the connected terminal to request diff --git a/src/terminal/parser/InputStateMachineEngine.cpp b/src/terminal/parser/InputStateMachineEngine.cpp index 4a25d056f4..debcbe7fe7 100644 --- a/src/terminal/parser/InputStateMachineEngine.cpp +++ b/src/terminal/parser/InputStateMachineEngine.cpp @@ -6,8 +6,9 @@ #include "stateMachine.hpp" #include "InputStateMachineEngine.hpp" +#include + #include "../../inc/unicode.hpp" -#include "ascii.hpp" #include "../../interactivity/inc/VtApiRedirection.hpp" using namespace Microsoft::Console::VirtualTerminal; @@ -102,16 +103,21 @@ InputStateMachineEngine::InputStateMachineEngine(std::unique_ptrMoveCursor(parameters.at(0), parameters.at(1)); // Right now we're only looking for on initial cursor // position response. After that, only look for F3. - _lookingForDSR = false; + _lookingForDSR.store(false, std::memory_order::relaxed); + til::atomic_notify_all(_lookingForDSR); break; } [[fallthrough]]; diff --git a/src/terminal/parser/InputStateMachineEngine.hpp b/src/terminal/parser/InputStateMachineEngine.hpp index 4b17669aed..32159f6f94 100644 --- a/src/terminal/parser/InputStateMachineEngine.hpp +++ b/src/terminal/parser/InputStateMachineEngine.hpp @@ -132,8 +132,9 @@ namespace Microsoft::Console::VirtualTerminal InputStateMachineEngine(std::unique_ptr pDispatch, const bool lookingForDSR); + void WaitUntilDSR(DWORD timeout) const noexcept; + bool EncounteredWin32InputModeSequence() const noexcept override; - void SetLookingForDSR(const bool looking) noexcept; bool ActionExecute(const wchar_t wch) override; bool ActionExecuteFromEscape(const wchar_t wch) override; @@ -165,7 +166,7 @@ namespace Microsoft::Console::VirtualTerminal private: const std::unique_ptr _pDispatch; std::function _pfnFlushToInputQueue; - bool _lookingForDSR; + std::atomic _lookingForDSR{ false }; bool _encounteredWin32InputModeSequence = false; DWORD _mouseButtonState = 0; std::chrono::milliseconds _doubleClickTime;