Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlocks due to holding locks across WriteFile calls #16224

Merged
merged 3 commits into from
Nov 8, 2023
Merged

Fix deadlocks due to holding locks across WriteFile calls #16224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
baf1d73
Fix deadlocks due to holding locks across WriteFile calls
lhecker Oct 24, 2023
cc6037d
Merge remote-tracking branch 'origin/main' into dev/lhecker/9617-term…
lhecker Oct 31, 2023
454e36d
Fix all the TerminalCore unit tests
lhecker Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 83 additions & 23 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
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>
Copy link
Member Author

@lhecker lhecker Oct 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was reported here: #9232 (comment)

This PR changes a lot of whitespace indentation, so use this for review: https://github.com/microsoft/terminal/pull/16224/files?w=1

#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();
lhecker marked this conversation as resolved.
Show resolved Hide resolved

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading? SendCharEvent manipulates state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, SendKeyEvent manipulates state and SendCharEvent only reads it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They both manipulate state and I should've used LockForWriting here. I think this is fine though, as this is almost assuredly not the only place where this is incorrect. I've been considering to just replace it with Lock() or to finally wrap Terminal entirely into a til::mutex<Terminal> kind of struct (but with a fair mutex).
Should I do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't necessarily do either of those for the backport version of this PR. I can see the ways in which the til::mutex-like wrapper would be great, though... what do you feel is the right answer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the wrapped mutex class has significant safety benefits, because you cannot forget to lock something. But it wouldn't have prevented the bug this PR addresses. I'm fairly confident that I've fixed all cases where we forgot to lock Terminal right now so I don't think it would have any additional benefit for a backport either.

out = _terminal->SendCharEvent(ch, scanCode, modifiers);
}
if (out)
{
_sendInputToConnection(*out);
return true;
}
return false;
}

void ControlCore::_handleControlC()
Expand Down Expand Up @@ -601,6 +619,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const WORD scanCode,
const ControlKeyStates modifiers,
const bool keyDown)
{
if (!vkey)
{
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous code did it:

        return vkey ? _terminal->SendKeyEvent(vkey,
                                              scanCode,
                                              modifiers,
                                              keyDown) :
                      true;

}

TerminalInput::OutputType out;
{
const auto lock = _terminal->LockForWriting();

Expand Down Expand Up @@ -637,11 +662,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// 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;
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔖

}

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above block is 1:1 what used to be the Terminal::WritePastedText but now it's outside the lock. It's inlined because Terminal otherwise asserts that it's locked when BracketedPasteEnabled gets called.


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())
lhecker marked this conversation as resolved.
Show resolved Hide resolved
{
// _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
37 changes: 31 additions & 6 deletions src/cascadia/TerminalControl/HwndTerminal.cpp
View file
Open in desktop
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,6 +814,8 @@ try
return;
}

TerminalInput::OutputType out;
{
const auto lock = _terminal->LockForWriting();

if (_terminal->IsSelectionActive())
Expand All @@ -821,7 +840,13 @@ try
{
modifiers |= ControlKeyStates::EnhancedKey;
}
_terminal->SendCharEvent(ch, scanCode, modifiers);

out = _terminal->SendCharEvent(ch, scanCode, modifiers);
}
if (out)
{
_WriteTextToConnection(*out);
}
}
CATCH_LOG();

Expand Down
9 changes: 4 additions & 5 deletions src/cascadia/TerminalCore/ITerminalInput.hpp
View file
Open in desktop
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
Loading