Fix a potential deadlock for PtySignal::SetParent #14463
Merged
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
lhecker
added
Product-Conpty
carlos-zamora
approved these changes
Dec 1, 2022
| _DoShowHide(_initialShowHide->show); | ||
| _DoShowHide(*_initialShowHide); |
There was a problem hiding this comment.
(for my education) why is ShowHideData a struct? Why not just make _initialShowHide an optional<unsigned short> directly?
There was a problem hiding this comment.
I suppose it was done to make the code more consistent? I'd probably would've done it the same way. (And because of that I built on this to store the struct in the optional.)
There was a problem hiding this comment.
FYI We made everything structs so that they could be read directly "off the wire".
DHowett
approved these changes
Dec 1, 2022
| _DoShowHide(_initialShowHide->show); | ||
| _DoShowHide(*_initialShowHide); |
There was a problem hiding this comment.
FYI We made everything structs so that they could be read directly "off the wire".
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 (
|
ghost
merged commit
carlos-zamora
pushed a commit
that referenced
this pull request
Dec 5, 2022
This changeset consists of two parts: * Refactor `PtySignalInputThread` to move more code from `_InputThread` into the various `_Do*` handlers. This allows us to precisely control console locking behavior which is the cause of this bug. * Add the 1-line fix to `_DoSetWindowParent` to unlock the console before calling foreign functions (`SetWindowLongPtrW` in this case). This fix is theoretical in nature, based on a memory dump from an affected user and most likely fixes: https://developercommunity.visualstudio.com/t/10199439 ## Validation Steps Performed * ConPTY tests complete. ✅
lhecker
added a commit
that referenced
this pull request
Dec 5, 2022
This changeset consists of two parts: * Refactor `PtySignalInputThread` to move more code from `_InputThread` into the various `_Do*` handlers. This allows us to precisely control console locking behavior which is the cause of this bug. * Add the 1-line fix to `_DoSetWindowParent` to unlock the console before calling foreign functions (`SetWindowLongPtrW` in this case). This fix is theoretical in nature, based on a memory dump from an affected user and most likely fixes: https://developercommunity.visualstudio.com/t/10199439 ## Validation Steps Performed * ConPTY tests complete. ✅ (cherry picked from commit 3c78e01)
DHowett
pushed a commit
that referenced
this pull request
Dec 12, 2022
This changeset consists of two parts: * Refactor `PtySignalInputThread` to move more code from `_InputThread` into the various `_Do*` handlers. This allows us to precisely control console locking behavior which is the cause of this bug. * Add the 1-line fix to `_DoSetWindowParent` to unlock the console before calling foreign functions (`SetWindowLongPtrW` in this case). This fix is theoretical in nature, based on a memory dump from an affected user and most likely fixes: https://developercommunity.visualstudio.com/t/10199439 ## Validation Steps Performed * ConPTY tests complete. ✅ (cherry picked from commit 3c78e01) Service-Card-Id: 87207820 Service-Version: 1.15
DHowett
pushed a commit
that referenced
this pull request
Dec 12, 2022
This changeset consists of two parts: * Refactor `PtySignalInputThread` to move more code from `_InputThread` into the various `_Do*` handlers. This allows us to precisely control console locking behavior which is the cause of this bug. * Add the 1-line fix to `_DoSetWindowParent` to unlock the console before calling foreign functions (`SetWindowLongPtrW` in this case). This fix is theoretical in nature, based on a memory dump from an affected user and most likely fixes: https://developercommunity.visualstudio.com/t/10199439 ## Validation Steps Performed * ConPTY tests complete. ✅ (cherry picked from commit 3c78e01) Service-Card-Id: 87207821 Service-Version: 1.16
|
🎉 Handy links: |
|
🎉 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.
This changeset consists of two parts:
PtySignalInputThreadto move more code from_InputThreadinto the various
_Do*handlers. This allows us to precisely controlconsole locking behavior which is the cause of this bug.
_DoSetWindowParentto unlock the console beforecalling foreign functions (
SetWindowLongPtrWin this case).This fix is theoretical in nature, based on a memory dump from an affected user
and most likely fixes: https://developercommunity.visualstudio.com/t/10199439
Validation Steps Performed