maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix startup race of resizing ConPTY (#10449)

Browse source 
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
This commit is contained in:
Michael Niksa 2021-06-22 12:23:16 -07:00 committed by GitHub
parent b7fc0f2d44
commit 1b79cc87c3
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 42 additions and 65 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
src/host/ConsoleArguments.cpp
View file

@ -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:
// - <none>
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

9
src/host/ConsoleArguments.hpp
View file

@ -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<std::wstring>& args,
const size_t index,
const bool skipFirst);

52
src/host/PtySignalInputThread.cpp
View file

@ -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:
// - <none>
// 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:
// - <none>
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.

13
src/host/PtySignalInputThread.hpp
View file

@ -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<ResizeWindowData> _earlyResize;
std::unique_ptr<Microsoft::Console::VirtualTerminal::ConGetSet> _pConApi;
};
}