Fix deadlocks due to holding locks across WriteFile calls (#16224)

This fixes a number of bugs introduced in 4370da9, all of which are of
the same kind: Holding the terminal lock across `WriteFile` calls into
the ConPTY pipe. This is problematic, because the pipe has a tiny buffer
size of just 4KiB and ConPTY may respond on its output pipe, before the
entire buffer given to `WriteFile` has been emptied. When the ConPTY
output thread then tries to acquire the terminal lock to begin parsing
the VT output, we get ourselves a proper deadlock (cross process too!).

The solution is to tease `Terminal` further apart into code that is
thread-safe and code that isn't. Functions like `SendKeyEvent` so far
have mixed them into one, because when they get called by `ControlCore`
they both, processed the data (not thread-safe as it accesses VT state)
and also sent that data back into `ControlCore` through a callback
which then indirectly called into the `ConptyConnection` which calls
`WriteFile`. Instead, we now return the data that needs to be sent from
these functions, and `ControlCore` is free to release the lock and
then call into the connection, which may then block indefinitely.

## Validation Steps Performed
* Start nvim in WSL
* Press `i` to enter the regular Insert mode
* Paste 1MB of text
* Doesn't deadlock ✅

(cherry picked from commit 71a1a97)
Service-Card-Id: 91043521
Service-Version: 1.19

Author: lhecker

src/cascadia/TerminalControl/ControlCore.cpp

@@ -10,11 +10,11 @@
 
 #include <DefaultSettings.h>
 #include <unicode.hpp>
+#include <utils.hpp>
 #include <WinUser.h>
 #include <LibraryResources.h>
 
 #include "EventArgs.h"
-#include "../../types/inc/GlyphWidth.hpp"
 #include "../../buffer/out/search.h"
 #include "../../renderer/atlas/AtlasEngine.h"
 #include "../../renderer/dx/DxRenderer.hpp"
@@ -443,6 +443,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - <none>
     void ControlCore::_sendInputToConnection(std::wstring_view wstr)
     {
+        if (wstr.empty())
+        {
+            return;
+        }
+
+        // The connection may call functions like WriteFile() which may block indefinitely.
+        // It's important we don't hold any mutexes across such calls.
+        _terminal->_assertUnlocked();
+
         if (_isReadOnly)
         {
             _raiseReadOnlyWarning();
@@ -492,8 +501,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             _handleControlC();
         }
 
-        const auto lock = _terminal->LockForWriting();
-        return _terminal->SendCharEvent(ch, scanCode, modifiers);
+        TerminalInput::OutputType out;
+        {
+            const auto lock = _terminal->LockForReading();
+            out = _terminal->SendCharEvent(ch, scanCode, modifiers);
+        }
+        if (out)
+        {
+            _sendInputToConnection(*out);
+            return true;
+        }
+        return false;
     }
 
     void ControlCore::_handleControlC()
@@ -602,46 +620,56 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                                       const ControlKeyStates modifiers,
                                       const bool keyDown)
     {
-        const auto lock = _terminal->LockForWriting();
+        if (!vkey)
+        {
+            return true;
+        }
 
-        // Update the selection, if it's present
-        // GH#8522, GH#3758 - Only modify the selection on key _down_. If we
-        // modify on key up, then there's chance that we'll immediately dismiss
-        // a selection created by an action bound to a keydown.
-        if (_shouldTryUpdateSelection(vkey) && keyDown)
+        TerminalInput::OutputType out;
         {
-            // try to update the selection
-            if (const auto updateSlnParams{ _terminal->ConvertKeyEventToUpdateSelectionParams(modifiers, vkey) })
-            {
-                _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers);
-                _updateSelectionUI();
-                return true;
-            }
+            const auto lock = _terminal->LockForWriting();
 
-            // GH#8791 - don't dismiss selection if Windows key was also pressed as a key-combination.
-            if (!modifiers.IsWinPressed())
+            // Update the selection, if it's present
+            // GH#8522, GH#3758 - Only modify the selection on key _down_. If we
+            // modify on key up, then there's chance that we'll immediately dismiss
+            // a selection created by an action bound to a keydown.
+            if (_shouldTryUpdateSelection(vkey) && keyDown)
             {
-                _terminal->ClearSelection();
-                _updateSelectionUI();
-            }
+                // try to update the selection
+                if (const auto updateSlnParams{ _terminal->ConvertKeyEventToUpdateSelectionParams(modifiers, vkey) })
+                {
+                    _terminal->UpdateSelection(updateSlnParams->first, updateSlnParams->second, modifiers);
+                    _updateSelectionUI();
+                    return true;
+                }
 
-            // When there is a selection active, escape should clear it and NOT flow through
-            // to the terminal. With any other keypress, it should clear the selection AND
-            // flow through to the terminal.
-            if (vkey == VK_ESCAPE)
-            {
-                return true;
+                // GH#8791 - don't dismiss selection if Windows key was also pressed as a key-combination.
+                if (!modifiers.IsWinPressed())
+                {
+                    _terminal->ClearSelection();
+                    _updateSelectionUI();
+                }
+
+                // When there is a selection active, escape should clear it and NOT flow through
+                // to the terminal. With any other keypress, it should clear the selection AND
+                // flow through to the terminal.
+                if (vkey == VK_ESCAPE)
+                {
+                    return true;
+                }
             }
-        }
 
