diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 935354ebf6e..6a91620198b 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -96,17 +96,30 @@ void PtySignalInputThread::CreatePseudoWindow() // Return Value: // - S_OK if the thread runs to completion. // - Otherwise it may cause an application termination and never return. -[[nodiscard]] HRESULT PtySignalInputThread::_InputThread() +[[nodiscard]] HRESULT PtySignalInputThread::_InputThread() noexcept +try { - PtySignal signalId; - while (_GetData(&signalId, sizeof(signalId))) + const auto shutdown = wil::scope_exit([this]() { + _Shutdown(); + }); + + for (;;) { + PtySignal signalId; + if (!_GetData(&signalId, sizeof(signalId))) + { + return S_OK; + } + switch (signalId) { case PtySignal::ShowHideWindow: { ShowHideData msg = { 0 }; - _GetData(&msg, sizeof(msg)); + if (!_GetData(&msg, sizeof(msg))) + { + return S_OK; + } LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); @@ -148,7 +161,10 @@ void PtySignalInputThread::CreatePseudoWindow() case PtySignal::ResizeWindow: { ResizeWindowData resizeMsg = { 0 }; - _GetData(&resizeMsg, sizeof(resizeMsg)); + if (!_GetData(&resizeMsg, sizeof(resizeMsg))) + { + return S_OK; + } LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); @@ -171,7 +187,10 @@ void PtySignalInputThread::CreatePseudoWindow() case PtySignal::SetParent: { SetParentData reparentMessage = { 0 }; - _GetData(&reparentMessage, sizeof(reparentMessage)); + if (!_GetData(&reparentMessage, sizeof(reparentMessage))) + { + return S_OK; + } LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); @@ -191,13 +210,11 @@ void PtySignalInputThread::CreatePseudoWindow() break; } default: - { THROW_HR(E_UNEXPECTED); } - } } - return S_OK; } +CATCH_LOG_RETURN_HR(S_OK) // Method Description: // - Dispatches a resize window message to the rest of the console code @@ -269,29 +286,27 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data) // - cbBuffer - Count of bytes in the given buffer. // Return Value: // - True if data was retrieved successfully. False otherwise. -bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, - const DWORD cbBuffer) +[[nodiscard]] bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer) { + if (!_hFile) + { + return false; + } + DWORD dwRead = 0; - // If we failed to read because the terminal broke our pipe (usually due - // to dying itself), close gracefully with ERROR_BROKEN_PIPE. - // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that - // we want to gracefully close in. if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) { - auto lastError = GetLastError(); - if (lastError == ERROR_BROKEN_PIPE) + if (const auto err = GetLastError(); err != ERROR_BROKEN_PIPE) { - _Shutdown(); - } - else - { - THROW_WIN32(lastError); + LOG_WIN32(err); } + _hFile.reset(); + return false; } - else if (dwRead != cbBuffer) + + if (dwRead != cbBuffer) { - _Shutdown(); + return false; } return true; @@ -326,9 +341,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu // - Perform a shutdown of the console. This happens when the signal pipe is // broken, which means either the parent terminal process has died, or they // called ClosePseudoConsole. -// CloseConsoleProcessState is not enough by itself - it will disconnect -// clients as if the X was pressed, but then we need to actually still die, -// so then call RundownAndExit. // Arguments: // - // Return Value: @@ -337,16 +349,4 @@ void PtySignalInputThread::_Shutdown() { // Trigger process shutdown. 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); } diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index f9f4268bb9c..12089d1a5e8 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -60,8 +60,8 @@ namespace Microsoft::Console uint64_t handle; }; - [[nodiscard]] HRESULT _InputThread(); - bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); + [[nodiscard]] HRESULT _InputThread() noexcept; + [[nodiscard]] bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); void _DoResizeWindow(const ResizeWindowData& data); void _DoSetWindowParent(const SetParentData& data); void _DoClearBuffer(); diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 9b2c8b3bdfe..495a6e51d8b 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -94,6 +94,7 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) { const auto pInstance = reinterpret_cast(lpParameter); pInstance->_InputThread(); + return S_OK; } // Method Description: @@ -110,10 +111,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail) DWORD dwRead = 0; auto fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr); - // If we failed to read because the terminal broke our pipe (usually due - // to dying itself), close gracefully with ERROR_BROKEN_PIPE. - // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that - // we want to gracefully close in. if (!fSuccess) { _exitRequested = true; diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index 98741ff7e77..7c075b70025 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -30,7 +30,7 @@ namespace Microsoft::Console private: [[nodiscard]] HRESULT _HandleRunInput(const std::string_view u8Str); - [[noreturn]] void _InputThread(); + void _InputThread(); wil::unique_hfile _hFile; wil::unique_handle _hThread; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index fcb97f4aac3..a02959f444a 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -463,18 +463,6 @@ void VtIo::_shutdownNow() // 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: diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index da1099704b1..7265464fbe6 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -38,7 +38,7 @@ namespace Microsoft::Console::VirtualTerminal [[nodiscard]] HRESULT SwitchScreenBuffer(const bool useAltBuffer); - [[noreturn]] void CloseInput(); + void CloseInput(); void CloseOutput(); void BeginResize(); @@ -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); - [[noreturn]] void _shutdownNow(); + void _shutdownNow(); #ifdef UNIT_TESTING friend class VtIoTests; diff --git a/src/host/output.cpp b/src/host/output.cpp index ce31aafc36b..3a2391af886 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -499,7 +499,11 @@ void SetActiveScreenBuffer(SCREEN_INFORMATION& screenInfo) // TODO: MSFT 9450717 This should join the ProcessList class when CtrlEvents become moved into the server. https://osgvsowi/9450717 void CloseConsoleProcessState() { + LockConsole(); + const auto unlock = wil::scope_exit([] { UnlockConsole(); }); + auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + // If there are no connected processes, sending control events is pointless as there's no one do send them to. In // this case we'll just exit conhost. @@ -511,14 +515,4 @@ void CloseConsoleProcessState() } HandleCtrlEvent(CTRL_CLOSE_EVENT); - - // Jiggle the handle: (see MSFT:19419231) - // When we call this function, we'll only actually close the console once - // we're totally unlocked. If our caller has the console locked, great, - // we'll dispatch the ctrl event once they unlock. However, if they're - // not running under lock (eg PtySignalInputThread::_GetData), then the - // ctrl event will never actually get dispatched. - // So, lock and unlock here, to make sure the ctrl event gets handled. - LockConsole(); - UnlockConsole(); } diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 47b7138f454..23adfdb3c9e 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -45,13 +45,21 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept void ServiceLocator::RundownAndExit(const HRESULT hr) { - static std::atomic locked; + // The TriggerTeardown() call below depends on the render thread being able to acquire the + // console lock, so that it can safely progress with flushing the last frame. Since there's no + // coming back from this function (it's [[noreturn]]), it's safe to unlock the console here. + auto& gci = s_globals.getConsoleInformation(); + while (gci.IsConsoleLocked()) + { + gci.UnlockConsole(); + } // 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(), // so that we don't TriggerTeardown() twice. LockConsole() can't be used here, // because doing so would prevent the render thread from progressing. + static std::atomic locked; if (locked.exchange(true, std::memory_order_relaxed)) { // If we reach this point, another thread is already in the process of exiting. diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp index 4d41e6135ee..8289023f879 100644 --- a/src/winconpty/ft_pty/ConPtyTests.cpp +++ b/src/winconpty/ft_pty/ConPtyTests.cpp @@ -10,14 +10,16 @@ using namespace WEX::TestExecution; class ConPtyTests { - TEST_CLASS(ConPtyTests); + BEGIN_TEST_CLASS(ConPtyTests) + TEST_CLASS_PROPERTY(L"TestTimeout", L"0:0:15") // 15s timeout + END_TEST_CLASS() + const COORD defaultSize = { 80, 30 }; TEST_METHOD(CreateConPtyNoPipes); TEST_METHOD(CreateConPtyBadSize); TEST_METHOD(GoodCreate); TEST_METHOD(GoodCreateMultiple); TEST_METHOD(SurvivesOnBreakOutput); - TEST_METHOD(DiesOnBreakBoth); TEST_METHOD(DiesOnClose); }; @@ -218,63 +220,6 @@ void ConPtyTests::SurvivesOnBreakOutput() VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE); } -void ConPtyTests::DiesOnBreakBoth() -{ - 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); - - // Close one of the pipes... - VERIFY_IS_TRUE(CloseHandle(outPipeOurSide.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); - - // Tricky - write some input to the pcon. We need to do this so conhost can - // realize that the output pipe has broken. - VERIFY_SUCCEEDED(WriteFile(inPipeOurSide.get(), L"a", sizeof(wchar_t), nullptr, nullptr)); - - // Close the other pipe, and make sure conhost dies - VERIFY_IS_TRUE(CloseHandle(inPipeOurSide.get())); - - VERIFY_ARE_EQUAL(WaitForSingleObject(pty.hConPtyProcess, 10000), (DWORD)WAIT_OBJECT_0); - VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit)); - VERIFY_ARE_NOT_EQUAL(dwExit, (DWORD)STILL_ACTIVE); -} - void ConPtyTests::DiesOnClose() { BEGIN_TEST_METHOD_PROPERTIES() diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index 7a4f9b56395..ed740920b9e 100644 --- a/src/winconpty/winconpty.cpp +++ b/src/winconpty/winconpty.cpp @@ -230,6 +230,8 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken, pPty->hConPtyProcess = pi.hProcess; pi.hProcess = nullptr; + // The hPtyReference we create here is used when the PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE attribute is processed. + // This ensures that conhost's client processes inherit the correct (= our) console handle. RETURN_IF_NTSTATUS_FAILED(CreateClientHandle(&pPty->hPtyReference, serverHandle.get(), L"\\Reference", @@ -361,6 +363,14 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait) CloseHandle(pPty->hSignal); pPty->hSignal = nullptr; } + // The reference handle ensures that conhost keeps running unless ClosePseudoConsole is called. + // We have to call it before calling WaitForSingleObject however in order to not deadlock, + // Due to conhost waiting for all clients to disconnect, while we wait for conhost to exit. + if (_HandleIsValid(pPty->hPtyReference)) + { + CloseHandle(pPty->hPtyReference); + pPty->hPtyReference = nullptr; + } // Then, wait on the conhost process before killing it. // We do this to make sure the conhost finishes flushing any output it // has yet to send before we hard kill it. @@ -374,14 +384,6 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait) CloseHandle(pPty->hConPtyProcess); pPty->hConPtyProcess = nullptr; } - // Then take care of the reference handle. - // TODO GH#1810: Closing the reference handle late leaves conhost thinking - // that we have an outstanding connected client. - if (_HandleIsValid(pPty->hPtyReference)) - { - CloseHandle(pPty->hPtyReference); - pPty->hPtyReference = nullptr; - } } } diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h index c52c60e02a9..82355ccb071 100644 --- a/src/winconpty/winconpty.h +++ b/src/winconpty/winconpty.h @@ -7,6 +7,7 @@ extern "C" { #endif +// This structure is part of an ABI shared with the rest of the operating system. typedef struct _PseudoConsole { HANDLE hSignal;