ConPTY: Fix shutdown if killed during startup (#18588)

During startup we relinquish ownership of the console lock to wait for
the DA1 response of the hosting terminal. The problem occurs if the
hosting terminal disconnects during that time. The broken pipe will
cause `VtIo` to send out `CTRL_CLOSE_EVENT` messages, but those won't
achieve anything, because the first and only client hasn't even finished
connecting yet. What we need to do instead is to return an error code.

In order to not use a bunch of booleans to control this behavior, I gave
`VtIo` a state enum. This however required restructuring the calling
code in order to not have a dozen states.

## Validation Steps Performed
* Launch cmd.exe with ConPTY
* ...but leave the stdin pipe unbound (which will hang the DA1 request)
* Immediately kill the ConPTY session
* cmd.exe exits after clicking away the error message ✅

(cherry picked from commit 733a5e7becd86400323c0536a86d88021b3748fb)
Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXjzIg
Service-Version: 1.23

src/host/VtIo.cpp

@@ -79,7 +79,11 @@ using namespace Microsoft::Console::Interactivity;
                                         const HANDLE OutHandle,
                                         _In_opt_ const HANDLE SignalHandle)
 {
-    FAIL_FAST_IF_MSG(_initialized, "Someone attempted to double-_Initialize VtIo");
+    if (_state != State::Uninitialized)
+    {
+        assert(false); // Don't call initialize twice.
+        return E_UNEXPECTED;
+    }
 
     _hInput.reset(InHandle);
     _hOutput.reset(OutHandle);
@@ -95,47 +99,33 @@ using namespace Microsoft::Console::Interactivity;
         }
     }
 
+    // - Create and start the signal thread. The signal thread can be created
+    //      independent of the i/o threads, and doesn't require a client first
+    //      attaching to the console. We need to create it first and foremost,
+    //      because it's possible that a terminal application could
+    //      CreatePseudoConsole, then ClosePseudoConsole without ever attaching a
+    //      client. Should that happen, we still need to exit.
+    if (IsValidHandle(_hSignal.get()))
+    {
+        try
+        {
+            _pPtySignalInputThread = std::make_unique<PtySignalInputThread>(std::move(_hSignal));
+
+            // Start it if it was successfully created.
+            RETURN_IF_FAILED(_pPtySignalInputThread->Start());
+        }
+        CATCH_RETURN();
+    }
+
     // The only way we're initialized is if the args said we're in conpty mode.
     // If the args say so, then at least one of in, out, or signal was specified
-    _initialized = true;
+    _state = State::Initialized;
-    return S_OK;
-}
-
-// Method Description:
-// - Create the VtEngine and the VtInputThread for this console.
-// MUST BE DONE AFTER CONSOLE IS INITIALIZED, to make sure we've gotten the
-//  buffer size from the attached client application.
-// Arguments:
-// - <none>
-// Return Value:
-//  S_OK if we initialized successfully,
-//  S_FALSE if VtIo hasn't been initialized (or we're not in conpty mode)
-//  otherwise an appropriate HRESULT indicating failure.
-[[nodiscard]] HRESULT VtIo::CreateIoHandlers() noexcept
-{
-    if (!_initialized)
-    {
-        return S_FALSE;
-    }
-
-    // SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine.
-    assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
-
-    try
-    {
-        if (IsValidHandle(_hInput.get()))
-        {
-            _pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput), _lookingForCursorPosition);
-        }
-    }
-    CATCH_RETURN();
-
     return S_OK;
 }
 
 bool VtIo::IsUsingVt() const
 {
-    return _initialized;
+    return _state != State::Uninitialized;
 }
 
 // Routine Description:
@@ -151,50 +141,64 @@ bool VtIo::IsUsingVt() const
 [[nodiscard]] HRESULT VtIo::StartIfNeeded()
 {
     // If we haven't been set up, do nothing (because there's nothing to start)
-    if (!_initialized)
+    if (_state != State::Initialized)
     {
         return S_FALSE;
     }
 
+    _state = State::Starting;
+
+    // SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine.
+    assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
+
+    try
+    {
+        if (IsValidHandle(_hInput.get()))
+        {
+            _pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput), _lookingForCursorPosition);
+        }
+    }
+    CATCH_RETURN();
+
     if (_pVtInputThread)
     {
         LOG_IF_FAILED(_pVtInputThread->Start());
-    }
 