-        // If the terminal translated the key, mark the event as handled.
-        // This will prevent the system from trying to get the character out
-        // of it and sending us a CharacterReceived event.
-        return vkey ? _terminal->SendKeyEvent(vkey,
-                                              scanCode,
-                                              modifiers,
-                                              keyDown) :
-                      true;
+            // If the terminal translated the key, mark the event as handled.
+            // This will prevent the system from trying to get the character out
+            // of it and sending us a CharacterReceived event.
+            out = _terminal->SendKeyEvent(vkey, scanCode, modifiers, keyDown);
+        }
+        if (out)
+        {
+            _sendInputToConnection(*out);
+            return true;
+        }
+        return false;
     }
 
     bool ControlCore::SendMouseEvent(const til::point viewportPos,
@@ -650,8 +678,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                                      const short wheelDelta,
                                      const TerminalInput::MouseButtonState state)
     {
-        const auto lock = _terminal->LockForWriting();
-        return _terminal->SendMouseEvent(viewportPos, uiButton, states, wheelDelta, state);
+        TerminalInput::OutputType out;
+        {
+            const auto lock = _terminal->LockForReading();
+            out = _terminal->SendMouseEvent(viewportPos, uiButton, states, wheelDelta, state);
+        }
+        if (out)
+        {
+            _sendInputToConnection(*out);
+            return true;
+        }
+        return false;
     }
 
     void ControlCore::UserScrollViewport(const int viewTop)
@@ -1324,8 +1361,19 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     //   before sending it over the terminal's connection.
     void ControlCore::PasteText(const winrt::hstring& hstr)
     {
+        using namespace ::Microsoft::Console::Utils;
+
+        auto filtered = FilterStringForPaste(hstr, CarriageReturnNewline | ControlCodes);
+        if (BracketedPasteEnabled())
+        {
+            filtered.insert(0, L"\x1b[200~");
+            filtered.append(L"\x1b[201~");
+        }
+
+        // It's important to not hold the terminal lock while calling this function as sending the data may take a long time.
+        _sendInputToConnection(filtered);
+
         const auto lock = _terminal->LockForWriting();
-        _terminal->WritePastedText(hstr);
         _terminal->ClearSelection();
         _updateSelectionUI();
         _terminal->TrySnapOnInput();
@@ -1894,17 +1942,27 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                     const auto endPoint = goRight ? clampedClick : cursorPos;
 
                     const auto delta = _terminal->GetTextBuffer().GetCellDistance(startPoint, endPoint);
-
                     const WORD key = goRight ? VK_RIGHT : VK_LEFT;
+
+                    std::wstring buffer;
+                    const auto append = [&](TerminalInput::OutputType&& out) {
+                        if (out)
+                        {
+                            buffer.append(std::move(*out));
+                        }
+                    };
+
                     // Send an up and a down once per cell. This won't
                     // accurately handle wide characters, or continuation
                     // prompts, or cases where a single escape character in the
                     // command (e.g. ^[) takes up two cells.
                     for (size_t i = 0u; i < delta; i++)
                     {
-                        _terminal->SendKeyEvent(key, 0, {}, true);
-                        _terminal->SendKeyEvent(key, 0, {}, false);
+                        append(_terminal->SendKeyEvent(key, 0, {}, true));
+                        append(_terminal->SendKeyEvent(key, 0, {}, false));
                     }
+
+                    _sendInputToConnection(buffer);
                 }
             }
         }
