Fix a deadlock during ConPTY shutdown

src/host/VtInputThread.cpp

@@ -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);
@@ -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:
@@ -118,7 +117,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
     if (!fSuccess)
     {
         _exitRequested = true;
-        _exitResult = HRESULT_FROM_WIN32(GetLastError());
         return;
     }
 
@@ -127,7 +125,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
     {
         if (throwOnFail)
         {
-            _exitResult = hr;
             _exitRequested = true;
         }
         else
@@ -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:

src/host/VtInputThread.hpp

@@ -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;
 

src/host/VtIo.cpp

@@ -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
     {
@@ -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));
 }
 
@@ -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:

src/host/VtIo.hpp

@@ -38,7 +38,7 @@ namespace Microsoft::Console::VirtualTerminal
 
         [[nodiscard]] HRESULT SwitchScreenBuffer(const bool useAltBuffer);
 
-        void CloseInput();
+        [[noreturn]] void CloseInput();
         void CloseOutput();
 
         void BeginResize();
@@ -67,7 +67,6 @@ namespace Microsoft::Console::VirtualTerminal
         bool _objectsCreated;
 
         bool _lookingForCursorPosition;
-        std::mutex _shutdownLock;
 
         bool _resizeQuirk{ false };
         bool _win32InputMode{ false };
@@ -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;

src/interactivity/base/ServiceLocator.cpp

@@ -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(),

src/interactivity/inc/ServiceLocator.hpp

@@ -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.

src/renderer/vt/paint.cpp

@@ -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;
     }

src/renderer/vt/state.cpp

@@ -46,7 +46,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe,
     _circled(false),
     _firstPaint(true),
     _skipCursor(false),
-    _pipeBroken(false),
     _exitResult{ S_OK },
     _terminalOwner{ nullptr },
     _newBottomLine{ false },
@@ -61,7 +60,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;
@@ -139,22 +138,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();

src/renderer/vt/vtrenderer.hpp

@@ -127,7 +127,6 @@ namespace Microsoft::Console::Render
         bool _newBottomLine;
         til::point _deferredCursorPos;
 
-        bool _pipeBroken;
         HRESULT _exitResult;
         Microsoft::Console::VirtualTerminal::VtIo* _terminalOwner;
 

src/winconpty/ft_pty/ConPtyTests.cpp

@@ -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);
@@ -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 };