-    {
-        Writer writer{ this };
-
-        // MSFT: 15813316
-        // If the terminal application wants us to inherit the cursor position,
-        // we're going to emit a VT sequence to ask for the cursor position.
-        // If we get a response, the InteractDispatch will call SetCursorPosition,
-        // which will call to our VtIo::SetCursorPosition method.
-        //
-        // By sending the request before sending the DA1 one, we can simply
-        // wait for the DA1 response below and effectively wait for both.
-        if (_lookingForCursorPosition)
         {
-            writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
+            Writer writer{ this };
+
+            // MSFT: 15813316
+            // If the terminal application wants us to inherit the cursor position,
+            // we're going to emit a VT sequence to ask for the cursor position.
+            // If we get a response, the InteractDispatch will call SetCursorPosition,
+            // which will call to our VtIo::SetCursorPosition method.
+            //
+            // By sending the request before sending the DA1 one, we can simply
+            // wait for the DA1 response below and effectively wait for both.
+            if (_lookingForCursorPosition)
+            {
+                writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
+            }
+
+            // GH#4999 - Send a sequence to the connected terminal to request
+            // win32-input-mode from them. This will enable the connected terminal to
+            // send us full INPUT_RECORDs as input. If the terminal doesn't understand
+            // this sequence, it'll just ignore it.
+            writer.WriteUTF8(
+                "\x1b[c" // DA1 Report (Primary Device Attributes)
+                "\x1b[?1004h" // Focus Event Mode
+                "\x1b[?9001h" // Win32 Input Mode
+            );
+
+            writer.Submit();
         }
 
-        // GH#4999 - Send a sequence to the connected terminal to request
+        {
-        // win32-input-mode from them. This will enable the connected terminal to
+            // Allow the input thread to momentarily gain the console lock.
-        // send us full INPUT_RECORDs as input. If the terminal doesn't understand
+            auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-        // this sequence, it'll just ignore it.
+            const auto suspension = gci.SuspendLock();
-        writer.WriteUTF8(
+            _deviceAttributes = _pVtInputThread->WaitUntilDA1(3000);
-            "\x1b[c" // DA1 Report (Primary Device Attributes)
+        }
-            "\x1b[?1004h" // Focus Event Mode
-            "\x1b[?9001h" // Win32 Input Mode
-        );
-
-        writer.Submit();
-    }
-
-    {
-        // Allow the input thread to momentarily gain the console lock.
-        auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-        const auto suspension = gci.SuspendLock();
-        _deviceAttributes = _pVtInputThread->WaitUntilDA1(3000);
     }
 
     if (_pPtySignalInputThread)
@@ -208,6 +212,17 @@ bool VtIo::IsUsingVt() const
         _pPtySignalInputThread->ConnectConsole();
     }
 
+    if (_state != State::Starting)
+    {
+        // Here's where we _could_ call CloseConsoleProcessState(), but this function
+        // only gets called once when the first client connects and CONSOLE_INITIALIZED
+        // is not set yet. The process list may already contain that first client,
+        // but since it hasn't finished connecting yet, it won't react to a CTRL_CLOSE_EVENT.
+        // Instead, we return an error here which will abort the connection setup.
+        return E_FAIL;
+    }
+
+    _state = State::Running;
     return S_OK;
 }
 
@@ -244,47 +259,21 @@ void VtIo::CreatePseudoWindow()
     }
 }
 
