Skip to content

Commit

Permalink
Fix deadlocks due to holding locks across WriteFile calls (#16224)
Browse files Browse the repository at this point in the history 
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 ✅
  • Loading branch information
@lhecker
lhecker authored Nov 8, 2023
1 parent 077d63e commit 71a1a97
Show file tree
Hide file tree
Showing 12 changed files with 270 additions and 205 deletions.
162 changes: 111 additions & 51 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
}
Expand All @@ -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 };
Expand Down Expand Up @@ -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()
Expand Down
69 changes: 47 additions & 22 deletions src/cascadia/TerminalControl/HwndTerminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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 (...)
{
Expand All @@ -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();

Expand All @@ -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();

Expand Down
9 changes: 4 additions & 5 deletions src/cascadia/TerminalCore/ITerminalInput.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
Loading

0 comments on commit 71a1a97

Please sign in to comment.