From 1b79cc87c3740c3ca5c26e4a7d7152981dd46412 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Tue, 22 Jun 2021 12:23:16 -0700 Subject: [PATCH] Fix startup race of resizing ConPTY (#10449) Fix startup race of resizing ConPTY - Depending on what the timing and ordering is of the message coming in from the signal thread, it may be applied to the startup structure after the I/O thread has begun initializing the console buffer structures but before it has signaled that it is done and the signal thread is ready to make changes directly. This likely happens because the end of the I/O thread setup has a weird unlock/lock jog for the input thread and the signal thread might have been scheduled in the middle of it. - My resolution here is to ensure that the signal thread just keeps storing the latest resize message until it is told that everything is initialized. Whomever comes in to tell the signal thread this information (under lock) will pickup and run the resize if one came in before everything was ready. This should resolve the race. ## Validation Steps Performed - o-sdn-o confirms this resolves their issue Closes #10400 --- src/host/ConsoleArguments.cpp | 33 +------------------- src/host/ConsoleArguments.hpp | 9 ------ src/host/PtySignalInputThread.cpp | 52 +++++++++++++++++-------------- src/host/PtySignalInputThread.hpp | 13 ++++++++ 4 files changed, 42 insertions(+), 65 deletions(-) diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index a95bb3800..148a6d321 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -103,10 +103,7 @@ ConsoleArguments::ConsoleArguments(const std::wstring& commandline, const HANDLE hStdOut) : _commandline(commandline), _vtInHandle(hStdIn), - _vtOutHandle(hStdOut), - _receivedEarlySizeChange{ false }, - _originalWidth{ -1 }, - _originalHeight{ -1 } + _vtOutHandle(hStdOut) { _clientCommandline = L""; _vtMode = L""; @@ -144,7 +141,6 @@ ConsoleArguments& ConsoleArguments::operator=(const ConsoleArguments& other) _width = other._width; _height = other._height; _inheritCursor = other._inheritCursor; - _receivedEarlySizeChange = other._receivedEarlySizeChange; _runAsComServer = other._runAsComServer; _forceNoHandoff = other._forceNoHandoff; } @@ -668,33 +664,6 @@ bool ConsoleArguments::IsWin32InputModeEnabled() const return _win32InputMode; } -// Method Description: -// - Tell us to use a different size than the one parsed as the size of the -// console. This is called by the PtySignalInputThread when it receives a -// resize before the first client has connected. Because there's no client, -// there's also no buffer yet, so it has nothing to resize. -// However, we shouldn't just discard that first resize message. Instead, -// store it in here, so we can use the value when the first client does connect. -// Arguments: -// - dimensions: the new size in characters of the conpty buffer & viewport. -// Return Value: -// - -void ConsoleArguments::SetExpectedSize(COORD dimensions) noexcept -{ - _width = dimensions.X; - _height = dimensions.Y; - // Stash away the original values we parsed when this is called. - // This is to help debugging - if the signal thread DOES change these values, - // we can still recover what was given to us on the commandline. - if (!_receivedEarlySizeChange) - { - _originalWidth = _width; - _originalHeight = _height; - // Mark that we've changed size from what our commandline values were - _receivedEarlySizeChange = true; - } -} - #ifdef UNIT_TESTING // Method Description: // - This is a test helper method. It can be used to trick us into thinking diff --git a/src/host/ConsoleArguments.hpp b/src/host/ConsoleArguments.hpp index 5d0e5b31e..d82dc0bf2 100644 --- a/src/host/ConsoleArguments.hpp +++ b/src/host/ConsoleArguments.hpp @@ -56,8 +56,6 @@ public: bool IsResizeQuirkEnabled() const; bool IsWin32InputModeEnabled() const; - void SetExpectedSize(COORD dimensions) noexcept; - #ifdef UNIT_TESTING void EnableConptyModeForTests(); #endif @@ -113,9 +111,6 @@ private: _signalHandle(signalHandle), _inheritCursor(inheritCursor), _resizeQuirk(false), - _receivedEarlySizeChange{ false }, - _originalWidth{ -1 }, - _originalHeight{ -1 }, _runAsComServer{ runAsComServer } { } @@ -146,10 +141,6 @@ private: bool _resizeQuirk{ false }; bool _win32InputMode{ false }; - bool _receivedEarlySizeChange; - short _originalWidth; - short _originalHeight; - [[nodiscard]] HRESULT _GetClientCommandline(_Inout_ std::vector& args, const size_t index, const bool skipFirst); diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 12197c8bc..df42f45f7 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -10,14 +10,6 @@ #include "../interactivity/inc/ServiceLocator.hpp" #include "../terminal/adapter/DispatchCommon.hpp" -#define PTY_SIGNAL_RESIZE_WINDOW 8u - -struct PTY_SIGNAL_RESIZE -{ - unsigned short sx; - unsigned short sy; -}; - using namespace Microsoft::Console; using namespace Microsoft::Console::Interactivity; using namespace Microsoft::Console::VirtualTerminal; @@ -63,6 +55,8 @@ DWORD WINAPI PtySignalInputThread::StaticThreadProc(_In_ LPVOID lpParameter) // 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. +// - NOTE: Call under LockConsole() to ensure other threads have an opportunity +// to set early-work state. // Arguments: // - // Return Value: @@ -70,6 +64,10 @@ DWORD WINAPI PtySignalInputThread::StaticThreadProc(_In_ LPVOID lpParameter) void PtySignalInputThread::ConnectConsole() noexcept { _consoleConnected = true; + if (_earlyResize) + { + _DoResizeWindow(*_earlyResize); + } } // Method Description: @@ -79,38 +77,30 @@ void PtySignalInputThread::ConnectConsole() noexcept // - Otherwise it may cause an application termination and never return. [[nodiscard]] HRESULT PtySignalInputThread::_InputThread() { - unsigned short signalId; + PtySignal signalId; while (_GetData(&signalId, sizeof(signalId))) { switch (signalId) { - case PTY_SIGNAL_RESIZE_WINDOW: + case PtySignal::ResizeWindow: { - PTY_SIGNAL_RESIZE resizeMsg = { 0 }; + ResizeWindowData resizeMsg = { 0 }; _GetData(&resizeMsg, sizeof(resizeMsg)); LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); + // If the client app hasn't yet connected, stash the new size in the launchArgs. // We'll later use the value in launchArgs to set up the console buffer + // We must be under lock here to ensure that someone else doesn't come in + // and set with `ConnectConsole` while we're looking and modifying this. if (!_consoleConnected) { - short sColumns = 0; - short sRows = 0; - if (SUCCEEDED(UShortToShort(resizeMsg.sx, &sColumns)) && - SUCCEEDED(UShortToShort(resizeMsg.sy, &sRows)) && - (sColumns > 0 && sRows > 0)) - { - ServiceLocator::LocateGlobals().launchArgs.SetExpectedSize({ sColumns, sRows }); - } - break; + _earlyResize = resizeMsg; } else { - if (DispatchCommon::s_ResizeWindow(*_pConApi, resizeMsg.sx, resizeMsg.sy)) - { - DispatchCommon::s_SuppressResizeRepaint(*_pConApi); - } + _DoResizeWindow(resizeMsg); } break; @@ -124,6 +114,20 @@ void PtySignalInputThread::ConnectConsole() noexcept return S_OK; } +// Method Description: +// - Dispatches a resize window message to the rest of the console code +// Arguments: +// - data - Packet information containing width/height (size) information +// Return Value: +// - +void PtySignalInputThread::_DoResizeWindow(const ResizeWindowData& data) +{ + if (DispatchCommon::s_ResizeWindow(*_pConApi, data.sx, data.sy)) + { + DispatchCommon::s_SuppressResizeRepaint(*_pConApi); + } +} + // Method Description: // - Retrieves bytes from the file stream and exits or throws errors should the pipe state // be compromised. diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index ea9419a32..d54cf6833 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -33,14 +33,27 @@ namespace Microsoft::Console void ConnectConsole() noexcept; private: + enum class PtySignal : unsigned short + { + ResizeWindow = 8 + }; + + struct ResizeWindowData + { + unsigned short sx; + unsigned short sy; + }; + [[nodiscard]] HRESULT _InputThread(); bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); + void _DoResizeWindow(const ResizeWindowData& data); void _Shutdown(); wil::unique_hfile _hFile; wil::unique_handle _hThread; DWORD _dwThreadId; bool _consoleConnected; + std::optional _earlyResize; std::unique_ptr _pConApi; }; }