Skip to content

Commit

Permalink
Fix unbalanced unlock in PtySignalInputThread::_Shutdown (#12181)
Browse files Browse the repository at this point in the history
The only way to notice that LeaveCriticalSection() was called more
often than EnterCriticalSection() is by calling EnterCriticalSection()
again in the future, which will then deadlock.
Since this bug occurs on exit it wasn't noticeable with the old console lock.
With the new, stricter ticket lock this bug causes a fail-fast exit.

## PR Checklist
* [x] Closes #12168
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Launch and then exit Windows Terminal
* OpenConsole cleanly exits ✅
  • Loading branch information
lhecker authored Jan 24, 2022
1 parent 43a275c commit 3bd3a4f
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 8 deletions.
3 changes: 1 addition & 2 deletions src/host/PtySignalInputThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
if (lastError == ERROR_BROKEN_PIPE)
{
_Shutdown();
return false;
}
else
{
Expand All @@ -180,7 +179,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
else if (dwRead != cbBuffer)
{
_Shutdown();
return false;
}

return true;
Expand Down Expand Up @@ -233,6 +231,7 @@ void PtySignalInputThread::_Shutdown()
// happens if this method is called outside of lock, but if we're
// currently locked, we want to make sure ctrl events are handled
// _before_ we RundownAndExit.
LockConsole();
ProcessCtrlEvents();

// Make sure we terminate.
Expand Down
2 changes: 2 additions & 0 deletions src/host/VtIo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "../renderer/base/renderer.hpp"
#include "../types/inc/utils.hpp"
#include "handle.h" // LockConsole
#include "input.h" // ProcessCtrlEvents
#include "output.h" // CloseConsoleProcessState

Expand Down Expand Up @@ -383,6 +384,7 @@ void VtIo::_ShutdownIfNeeded()
// happens if this method is called outside of lock, but if we're
// currently locked, we want to make sure ctrl events are handled
// _before_ we RundownAndExit.
LockConsole();
ProcessCtrlEvents();

// Make sure we terminate.
Expand Down
2 changes: 1 addition & 1 deletion src/host/output.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -520,5 +520,5 @@ void CloseConsoleProcessState()
// ctrl event will never actually get dispatched.
// So, lock and unlock here, to make sure the ctrl event gets handled.
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
UnlockConsole();
}
2 changes: 0 additions & 2 deletions src/interactivity/base/HostSignalInputThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
if (lastError == ERROR_BROKEN_PIPE)
{
_Shutdown();
return false;
}

THROW_WIN32(lastError);
Expand All @@ -172,7 +171,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
if (bytesRead != buffer.size())
{
_Shutdown();
return false;
}

return true;
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/base/ServiceLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ wil::unique_hwnd ServiceLocator::s_pseudoWindow = nullptr;

#pragma region Public Methods

void ServiceLocator::RundownAndExit(const HRESULT hr)
[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr)
{
// MSFT:15506250
// In VT I/O Mode, a client application might die before we've rendered
Expand Down Expand Up @@ -67,7 +67,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr)
s_inputServices.reset(nullptr);
}

TerminateProcess(GetCurrentProcess(), hr);
ExitProcess(hr);
}

#pragma region Creation Methods
Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/inc/ServiceLocator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace Microsoft::Console::Interactivity
class ServiceLocator final
{
public:
static void RundownAndExit(const HRESULT hr);
[[noreturn]] static void RundownAndExit(const HRESULT hr);

// N.B.: Location methods without corresponding creation methods
// automatically create the singleton object on demand.
Expand Down

0 comments on commit 3bd3a4f

Please sign in to comment.