Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a deadlock during ConPTY shutdown #14160

Merged
4 commits merged into from
Oct 13, 2022
Merged

Fix a deadlock during ConPTY shutdown #14160

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83d14f0
Fix a deadlock during ConPTY shutdown
lhecker Oct 7, 2022
2782af5
Fix issues, address feedback
lhecker Oct 7, 2022
e1a8d06
Remove obsolete test
lhecker Oct 10, 2022
06a56fa
Address feedback
lhecker Oct 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions src/host/VtInputThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
_u8State{},
_dwThreadId{ 0 },
_exitRequested{ false },
_exitResult{ S_OK },
_pfnSetLookingForDSR{}
{
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
Expand Down Expand Up @@ -94,7 +93,7 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
{
const auto pInstance = reinterpret_cast<VtInputThread*>(lpParameter);
return pInstance->_InputThread();
pInstance->_InputThread();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI _InputThread is now [[noreturn]] because CloseInput is [[noreturn]]. Hence, no return is needed, despite this function "returning" a DWORD.

}

// Method Description:
Expand All @@ -118,7 +117,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
if (!fSuccess)
{
_exitRequested = true;
_exitResult = HRESULT_FROM_WIN32(GetLastError());
return;
}

Expand All @@ -127,7 +125,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
{
if (throwOnFail)
{
_exitResult = hr;
_exitRequested = true;
}
else
Expand All @@ -152,15 +149,13 @@ void VtInputThread::SetLookingForDSR(const bool looking) noexcept
// Return Value:
// - Any error from reading the pipe or writing to the input buffer that might
// have caused us to exit.
lhecker marked this conversation as resolved.
Show resolved Hide resolved
DWORD VtInputThread::_InputThread()
void VtInputThread::_InputThread()
{
while (!_exitRequested)
{
DoReadInput(true);
}
ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput();

return _exitResult;
}

// Method Description:
Expand Down
3 changes: 1 addition & 2 deletions src/host/VtInputThread.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,13 @@ namespace Microsoft::Console

private:
[[nodiscard]] HRESULT _HandleRunInput(const std::string_view u8Str);
DWORD _InputThread();
[[noreturn]] void _InputThread();

wil::unique_hfile _hFile;
wil::unique_handle _hThread;
DWORD _dwThreadId;

bool _exitRequested;
HRESULT _exitResult;

std::function<void(bool)> _pfnSetLookingForDSR;

Expand Down
12 changes: 7 additions & 5 deletions src/host/VtIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@ VtIo::VtIo() :
auto& globals = ServiceLocator::LocateGlobals();

const auto& gci = globals.getConsoleInformation();
// SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine.
assert(gci.IsConsoleLocked());

try
{
Expand Down Expand Up @@ -337,6 +339,11 @@ void VtIo::CreatePseudoWindow()

void VtIo::SetWindowVisibility(bool showOrHide) noexcept
{
auto& gci = ::Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, i am suddenly worried this is a deadlock factory. We call SetWindowVisibility off the I/O thread when Terminal says "I got focus." The PseudoWindow itself may also take the console lock to send Terminal messages. Is there a condition that will result in Terminal receiving and processing a message while the console lock is held, and then trying to dump a focus message back into the VtIo pipe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous code already acquired the console lock here. All I did was move the if condition inside the locked area. If we have such a bug then we should have had it before this PR is/was merged.


gci.LockConsole();
auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });

// ConsoleInputThreadProcWin32 calls VtIo::CreatePseudoWindow,
// which calls CreateWindowExW, which causes a WM_SIZE message.
// In short, this function might be called before _pVtRenderEngine exists.
Expand All @@ -346,11 +353,6 @@ void VtIo::SetWindowVisibility(bool showOrHide) noexcept
return;
}

auto& gci = ::Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation();

gci.LockConsole();
auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); });

LOG_IF_FAILED(_pVtRenderEngine->SetWindowVisibility(showOrHide));
}

Expand Down
4 changes: 2 additions & 2 deletions src/host/VtIo.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ namespace Microsoft::Console::VirtualTerminal

[[nodiscard]] HRESULT SwitchScreenBuffer(const bool useAltBuffer);

void CloseInput();
[[noreturn]] void CloseInput();
void CloseOutput();

void BeginResize();
Expand Down Expand Up @@ -78,7 +78,7 @@ namespace Microsoft::Console::VirtualTerminal

[[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, const std::wstring& VtMode, _In_opt_ const HANDLE SignalHandle);

void _shutdownNow();
[[noreturn]] void _shutdownNow();

#ifdef UNIT_TESTING
friend class VtIoTests;
Expand Down