Terminal crashes deallocating til::bitmap's vector run w/ PMR allocator (??) #9410
Assignees
Labels
Area-TerminalControl
Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.)
Issue-Bug
It either shouldn't be doing this or needs an investigation.
Priority-1
A description (P1)
Product-Terminal
The new Windows Terminal.
Resolution-Fix-Committed
Fix is checked in, but it might be 3-4 weeks until a release.
Severity-Crash
Crashes are real bad news.
Comments
ghost
added
Needs-Triage
|
@DHowett I'm assigning you because I can't open feedback hub links anymore 😕 |
zadjii-msft
added
Product-Terminal
DHowett
changed the title
|
Copied from @asklar Stack trace: |
|
@miniksa may be interested in this (and there's a chance it'll hit conhost, too.) |
|
I suspect this is due to a lack of locking around clearing the pattern interval tree on scroll. In trying to reproduce it, though, I hit another issue that seems very similar. |
DHowett
added
Area-TerminalControl
|
oh god in my refactor I totally found a place where we weren't taking the lock while updating the pattern interval tree (or something like that) and fixed it, but I doubt that I did it in an atomic commit. I'd be even more surprised if the commit message was even 1% useful in identifying that code change 😕 |
|
There's a few places we're not taking the lock. Here's the patch I'm testing... From 7b1cec066d2c41c81bc4be12a81b7075381565ce Mon Sep 17 00:00:00 2001
From: Dustin Howett <[email protected]>
Date: Thu, 25 Mar 2021 11:44:29 -0500
Subject: [PATCH] LOCK IT UP
---
src/cascadia/TerminalControl/TermControl.cpp | 24 ++++++++++++++++----
src/cascadia/TerminalCore/Terminal.cpp | 3 +++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp
index 08b78ec84..6b97c1ffd 100644
--- a/src/cascadia/TerminalControl/TermControl.cpp
+++ b/src/cascadia/TerminalControl/TermControl.cpp
@@ -1815,7 +1815,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
// Clear the regex pattern tree so the renderer does not try to render them while scrolling
- _terminal->ClearPatternTree();
+ {
+ // We're taking the lock here instead of in ClearPatternTree because ClearPatternTree is
+ // sometimes called from an already-locked context. Here, we are sure we are not
+ // already under lock (since it is not an internal scroll bar update)
+ // GH#XXXX refine locking around pattern tree
+ auto lock = _terminal->LockForWriting();
+ _terminal->ClearPatternTree();
+ }
const auto newValue = static_cast<int>(args.NewValue());
@@ -2434,6 +2441,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
// Clear the regex pattern tree so the renderer does not try to render them while scrolling
+ // We're **NOT** taking the lock here unlike _ScrollbarChangeHandler because
+ // we are already under lock (since this usually happens as a result of writing).
+ // GH#XXXX refine locking around pattern tree
_terminal->ClearPatternTree();
_scrollPositionChangedHandlers(viewTop, viewHeight, bufferSize);
@@ -3334,8 +3344,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_lastHoveredCell = terminalPosition;
+ uint16_t newId{ 0u };
+ // we can't use auto here becuase we're pre-declaring newInterval.
+ decltype(_terminal->GetHyperlinkIntervalFromPosition(COORD{})) newInterval{ std::nullopt };
if (terminalPosition.has_value())
{
+ auto lock = _terminal->LockForReading(); // Lock for the duration of our reads.
+
const auto uri = _terminal->GetHyperlinkAtPosition(*terminalPosition);
if (!uri.empty())
{
@@ -3361,15 +3376,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
OverlayCanvas().SetLeft(HyperlinkTooltipBorder(), (locationInDIPs.x() - SwapChainPanel().ActualOffset().x));
OverlayCanvas().SetTop(HyperlinkTooltipBorder(), (locationInDIPs.y() - SwapChainPanel().ActualOffset().y));
}
- }
- const uint16_t newId = terminalPosition.has_value() ? _terminal->GetHyperlinkIdAtPosition(*terminalPosition) : 0u;
- const auto newInterval = terminalPosition.has_value() ? _terminal->GetHyperlinkIntervalFromPosition(*terminalPosition) : std::nullopt;
+ newId = _terminal->GetHyperlinkIdAtPosition(*terminalPosition);
+ newInterval = _terminal->GetHyperlinkIntervalFromPosition(*terminalPosition);
+ }
// If the hyperlink ID changed or the interval changed, trigger a redraw all
// (so this will happen both when we move onto a link and when we move off a link)
if (newId != _lastHoveredId || (newInterval != _lastHoveredInterval))
{
+ auto lock = _terminal->LockForWriting();
_lastHoveredId = newId;
_lastHoveredInterval = newInterval;
_renderEngine->UpdateHyperlinkHoveredId(newId);
diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp
index 3d0a0b810..4b5999778 100644
--- a/src/cascadia/TerminalCore/Terminal.cpp
+++ b/src/cascadia/TerminalCore/Terminal.cpp
@@ -453,6 +453,7 @@ bool Terminal::IsTrackingMouseInput() const noexcept
// - The position
std::wstring Terminal::GetHyperlinkAtPosition(const COORD position)
{
+ auto lock = LockForReading();
auto attr = _buffer->GetCellDataAt(_ConvertToBufferCell(position))->TextAttr();
if (attr.IsHyperlink())
{
@@ -486,6 +487,7 @@ std::wstring Terminal::GetHyperlinkAtPosition(const COORD position)
// - The hyperlink ID
uint16_t Terminal::GetHyperlinkIdAtPosition(const COORD position)
{
+ auto lock = LockForReading();
return _buffer->GetCellDataAt(_ConvertToBufferCell(position))->TextAttr().GetHyperlinkId();
}
@@ -497,6 +499,7 @@ uint16_t Terminal::GetHyperlinkIdAtPosition(const COORD position)
// - The interval representing the start and end coordinates
std::optional<PointTree::interval> Terminal::GetHyperlinkIntervalFromPosition(const COORD position)
{
+ auto lock = LockForReading();
const auto results = _patternIntervalTree.findOverlapping(COORD{ position.X + 1, position.Y }, position);
if (results.size() > 0)
{
--
2.29.0.vfs.0.0 |
|
The read locks in the inner leaves proved to be bad because shared_mutex isn't intended to be taken recursively, simply shared. |
zadjii-msft
removed
the
Needs-Triage
ghost
pushed a commit
that referenced
this issue
Mar 26, 2021
We have been seeing some crashes (#9410) originating from a use-after-free or a double-free in the renderer. The renderer is iterating over the dirty rects from the render engine¹ and the rect list is being freed out from under it. Things like this are usually the result of somebody manipulating the renderer's state outside of lock. Therefore, this pull request introduces some targeted locking fixes around manipulation of the pattern buffer (which, in turn, changes the renderer state.) ¹ This was not a problem until #8621, which made the renderer return a span instead of a copy for the list of dirty rects. ## Validation I ran Terminal under App Verifier, and introduced a manul delay (under lock) in the renderer such that the invalid map would definitely have been invalidated between the renderer taking the lock and the renderer handling the frame. AppVerif failed us without these locking changes, and did not do so once they were introduced. Closes #9410.
ghost
added
Resolution-Fix-Committed
DHowett
added a commit
that referenced
this issue
Apr 2, 2021
We have been seeing some crashes (#9410) originating from a use-after-free or a double-free in the renderer. The renderer is iterating over the dirty rects from the render engine¹ and the rect list is being freed out from under it. Things like this are usually the result of somebody manipulating the renderer's state outside of lock. Therefore, this pull request introduces some targeted locking fixes around manipulation of the pattern buffer (which, in turn, changes the renderer state.) ¹ This was not a problem until #8621, which made the renderer return a span instead of a copy for the list of dirty rects. ## Validation I ran Terminal under App Verifier, and introduced a manul delay (under lock) in the renderer such that the invalid map would definitely have been invalidated between the renderer taking the lock and the renderer handling the frame. AppVerif failed us without these locking changes, and did not do so once they were introduced. Closes #9410. (cherry picked from commit ea3e56d)
|
🎉This issue was addressed in #9618, which has now been successfully released as Handy links: |
|
🎉This issue was addressed in #9618, which has now been successfully released as Handy links: |
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Environment
Steps to reproduce
Unable to reproduce reliably but suspect it's related to split panes across numerous tabs. I can't remember the last time I saw Windows Terminal crash, but I usually don't use split planes much. This time I had numerous tabs (common) and a vertical split in most of those tabs. Probably not material but all terminals were PowerShell 7 and most had open WinRM sessions.
I was able to retrieve a crash dump which I've uploaded via Feedback Hub here. A quick analysis in WinDbg points to heap corruption. The lack of symbols though makes interpreting the stack trace of the faulting thread a bit more involved.
Expected behavior
Not to crash :-)
Actual behavior
Crash of Windows Terminal and the loss of all open terminals.
Other notes
It'd be nice if symbols could be published at least for official releases. This could be as simple as uploading them in a separate Zip attached to the GitHub releases page, instead of publishing them on the official Microsoft symbol server. In turn, this would allow people like me to perform more analysis of issues themselves, hopefully leading to more detailed bug reports :-)
The text was updated successfully, but these errors were encountered: