Skip to content

Commit

Permalink
Fix a potential deadlock for PtySignal::SetParent (#14463)
Browse files Browse the repository at this point in the history 
This changeset consists of two parts:
* Refactor `PtySignalInputThread` to move more code from `_InputThread`
  into the various `_Do*` handlers. This allows us to precisely control
  console locking behavior which is the cause of this bug.
* Add the 1-line fix to `_DoSetWindowParent` to unlock the console before
  calling foreign functions (`SetWindowLongPtrW` in this case).

This fix is theoretical in nature, based on a memory dump from an affected user
and most likely fixes: https://developercommunity.visualstudio.com/t/10199439

## Validation Steps Performed
* ConPTY tests complete. ✅
  • Loading branch information
@lhecker @carlos-zamora
lhecker authored and carlos-zamora committed Dec 5, 2022
1 parent 47f1167 commit a143682
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 69 deletions.
137 changes: 70 additions & 67 deletions src/host/PtySignalInputThread.cpp
View file
Expand Up @@ -72,7 +72,7 @@ void PtySignalInputThread::ConnectConsole() noexcept
}
if (_initialShowHide)
{
_DoShowHide(_initialShowHide->show);
_DoShowHide(*_initialShowHide);
}

// We should have successfully used the _earlyReparent message in CreatePseudoWindow.
Expand Down Expand Up @@ -108,86 +108,28 @@ void PtySignalInputThread::CreatePseudoWindow()
ShowHideData msg = { 0 };
_GetData(&msg, sizeof(msg));

LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

// If the client app hasn't yet connected, stash our initial
// visibility for when we do. We default to not being visible - if a
// terminal wants the ConPTY windows to start "visible", then they
// should send a ShowHidePseudoConsole(..., true) to tell us to
// initially be visible.
//
// Notably, if they don't, then a ShowWindow(SW_HIDE) on the ConPTY
// HWND will initially do _nothing_, because the OS will think that
// the window is already hidden.
if (!_consoleConnected)
{
_initialShowHide = msg;
}
else
{
_DoShowHide(msg.show);
}
_DoShowHide(msg);
break;
}
case PtySignal::ClearBuffer:
{
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)
{
_DoClearBuffer();
}
_DoClearBuffer();
break;
}
case PtySignal::ResizeWindow:
{
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)
{
_earlyResize = resizeMsg;
}
else
{
_DoResizeWindow(resizeMsg);
}

_DoResizeWindow(resizeMsg);
break;
}
case PtySignal::SetParent:
{
SetParentData reparentMessage = { 0 };
_GetData(&reparentMessage, sizeof(reparentMessage));

LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

// If the client app hasn't yet connected, stash the new owner.
// We'll later (PtySignalInputThread::ConnectConsole) use the value
// to set up the owner of the conpty window.
if (!_consoleConnected)
{
_earlyReparent = reparentMessage;
}
else
{
_DoSetWindowParent(reparentMessage);
}

_DoSetWindowParent(reparentMessage);
break;
}
default:
Expand All @@ -207,22 +149,65 @@ void PtySignalInputThread::CreatePseudoWindow()
// - <none>
void PtySignalInputThread::_DoResizeWindow(const ResizeWindowData& data)
{
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)
{
_earlyResize = data;
return;
}

if (_api.ResizeWindow(data.sx, data.sy))
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
THROW_IF_FAILED(gci.GetVtIo()->SuppressResizeRepaint());
}
}

void PtySignalInputThread::_DoClearBuffer()
void PtySignalInputThread::_DoClearBuffer() const
{
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)
{
return;
}

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
THROW_IF_FAILED(gci.GetActiveOutputBuffer().ClearBuffer());
}

void PtySignalInputThread::_DoShowHide(const bool show)
void PtySignalInputThread::_DoShowHide(const ShowHideData& data)
{
_api.ShowWindow(show);
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

// If the client app hasn't yet connected, stash our initial
// visibility for when we do. We default to not being visible - if a
// terminal wants the ConPTY windows to start "visible", then they
// should send a ShowHidePseudoConsole(..., true) to tell us to
// initially be visible.
//
// Notably, if they don't, then a ShowWindow(SW_HIDE) on the ConPTY
// HWND will initially do _nothing_, because the OS will think that
// the window is already hidden.
if (!_consoleConnected)
{
_initialShowHide = data;
return;
}

_api.ShowWindow(data.show);
}

// Method Description:
Expand All @@ -236,6 +221,18 @@ void PtySignalInputThread::_DoShowHide(const bool show)
// - <none>
void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
{
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });

// If the client app hasn't yet connected, stash the new owner.
// We'll later (PtySignalInputThread::ConnectConsole) use the value
// to set up the owner of the conpty window.
if (!_consoleConnected)
{
_earlyReparent = data;
return;
}

const auto owner{ reinterpret_cast<HWND>(data.handle) };
// This will initialize s_interactivityFactory for us. It will also
// conveniently return 0 when we're on OneCore.
Expand All @@ -245,6 +242,12 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
// window.
if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(owner) })
{
// SetWindowLongPtrW may call back into the message handler and wait for it to finish,
// similar to SendMessageW(). If the conhost message handler is already processing and
// waiting to acquire the console lock, which we're currently holding, we'd deadlock.
// --> Release the lock now.
Unlock.reset();

// DO NOT USE SetParent HERE!
//
// Calling SetParent on a window that is WS_VISIBLE will cause the OS to
Expand All @@ -257,7 +260,7 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
// SetWindowLongPtr seems to do the job of changing who the window owner
// is, without all the other side effects of reparenting the window.
// See #13066
::SetWindowLongPtr(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
::SetWindowLongPtrW(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/host/PtySignalInputThread.hpp
View file
Expand Up @@ -64,8 +64,8 @@ namespace Microsoft::Console
bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
void _DoResizeWindow(const ResizeWindowData& data);
void _DoSetWindowParent(const SetParentData& data);
void _DoClearBuffer();
void _DoShowHide(const bool show);
void _DoClearBuffer() const;
void _DoShowHide(const ShowHideData& data);
void _Shutdown();

wil::unique_hfile _hFile;
Expand Down

0 comments on commit a143682

Please sign in to comment.