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

Jump to bottom

Add missing lock guards on Terminal access #15894

Merged

zadjii-msft merged 1 commit into main from dev/lhecker/9617-terminal-race-conditions

Sep 19, 2023

Member

lhecker commented Aug 29, 2023 •

edited by DHowett

Loading

Terminal is used concurrently by at least 4 threads. The table
below lists the class members and the threads that access them
to the best of my knowledge. Where:

UI: UI Thread
BG: Background worker threads (winrt::resume_background)
RD: Render thread
VT: VT connection thread

	UI	BG	RD	VT
`_pfnWriteInput`	x	x		x
`_pfnWarningBell`				x
`_pfnTitleChanged`				x
`_pfnCopyToClipboard`				x
`_pfnScrollPositionChanged`	x	x		x
`_pfnCursorPositionChanged`				x
`_pfnTaskbarProgressChanged`				x
`_pfnShowWindowChanged`				x
`_pfnPlayMidiNote`				x
`_pfnCompletionsChanged`				x
`_renderSettings`	x		x	x
`_stateMachine`	x			x
`_terminalInput`	x			x
`_title`	x		x	x
`_startingTitle`	x		x
`_startingTabColor`	x
`_defaultCursorShape`	x			x
`_systemMode`		x	x	x
`_snapOnInput`	x	x
`_altGrAliasing`	x
`_suppressApplicationTitle`	x			x
`_trimBlockSelection`	x
`_autoMarkPrompts`	x
`_taskbarState`	x			x
`_taskbarProgress`	x			x
`_workingDirectory`	x			x
`_fontInfo`	x		x
`_selection`	x	x	x	x
`_blockSelection`	x	x	x
`_wordDelimiters`	x	x
`_multiClickSelectionMode`	x	x	x
`_selectionMode`	x	x	x
`_selectionIsTargetingUrl`	x	x	x
`_selectionEndpoint`	x	x	x
`_anchorInactiveSelectionEndpoint`	x	x	x
`_mainBuffer`	x	x	x	x
`_altBuffer`	x	x	x	x
`_mutableViewport`	x		x	x
`_scrollbackLines`	x
`_detectURLs`	x
`_altBufferSize`	x	x	x	x
`_deferredResize`	x			x
`_scrollOffset`	x	x	x	x
`_patternIntervalTree`	x	x	x	x
`_lastKeyEventCodes`	x
`_currentPromptState`	x			x

Only 7 members are specific to one thread and don't require locking.
All other members require some for of locking to be safe for use.

To address the issue this changeset adds LockForReading/LockForWriting
calls everywhere _terminal is accessed in ControlCore/HwndTerminal.
Additionally, to ensure these issues don't pop up anymore, it adds to
all Terminal functions a debug assertion that the lock is being held.

Finally, because this changeset started off rather modest, it contains
changes that I initially made without being aware about the extent of
the issue. It simplifies the access around _patternIntervalTree by
making _InvalidatePatternTree() directly use that member.
Furthermore, it simplifies _terminal->SetCursorOn(!IsCursorOn()) to
BlinkCursor(), allowing the code to be shared with HwndTerminal.

Ideally Terminal should not be that much of a class so that we don't
need such coarse locking. Splitting out selection and rendering state
should allow deduplicating code with conhost and use finer locking.

Closes #9617

Validation Steps Performed

I tried to use as many Windows Terminal features as I could and fixed
every occurrence of _assertLocked() failures.

lhecker added the Issue-Bug label

microsoft-github-policy-service bot added Issue-Task Area-CodeHealth Area-TerminalControl Priority-2 Product-Terminal labels

lhecker commented

View reviewed changes

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

    
            @@ -43,13 +43,13 @@ try
          
                      return DefWindowProc(hwnd, WM_NCCREATE, wParam, lParam);

                  }

              #pragma warning(suppress : 26490) // Win32 APIs can only store void*, have to use reinterpret_cast

                  auto terminal = reinterpret_cast<HwndTerminal*>(GetWindowLongPtr(hwnd, GWLP_USERDATA));

                  const auto publicTerminal = reinterpret_cast<HwndTerminal*>(GetWindowLongPtr(hwnd, GWLP_USERDATA));

Member Author

lhecker Aug 29, 2023

I renamed this variable so that it is named identical to all the other code in this file. This helped me review my changeset and to use Ctrl+F & replace.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

+                                  const auto lock = publicTerminal->_terminal->LockForWriting();
+                                  const auto bufferData = publicTerminal->_terminal->RetrieveSelectedTextFromBuffer(false);
+                                  LOG_IF_FAILED(publicTerminal->_CopyTextToSystemClipboard(bufferData, true));
+                                  publicTerminal->_ClearSelection();

Member Author

lhecker Aug 29, 2023

This adds the lock call and replaces TerminalClearSelection with _ClearSelection so that it isn't acquired again.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

                   publicTerminal->RegisterScrollCallback(callback);
               }
+              CATCH_LOG()

Member Author

lhecker Aug 29, 2023