@@ -1915,7 +1973,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - Updates the renderer's representation of the selection as well as the selection marker overlay in TermControl
     void ControlCore::_updateSelectionUI()
     {
-        const auto lock = _terminal->LockForWriting();
         _renderer->TriggerSelection();
         // only show the markers if we're doing a keyboard selection or in mark mode
         const bool showMarkers{ _terminal->SelectionMode() >= ::Microsoft::Terminal::Core::Terminal::SelectionInteractionMode::Keyboard };
@@ -2247,14 +2304,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
     void ControlCore::_focusChanged(bool focused)
     {
-        // GH#13461 - temporarily turn off read-only mode, send the focus event,
-        // then turn it back on. Even in focus mode, focus events are fine to
-        // send. We don't want to pop a warning every time the control is
-        // focused.
-        const auto previous = std::exchange(_isReadOnly, false);
-        const auto restore = wil::scope_exit([&]() { _isReadOnly = previous; });
-        const auto lock = _terminal->LockForWriting();
-        _terminal->FocusChanged(focused);
+        TerminalInput::OutputType out;
+        {
+            const auto lock = _terminal->LockForReading();
+            out = _terminal->FocusChanged(focused);
+        }
+        if (out && !out->empty())
+        {
+            // _sendInputToConnection() asserts that we aren't in focus mode,
+            // but window focus events are always fine to send.
+            _connection.WriteInput(*out);
+        }
     }
 
     bool ControlCore::_isBackgroundTransparent()

src/cascadia/TerminalControl/HwndTerminal.cpp

@@ -272,7 +272,7 @@ void HwndTerminal::RegisterScrollCallback(std::function<void(int, int, int)> cal
 
 void HwndTerminal::_WriteTextToConnection(const std::wstring_view input) noexcept
 {
-    if (!_pfnWriteCallback)
+    if (input.empty() || !_pfnWriteCallback)
     {
         return;
     }
@@ -758,8 +758,17 @@ try
         WI_IsFlagSet(GetKeyState(VK_RBUTTON), KeyPressed)
     };
 
-    const auto lock = _terminal->LockForWriting();
-    return _terminal->SendMouseEvent(cursorPosition / fontSize, uMsg, getControlKeyState(), wheelDelta, state);
+    TerminalInput::OutputType out;
+    {
+        const auto lock = _terminal->LockForReading();
+        out = _terminal->SendMouseEvent(cursorPosition / fontSize, uMsg, getControlKeyState(), wheelDelta, state);
+    }
+    if (out)
+    {
+        _WriteTextToConnection(*out);
+        return true;
+    }
+    return false;
 }
 catch (...)
 {
@@ -784,8 +793,16 @@ try
     {
         _uiaProvider->RecordKeyEvent(vkey);
     }
-    const auto lock = _terminal->LockForWriting();
-    _terminal->SendKeyEvent(vkey, scanCode, modifiers, keyDown);
+
+    TerminalInput::OutputType out;
+    {
+        const auto lock = _terminal->LockForReading();
+        out = _terminal->SendKeyEvent(vkey, scanCode, modifiers, keyDown);
+    }
+    if (out)
+    {
+        _WriteTextToConnection(*out);
+    }
 }
 CATCH_LOG();
 
@@ -797,31 +814,39 @@ try
         return;
     }
 
