Skip to content

Commit

Permalink
Add mutex to keyEvents in TermControlAutomationPeer (#14694)
Browse files Browse the repository at this point in the history
Some investigation revealed that `_keyEvents` would get a `NULL_POINTER_READ` error. On the main thread, `RecordKeyEvent()` would be called, which mainly updates `_keyEvents`. On the renderer thread, `NotifyNewOutput()` would be called, which starts by iterating through `_keyEvents` and slowly clearing it out. On occasion, these two threads are modifying `_keyEvents` simultaneously, causing a crash.

The fix is to add a mutex on this variable. 

Closes #14592
  • Loading branch information
carlos-zamora committed Jan 18, 2023
1 parent 4c7879b commit 8e04169
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 20 deletions.
41 changes: 22 additions & 19 deletions src/cascadia/TerminalControl/TermControlAutomationPeer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
if (const auto keyEventChar{ gsl::narrow_cast<wchar_t>(charCode) }; IsReadable({ &keyEventChar, 1 }))
{
_keyEvents.emplace_back(keyEventChar);
_keyEvents.lock()->emplace_back(keyEventChar);
}
}
}
Expand Down Expand Up @@ -202,27 +202,30 @@ namespace winrt::Microsoft::Terminal::Control::implementation

void TermControlAutomationPeer::NotifyNewOutput(std::wstring_view newOutput)
{
// Try to suppress any events (or event data)
// that is just the keypress the user made
auto sanitized{ Sanitize(newOutput) };
while (!_keyEvents.empty() && IsReadable(sanitized))
// Try to suppress any events (or event data)
// that are just the keypresses the user made
{
if (til::toupper_ascii(sanitized.front()) == _keyEvents.front())
{
// the key event's character (i.e. the "A" key) matches
// the output character (i.e. "a" or "A" text).
// We can assume that the output character resulted from
// the pressed key, so we can ignore it.
sanitized = sanitized.substr(1);
_keyEvents.pop_front();
}
else
auto keyEvents = _keyEvents.lock();
while (!keyEvents->empty() && IsReadable(sanitized))
{
// The output doesn't match,
// so clear the input stack and
// move on to fire the event.
_keyEvents.clear();
break;
if (til::toupper_ascii(sanitized.front()) == keyEvents->front())
{
// the key event's character (i.e. the "A" key) matches
// the output character (i.e. "a" or "A" text).
// We can assume that the output character resulted from
// the pressed key, so we can ignore it.
sanitized = sanitized.substr(1);
keyEvents->pop_front();
}
else
{
// The output doesn't match,
// so clear the input stack and
// move on to fire the event.
keyEvents->clear();
break;
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalControl/TermControlAutomationPeer.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
winrt::weak_ref<Microsoft::Terminal::Control::implementation::TermControl> _termControl;
Control::InteractivityAutomationPeer _contentAutomationPeer;
std::deque<wchar_t> _keyEvents;
til::shared_mutex<std::deque<wchar_t>> _keyEvents;
};
}
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ TRACELOGGING_DECLARE_PROVIDER(g_hTerminalControlProvider);
#include <WinUser.h>

#include "til.h"
#include <til/mutex.h>

#include "ThrottledFunc.h"

Expand Down

0 comments on commit 8e04169

Please sign in to comment.