While making the necessary modifications, I noticed that all of the C ABI functions where missing try/catch clauses.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

               {
                   const auto publicTerminal = static_cast<const HwndTerminal*>(terminal);
-                  if (!publicTerminal || !publicTerminal->_terminal || (!publicTerminal->_terminal->IsCursorBlinkingAllowed() && publicTerminal->_terminal->IsCursorVisible()))

Member Author

lhecker Aug 29, 2023

BlinkCursor() handles this now.

src/cascadia/PublicTerminalCore/HwndTerminal.cpp

-                  try
-                  {
-                      std::wstring text(pData);
-                      _WriteTextToConnection(text);

Member Author

lhecker Aug 29, 2023

_WriteTextToConnection is already noexcept. No need for the extra function.

src/cascadia/TerminalControl/ControlCore.cpp

                                   _OpenHyperlinkHandlers(*this, winrt::make<OpenHyperlinkEventArgs>(winrt::hstring{ selectedText }));
                               }
                               return true;
                           }
                           else if (vkey == VK_RETURN && !mods.IsCtrlPressed() && !mods.IsAltPressed())
                           {
                               // [Shift +] Enter --> copy text
-                              // Don't lock here! CopySelectionToClipboard already locks for you!

Member Author

lhecker Aug 29, 2023

...true, but not helpful here. UpdateSelection modifies the selection, which is concurrently being used by the renderer!

src/cascadia/TerminalControl/ControlCore.cpp

-                      // We're **NOT** taking the lock here unlike _scrollbarChangeHandler because
-                      // we are already under lock (since this usually happens as a result of writing).
-                      // TODO GH#9617: refine locking around pattern tree
-                      _terminal->ClearPatternTree();

Member Author

lhecker Aug 29, 2023

This moved into Terminal. This makes the code more robust (it's also a private function now).

src/cascadia/TerminalControl/ControlCore.cpp

Comment on lines -1646 to -1647

		if (!_terminal->IsCursorBlinkingAllowed() &&
		_terminal->IsCursorVisible())

Member Author

lhecker Aug 29, 2023

BTW this if condition is incorrect. If you flip the early return this old code says:

if (!(!_terminal->IsCursorBlinkingAllowed() && _terminal->IsCursorVisible()))
    _terminal->SetCursorOn(!_terminal->IsCursorOn());

which is:

if (_terminal->IsCursorBlinkingAllowed() || !_terminal->IsCursorVisible())
    _terminal->SetCursorOn(!_terminal->IsCursorOn());

In other words, this old code would blink the cursor even if it's invisible.

src/cascadia/TerminalCore/Terminal.cpp

+                  if (_selectionMode != SelectionInteractionMode::Mark)
+                  {
+                      auto& cursor = _activeBuffer().GetCursor();
+                      if (cursor.IsBlinkingAllowed() && cursor.IsVisible())

Member Author

lhecker Aug 29, 2023

This now matches CursorBlinker.cpp which doesn't check for cursor.IsPopupShown() either (the old code here did).

src/cascadia/TerminalCore/Terminal.cpp

-                  _patternIntervalTree = {};
-                  _InvalidatePatternTree(oldTree);
+                  _assertLocked();
+                  if (!_patternIntervalTree.empty())

Member Author

lhecker Aug 29, 2023

Checking if it's empty allows us to skip a potentially expensive operation.

lhecker force-pushed the dev/lhecker/9617-terminal-race-conditions branch 2 times, most recently from accc78a to 10ee98c Compare

August 29, 2023 15:44

zadjii-msft added the zBugBash-Consider label

zadjii-msft added a commit that referenced this pull request


          PRE-MERGE #15894 Add missing lock guards on Terminal access

4ffa871


          Add missing lock guards on Terminal access

6a1c8fe

lhecker force-pushed the dev/lhecker/9617-terminal-race-conditions branch from f7b287b to 6a1c8fe Compare

August 31, 2023 21:07

DHowett reviewed

View reviewed changes

src/cascadia/TerminalCore/TerminalSelection.cpp

@@ @@ -832,8 +839,6 @@ void Terminal::ClearSelection() @@
               // - wstring text from buffer. If extended to multiple lines, each line is separated by \r\n
               const TextBuffer::TextAndColor Terminal::RetrieveSelectedTextFromBuffer(bool singleLine)
               {
-                  auto lock = LockForReading();

Member

DHowett Aug 31, 2023

note to self: watch to see where the read lock in RetrieveSelectedText goes

Member Author

lhecker Aug 31, 2023

Into ControlCore here: https://github.com/microsoft/terminal/blob/dev/lhecker/9617-terminal-race-conditions/src/cascadia/TerminalControl/ControlCore.cpp#L1168
And one more down below. This PR moves all locking outside of Terminal.

DHowett approved these changes

View reviewed changes

Member

DHowett left a comment

Thanks so much for doing this!

zadjii-msft approved these changes

View reviewed changes

Member

zadjii-msft left a comment

Almost feels like _terminal in ControlCore should like, be behind a mutex at this point.

zadjii-msft approved these changes

View reviewed changes

Member

zadjii-msft left a comment

Almost feels like _terminal in ControlCore should like, be behind a mutex at this point.

zadjii-msft merged commit 4370da9 into main

17 checks passed

zadjii-msft deleted the dev/lhecker/9617-terminal-race-conditions branch

September 19, 2023 16:59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Area-TerminalControl Issue-Bug Issue-Task Priority-2 Product-Terminal zBugBash-Consider