-// Method Description:
-// - Create and start the signal thread. The signal thread can be created
-//      independent of the i/o threads, and doesn't require a client first
-//      attaching to the console. We need to create it first and foremost,
-//      because it's possible that a terminal application could
-//      CreatePseudoConsole, then ClosePseudoConsole without ever attaching a
-//      client. Should that happen, we still need to exit.
-// Arguments:
-// - <none>
-// Return Value:
-// - S_FALSE if we're not in VtIo mode,
-//   S_OK if we succeeded,
-//   otherwise an appropriate HRESULT indicating failure.
-[[nodiscard]] HRESULT VtIo::CreateAndStartSignalThread() noexcept
-{
-    if (!_initialized)
-    {
-        return S_FALSE;
-    }
-
-    // If we were passed a signal handle, try to open it and make a signal reading thread.
-    if (IsValidHandle(_hSignal.get()))
-    {
-        try
-        {
-            _pPtySignalInputThread = std::make_unique<PtySignalInputThread>(std::move(_hSignal));
-
-            // Start it if it was successfully created.
-            RETURN_IF_FAILED(_pPtySignalInputThread->Start());
-        }
-        CATCH_RETURN();
-    }
-
-    return S_OK;
-}
-
 void VtIo::SendCloseEvent()
 {
     LockConsole();
     const auto unlock = wil::scope_exit([] { UnlockConsole(); });
 
+    // If we're still in the process of starting up, and we're asked to shut down
+    // (broken pipe), `VtIo::StartIfNeeded()` will handle the cleanup for us.
+    // This can happen during the call to `WaitUntilDA1`, because we relinquish
+    // ownership of the console lock.
+    if (_state == State::Starting)
+    {
+        _state = State::StartupFailed;
+        return;
+    }
+
     // This function is called when the ConPTY signal pipe is closed (PtySignalInputThread) and when the input
     // pipe is closed (VtIo). Usually these two happen at about the same time. This if condition is a bit of
     // a premature optimization and prevents us from sending out a CTRL_CLOSE_EVENT right after another.

src/host/VtIo.hpp

@@ -57,8 +57,6 @@ namespace Microsoft::Console::VirtualTerminal
         static wchar_t SanitizeUCS2(wchar_t ch);
 
         [[nodiscard]] HRESULT Initialize(const ConsoleArguments* const pArgs);
-        [[nodiscard]] HRESULT CreateAndStartSignalThread() noexcept;
-        [[nodiscard]] HRESULT CreateIoHandlers() noexcept;
 
         bool IsUsingVt() const;
         [[nodiscard]] HRESULT StartIfNeeded();
@@ -69,6 +67,15 @@ namespace Microsoft::Console::VirtualTerminal
         void CreatePseudoWindow();
 
     private:
+        enum class State : uint8_t
+        {
+            Uninitialized,
+            Initialized,
+            Starting,
+            StartupFailed,
+            Running,
+        };
+
         [[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, _In_opt_ const HANDLE SignalHandle);
 
         void _uncork();
@@ -77,7 +84,7 @@ namespace Microsoft::Console::VirtualTerminal
         // After CreateIoHandlers is called, these will be invalid.
         wil::unique_hfile _hInput;
         wil::unique_hfile _hOutput;
-        // After CreateAndStartSignalThread is called, this will be invalid.
+        // After Initialize is called, this will be invalid.
         wil::unique_hfile _hSignal;
 
         std::unique_ptr<Microsoft::Console::VtInputThread> _pVtInputThread;
@@ -96,7 +103,7 @@ namespace Microsoft::Console::VirtualTerminal
         bool _writerRestoreCursor = false;
         bool _writerTainted = false;
 
-        bool _initialized = false;
+        State _state = State::Uninitialized;
         bool _lookingForCursorPosition = false;
         bool _closeEventSent = false;
         int _corked = 0;

src/host/srvinit.cpp

@@ -381,7 +381,6 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
     //      can start, so they're started below, in ConsoleAllocateConsole
     auto& gci = g.getConsoleInformation();
     RETURN_IF_FAILED(gci.GetVtIo()->Initialize(args));
-    RETURN_IF_FAILED(gci.GetVtIo()->CreateAndStartSignalThread());
 
     return S_OK;
 }
@@ -945,27 +944,11 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand,
     // We'll need the size of the screen buffer in the vt i/o initialization
     if (SUCCEEDED_NTSTATUS(Status))
     {
-        auto hr = gci.GetVtIo()->CreateIoHandlers();
+        // Actually start the VT I/O threads
-        if (hr == S_FALSE)
+        auto hr = gci.GetVtIo()->StartIfNeeded();
-        {
+        // Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS
-            // We're not in VT I/O mode, this is fine.
+        //      is treated as an error
-        }
+        if (FAILED(hr))
-        else if (SUCCEEDED(hr))
-        {
-            // Actually start the VT I/O threads
-            hr = gci.GetVtIo()->StartIfNeeded();
-            // Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS
-            //      is treated as an error
-            if (hr != S_FALSE)
-            {
-                Status = NTSTATUS_FROM_HRESULT(hr);
-            }
-            else
-            {
-                Status = ERROR_SUCCESS;
-            }
-        }
-        else
         {
             Status = NTSTATUS_FROM_HRESULT(hr);
         }