Skip to content

Commit

Permalink
Fix a deadlock during ConPTY shutdown (#14160)
Browse files Browse the repository at this point in the history
Problem:
* Calling `RundownAndExit` tries to flush out the last frame from `VtEngine`
* `VtEngine` indirectly calls `RundownAndExit` if the pipe is gone via `VtIo`
* `RundownAndExit` is called by other parts of OpenConsole
* `RundownAndExit` must be `[[noreturn]]` for most parts of OpenConsole
* `VtIo` itself has a mutex ensuring orderly shutdown
* In short, doing a thread safe orderly shutdown requires us to hold both,
  a lock in `RundownAndExit` and `VtIo` at the same time, but while other parts
  need a blocking `RundownAndExit`, `VtIo` needs a non-blocking one
* Refactoring the code to use optionally non-blocking `RundownAndExit`
  requires refactoring and might prove to be just as buggy

Solution:
* Simply don't call `RundownAndExit` in `VtEngine` at all
* In case the write pipe breaks:
  * `VtEngine` will close the handle
  * The client should notice that their read pipe is broken and
    close their write pipe sooner or later
  * Once we notice that our read pipe is broken, we call `RundownAndExit`
  * `RundownAndExit` might call back into `VtEngine` but
    without a pipe it won't do anything
* In case the read pipe breaks or any other part calls `RundownAndExit`:
  * We call `RundownAndExit`
  * `RundownAndExit` might call back into `VtEngine` and depending on whether
    the write pipe is broken or not it will simply write into it or ignore it

Closes #14132
Pretty sure this also applies to #1810

## Validation Steps Performed
* Open 5 tabs and run MSYS2's `bash --login` in each of them
* `Enter-VsDevShell` in another tab
* Close window
* 5 tab processes are killed instantly, 1 after ~3s ✅
* Replace conhost with OpenConsole via sfpcopy
* Launch Dozens of Git Bash tabs in VS Code
* Close them randomly
* Remaining ones still work, processes are gone ✅
  • Loading branch information
lhecker authored and carlos-zamora committed Dec 5, 2022
1 parent 31bccf9 commit d2018c8
Show file tree
Hide file tree
Showing 10 changed files with 40 additions and 135 deletions.
12 changes: 2 additions & 10 deletions src/host/VtInputThread.cpp
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();
}

// 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 @@ -149,18 +146,13 @@ void VtInputThread::SetLookingForDSR(const bool looking) noexcept
// - The ThreadProc for the VT Input Thread. Reads input from the pipe, and
// passes it to _HandleRunInput to be processed by the
// InputStateMachineEngine.
// Return Value:
// - Any error from reading the pipe or writing to the input buffer that might
// have caused us to exit.
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
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
79 changes: 30 additions & 49 deletions src/host/VtIo.cpp
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,21 +339,20 @@ void VtIo::CreatePseudoWindow()

