Add some read and write locks around pattern tree manipulation #9618
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DHowett
added
zStable-Service-Queued-1.12
ghost
added
Area-TerminalControl
|
Follow-up: #9617 |
zadjii-msft
approved these changes
Mar 25, 2021
| @@ -3334,8 +3344,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
|
|
|||
| _lastHoveredCell = terminalPosition; | |||
|
|
|||
| uint16_t newId{ 0u }; | |||
| // we can't use auto here because we're pre-declaring newInterval. | |||
There was a problem hiding this comment.
thank you, I would not have been able to decipher that next line without this comment
ghost
requested review from
carlos-zamora
approved these changes
Mar 25, 2021
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
DHowett
removed
the
zPreview-Service-Queued-1.13
DHowett
added a commit
that referenced
this pull request
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)
DHowett
removed
the
zStable-Service-Queued-1.12
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.