Fix deadlocks due to holding locks across WriteFile calls #16224

lhecker · 2023-10-24T13:16:24Z

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 ✅

lhecker · 2023-10-24T13:17:18Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -10,11 +10,11 @@

 #include <DefaultSettings.h>
 #include <unicode.hpp>
+#include <utils.hpp>


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

carlos-zamora · 2023-10-24T18:28:36Z

src/cascadia/TerminalControl/ControlCore.cpp

-        const auto lock = _terminal->LockForWriting();
+        if (!vkey)
+        {
+            return true;


why not false?

The previous code did it:

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

…inal-race-conditions-fixup

lhecker · 2023-11-07T20:36:51Z

@DHowett @zadjii-msft FYI this also needs backporting to 1.19. It can cause WT to deadlock under certain circumstances.

zadjii-msft

conceptually I get it. Haven't read it all but looks fine

zadjii-msft · 2023-11-07T23:08:49Z

src/cascadia/TerminalControl/ControlCore.cpp

+            _sendInputToConnection(*out);
+            return true;
+        }
+        return false;


lhecker · 2023-11-08T09:27:47Z

src/cascadia/TerminalControl/ControlCore.cpp

+        }
+
+        // It's important to not hold the terminal lock while calling this function as sending the data may take a long time.
+        _sendInputToConnection(filtered);


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.

DHowett · 2023-11-08T17:13:14Z

src/cascadia/TerminalControl/ControlCore.cpp

-        return _terminal->SendCharEvent(ch, scanCode, modifiers);
+        TerminalInput::OutputType out;
+        {
+            const auto lock = _terminal->LockForReading();


Reading? SendCharEvent manipulates state.

Wait, SendKeyEvent manipulates state and SendCharEvent only reads it?

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?

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?

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.

src/cascadia/TerminalControl/ControlCore.cpp

…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 ✅

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

src/cascadia/TerminalControl/ControlCore.cpp

As reported here: #16224 (comment) The underlying `WriteFile` call may block indefinitely and we shouldn't hold the terminal lock during that period.

Fix deadlocks due to holding locks across WriteFile calls

baf1d73

lhecker added Area-Input Related to input processing (key presses, mouse, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Oct 24, 2023

lhecker commented Oct 24, 2023

View reviewed changes

carlos-zamora reviewed Oct 24, 2023

View reviewed changes

carlos-zamora approved these changes Oct 24, 2023

View reviewed changes

lhecker added 2 commits October 31, 2023 14:30

Merge remote-tracking branch 'origin/main' into dev/lhecker/9617-term…

cc6037d

…inal-race-conditions-fixup

Fix all the TerminalCore unit tests

454e36d

lhecker force-pushed the dev/lhecker/9617-terminal-race-conditions-fixup branch from 59cb7b6 to 454e36d Compare November 1, 2023 22:17

zadjii-msft reviewed Nov 7, 2023

View reviewed changes

src/cascadia/TerminalControl/ControlCore.cpp

_sendInputToConnection(*out);

return true;

}

return false;

Copy link

Member

zadjii-msft Nov 7, 2023

Choose a reason for hiding this comment

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

🔖

lhecker commented Nov 8, 2023

View reviewed changes

zadjii-msft approved these changes Nov 8, 2023

View reviewed changes

zadjii-msft merged commit 71a1a97 into main Nov 8, 2023
17 checks passed

zadjii-msft deleted the dev/lhecker/9617-terminal-race-conditions-fixup branch November 8, 2023 16:28

DHowett reviewed Nov 8, 2023

View reviewed changes

adonis-jimenez mentioned this pull request Nov 18, 2023

Windows Terminal Preview v1.19.3172.0 #16337

Closed

krzysdz reviewed May 9, 2024

View reviewed changes

src/cascadia/TerminalControl/ControlCore.cpp Show resolved Hide resolved

lhecker mentioned this pull request May 14, 2024

Fix lock warning during ReturnResponse #17266

Merged

github-merge-queue bot pushed a commit that referenced this pull request May 15, 2024

Fix lock warning during ReturnResponse (#17266)

183a895

As reported here: #16224 (comment) The underlying `WriteFile` call may block indefinitely and we shouldn't hold the terminal lock during that period.

lhecker mentioned this pull request Aug 23, 2024

Low reliability of VT replies #17775

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deadlocks due to holding locks across WriteFile calls #16224

Fix deadlocks due to holding locks across WriteFile calls #16224

lhecker commented Oct 24, 2023 •

edited

Loading

lhecker Oct 24, 2023 •

edited

Loading

carlos-zamora Oct 24, 2023

lhecker Oct 24, 2023

lhecker commented Nov 7, 2023

zadjii-msft left a comment

zadjii-msft Nov 7, 2023

lhecker Nov 8, 2023

DHowett Nov 8, 2023

DHowett Nov 8, 2023

lhecker Nov 8, 2023

DHowett Nov 8, 2023

lhecker Nov 8, 2023

Fix deadlocks due to holding locks across WriteFile calls #16224

Fix deadlocks due to holding locks across WriteFile calls #16224

Conversation

lhecker commented Oct 24, 2023 • edited Loading

Validation Steps Performed

lhecker Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Nov 7, 2023

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker commented Oct 24, 2023 •

edited

Loading

lhecker Oct 24, 2023 •

edited

Loading