void VtIo::SetWindowVisibility(bool showOrHide) noexcept
{
// MSFT:40853556 Grab the shutdown lock here, so that another
// thread can't trigger a CloseOutput and release the
// _pVtRenderEngine out from underneath us.
std::lock_guard<std::mutex> lk(_shutdownLock);
auto& gci = ::Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation();

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.
// See PtySignalInputThread::CreatePseudoWindow().
if (!_pVtRenderEngine)
{
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 Expand Up @@ -444,56 +445,36 @@ void VtIo::SetWindowVisibility(bool showOrHide) noexcept

void VtIo::CloseInput()
{
// This will release the lock when it goes out of scope
std::lock_guard<std::mutex> lk(_shutdownLock);
_pVtInputThread = nullptr;
_ShutdownIfNeeded();
_shutdownNow();
}

void VtIo::CloseOutput()
{
// This will release the lock when it goes out of scope
std::lock_guard<std::mutex> lk(_shutdownLock);

auto& g = ServiceLocator::LocateGlobals();
// DON'T RemoveRenderEngine, as that requires the engine list lock, and this
// is usually being triggered on a paint operation, when the lock is already
// owned by the paint.
// Instead we're releasing the Engine here. A pointer to it has already been
// given to the Renderer, so we don't want the unique_ptr to delete it. The
// Renderer will own its lifetime now.
_pVtRenderEngine.release();

g.getConsoleInformation().GetActiveOutputBuffer().SetTerminalConnection(nullptr);

_ShutdownIfNeeded();
}

void VtIo::_ShutdownIfNeeded()
void VtIo::_shutdownNow()
{
// The callers should have both acquired the _shutdownLock at this point -
// we dont want a race on who is actually responsible for closing it.
if (_objectsCreated && _pVtInputThread == nullptr && _pVtRenderEngine == nullptr)
{
// At this point, we no longer have a renderer or inthread. So we've
// effectively been disconnected from the terminal.

// If we have any remaining attached processes, this will prepare us to send a ctrl+close to them
// if we don't, this will cause us to rundown and exit.
CloseConsoleProcessState();

// If we haven't terminated by now, that's because there's a client that's still attached.
// Force the handling of the control events by the attached clients.
// As of MSFT:19419231, CloseConsoleProcessState will make sure this
// 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.
ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
}
// At this point, we no longer have a renderer or inthread. So we've
// effectively been disconnected from the terminal.

// If we have any remaining attached processes, this will prepare us to send a ctrl+close to them
// if we don't, this will cause us to rundown and exit.
CloseConsoleProcessState();

// If we haven't terminated by now, that's because there's a client that's still attached.
// Force the handling of the control events by the attached clients.
// As of MSFT:19419231, CloseConsoleProcessState will make sure this
// 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.
ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
}

// Method Description:
Expand Down
5 changes: 2 additions & 3 deletions src/host/VtIo.hpp
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 @@ -67,7 +67,6 @@ namespace Microsoft::Console::VirtualTerminal
bool _objectsCreated;

bool _lookingForCursorPosition;
std::mutex _shutdownLock;

bool _resizeQuirk{ false };
bool _win32InputMode{ false };
Expand All @@ -79,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 _ShutdownIfNeeded();
[[noreturn]] void _shutdownNow();

#ifdef UNIT_TESTING
friend class VtIoTests;
Expand Down
10 changes: 0 additions & 10 deletions src/interactivity/base/ServiceLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,8 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept

void ServiceLocator::RundownAndExit(const HRESULT hr)
{
static thread_local bool preventRecursion = false;
static std::atomic<bool> locked;

// BODGY:
// pRender->TriggerTeardown() might cause another VtEngine pass, which then might fail to write to the IO pipe.
// If that happens it calls VtIo::CloseOutput(), which in turn calls ServiceLocator::RundownAndExit().
// This prevents the unintended recursion and resulting deadlock.
if (std::exchange(preventRecursion, true))
{
return;
}

// MSFT:40146639
// The premise of this function is that 1 thread enters and 0 threads leave alive.
// We need to prevent anyone from calling us until we actually ExitProcess(),
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 @@ -32,7 +32,7 @@ namespace Microsoft::Console::Interactivity
public:
static void SetOneCoreTeardownFunction(void (*pfn)()) noexcept;

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
2 changes: 1 addition & 1 deletion src/renderer/vt/paint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ using namespace Microsoft::Console::Types;
// HRESULT error code if painting didn't start successfully.
[[nodiscard]] HRESULT VtEngine::StartPaint() noexcept
{
if (_pipeBroken)
if (!_hFile)
{
return S_FALSE;
}
Expand Down
15 changes: 3 additions & 12 deletions src/renderer/vt/state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
_circled(false),
_firstPaint(true),
_skipCursor(false),
_pipeBroken(false),
_exitResult{ S_OK },
_terminalOwner{ nullptr },
_newBottomLine{ false },
Expand All @@ -58,7 +57,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
{
#ifndef UNIT_TESTING
// When unit testing, we can instantiate a VtEngine without a pipe.
THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
THROW_HR_IF(E_HANDLE, !_hFile);
#else
// member is only defined when UNIT_TESTING is.
_usingTestCallback = false;
Expand Down Expand Up @@ -136,22 +135,14 @@ CATCH_RETURN();

[[nodiscard]] HRESULT VtEngine::_Flush() noexcept
{
#ifdef UNIT_TESTING
if (_hFile.get() == INVALID_HANDLE_VALUE)
{
// Do not flush during Unit Testing because we won't have a valid file.
return S_OK;
}
#endif

if (!_pipeBroken)
if (_hFile)
{
auto fSuccess = !!WriteFile(_hFile.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), nullptr, nullptr);
_buffer.clear();
if (!fSuccess)
{
_exitResult = HRESULT_FROM_WIN32(GetLastError());
_pipeBroken = true;
_hFile.reset();
if (_terminalOwner)
{
_terminalOwner->CloseOutput();
Expand Down
1 change: 0 additions & 1 deletion src/renderer/vt/vtrenderer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ namespace Microsoft::Console::Render
bool _newBottomLine;
til::point _deferredCursorPos;

bool _pipeBroken;
HRESULT _exitResult;
Microsoft::Console::VirtualTerminal::VtIo* _terminalOwner;

Expand Down
46 changes: 0 additions & 46 deletions src/winconpty/ft_pty/ConPtyTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ class ConPtyTests
TEST_METHOD(CreateConPtyBadSize);
TEST_METHOD(GoodCreate);
TEST_METHOD(GoodCreateMultiple);
TEST_METHOD(SurvivesOnBreakInput);
TEST_METHOD(SurvivesOnBreakOutput);
TEST_METHOD(DiesOnBreakBoth);
TEST_METHOD(DiesOnClose);
Expand Down Expand Up @@ -174,51 +173,6 @@ void ConPtyTests::GoodCreateMultiple()
});
}

void ConPtyTests::SurvivesOnBreakInput()
{
PseudoConsole pty = { 0 };
wil::unique_handle outPipeOurSide;
wil::unique_handle inPipeOurSide;
wil::unique_handle outPipePseudoConsoleSide;
wil::unique_handle inPipePseudoConsoleSide;
SECURITY_ATTRIBUTES sa;
sa.nLength = sizeof(sa);
sa.bInheritHandle = TRUE;
sa.lpSecurityDescriptor = nullptr;
VERIFY_IS_TRUE(CreatePipe(inPipePseudoConsoleSide.addressof(), inPipeOurSide.addressof(), &sa, 0));
VERIFY_IS_TRUE(CreatePipe(outPipeOurSide.addressof(), outPipePseudoConsoleSide.addressof(), &sa, 0));
VERIFY_IS_TRUE(SetHandleInformation(inPipeOurSide.get(), HANDLE_FLAG_INHERIT, 0));
VERIFY_IS_TRUE(SetHandleInformation(outPipeOurSide.get(), HANDLE_FLAG_INHERIT, 0));

VERIFY_SUCCEEDED(
_CreatePseudoConsole(defaultSize,
inPipePseudoConsoleSide.get(),
outPipePseudoConsoleSide.get(),
0,
&pty));
auto closePty1 = wil::scope_exit([&] {
_ClosePseudoConsoleMembers(&pty, TRUE);
});

DWORD dwExit;
VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit));
VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);

wil::unique_process_information piClient;
std::wstring realCommand = L"cmd.exe";
VERIFY_SUCCEEDED(AttachPseudoConsole(&pty, realCommand, piClient.addressof()));

VERIFY_IS_TRUE(GetExitCodeProcess(piClient.hProcess, &dwExit));
VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);

VERIFY_IS_TRUE(CloseHandle(inPipeOurSide.get()));

// Wait for a couple seconds, make sure the child is still alive.
VERIFY_ARE_EQUAL(WaitForSingleObject(pty.hConPtyProcess, 2000), (DWORD)WAIT_TIMEOUT);
VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit));
VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
}

void ConPtyTests::SurvivesOnBreakOutput()
{
PseudoConsole pty = { 0 };
Expand Down

0 comments on commit d2018c8

Please sign in to comment.