-    const auto lock = _terminal->LockForWriting();
-
-    if (_terminal->IsSelectionActive())
+    TerminalInput::OutputType out;
     {
-        _ClearSelection();
-        if (ch == UNICODE_ESC)
+        const auto lock = _terminal->LockForWriting();
+
+        if (_terminal->IsSelectionActive())
         {
-            // ESC should clear any selection before it triggers input.
-            // Other characters pass through.
+            _ClearSelection();
+            if (ch == UNICODE_ESC)
+            {
+                // ESC should clear any selection before it triggers input.
+                // Other characters pass through.
+                return;
+            }
+        }
+
+        if (ch == UNICODE_TAB)
+        {
+            // TAB was handled as a keydown event (cf. Terminal::SendKeyEvent)
             return;
         }
-    }
 
-    if (ch == UNICODE_TAB)
-    {
-        // TAB was handled as a keydown event (cf. Terminal::SendKeyEvent)
-        return;
-    }
+        auto modifiers = getControlKeyState();
+        if (WI_IsFlagSet(flags, ENHANCED_KEY))
+        {
+            modifiers |= ControlKeyStates::EnhancedKey;
+        }
 
-    auto modifiers = getControlKeyState();
-    if (WI_IsFlagSet(flags, ENHANCED_KEY))
+        out = _terminal->SendCharEvent(ch, scanCode, modifiers);
+    }
+    if (out)
     {
-        modifiers |= ControlKeyStates::EnhancedKey;
+        _WriteTextToConnection(*out);
     }
-    _terminal->SendCharEvent(ch, scanCode, modifiers);
 }
 CATCH_LOG();
 

src/cascadia/TerminalCore/ITerminalInput.hpp

@@ -16,18 +16,17 @@ namespace Microsoft::Terminal::Core
         ITerminalInput& operator=(const ITerminalInput&) = default;
         ITerminalInput& operator=(ITerminalInput&&) = default;
 
-        virtual bool SendKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states, const bool keyDown) = 0;
-        virtual bool SendMouseEvent(const til::point viewportPos, const unsigned int uiButton, const ControlKeyStates states, const short wheelDelta, const Microsoft::Console::VirtualTerminal::TerminalInput::MouseButtonState state) = 0;
-        virtual bool SendCharEvent(const wchar_t ch, const WORD scanCode, const ControlKeyStates states) = 0;
+        virtual [[nodiscard]] ::Microsoft::Console::VirtualTerminal::TerminalInput::OutputType SendKeyEvent(const WORD vkey, const WORD scanCode, const ControlKeyStates states, const bool keyDown) = 0;
+        virtual [[nodiscard]] ::Microsoft::Console::VirtualTerminal::TerminalInput::OutputType SendMouseEvent(const til::point viewportPos, const unsigned int uiButton, const ControlKeyStates states, const short wheelDelta, const Microsoft::Console::VirtualTerminal::TerminalInput::MouseButtonState state) = 0;
+        virtual [[nodiscard]] ::Microsoft::Console::VirtualTerminal::TerminalInput::OutputType SendCharEvent(const wchar_t ch, const WORD scanCode, const ControlKeyStates states) = 0;
+        virtual [[nodiscard]] ::Microsoft::Console::VirtualTerminal::TerminalInput::OutputType FocusChanged(const bool focused) = 0;
 
         [[nodiscard]] virtual HRESULT UserResize(const til::size size) noexcept = 0;
         virtual void UserScrollViewport(const int viewTop) = 0;
         virtual int GetScrollOffset() = 0;
 
         virtual void TrySnapOnInput() = 0;
 
-        virtual void FocusChanged(const bool focused) = 0;
-
     protected:
         ITerminalInput() = default;
     };