Give the root PID ownership of the pseudoconsole window #14196
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
|
There's a TODO yet to be done: make sure that ownership changes when the root process exits. Terminal doesn't care about that right now, but other ConPTY clients don't have the same requirements as Terminal. |
DHowett
changed the title
This is a partial fix for the Get-Credential issue. While investigating it, I found that the pseudoconsole window is not marked as being "owned" (in NTUSER) by the PID/TID of the console application that is "hosted" "in" it. Doing this does not (and cannot) fix `Get-Credential` failing in DefTerm scenarios. ConsoleSetWindowOwner is one of the operations that can be done by a conhost to any window, and so the RemoteConsoleControl can call through to the Win32 ConsoleControl to pull it off. I chose to add SetWindowOwner to the IConsoleControl interface instead of moving ConsoleControl::Control into the interface to reduce the amount of churn and better separate interface responsibilities. References #14119
DHowett
force-pushed
the
dev/duhowett/looking_for_an_owner_plz_adopt_me
branch
from
e051751 to
f0f74d7
Compare
zadjii-msft
approved these changes
Oct 25, 2022
| @@ -78,7 +78,13 @@ VOID SetConsoleWindowOwner(const HWND hwnd, _Inout_opt_ ConsoleProcessHandle* pP | |||
| else | |||
| { | |||
| // Find a process to own the console window. If there are none then let's use conhost's. | |||
| pProcessData = gci.ProcessHandleList.GetFirstProcess(); | |||
| pProcessData = gci.ProcessHandleList.FindProcessInList(ConsoleProcessList::ROOT_PROCESS_ID); | |||
There was a problem hiding this comment.
This line feels like a scary change that might stealth break something we can't immagine
There was a problem hiding this comment.
So, I went through all the callers of this function. They either provide the root process, OR there is no root process... at which point, a random process is selected. The oldest one still connected.
I think it's safe to say "the root process IS the oldest, and we should default to that if we can. Otherwise, we can choose the NEXT oldest"
There was a problem hiding this comment.
Ah btw, GetFirstProcess() returns the newest process and not the oldest. See: #14421 (comment)
lhecker
approved these changes
Oct 29, 2022
Co-authored-by: Mike Griese <migrie@microsoft.com>
This comment has been minimized.
This comment has been minimized.
…for_an_owner_plz_adopt_me
This comment has been minimized.
This comment has been minimized.
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! 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
AutoMerge
ghost
merged commit
DHowett
added a commit
that referenced
this pull request
Dec 12, 2022
This is a partial fix for the Get-Credential issue. While investigating it, I found that the pseudoconsole window is not marked as being "owned" (in NTUSER) by the PID/TID of the console application that is "hosted" "in" it. Doing this does not (and cannot) fix `Get-Credential` failing in DefTerm scenarios. ConsoleSetWindowOwner is one of the operations that can be done by a conhost to any window, and so the RemoteConsoleControl can call through to the Win32 ConsoleControl to pull it off. I chose to add SetWindowOwner to the IConsoleControl interface instead of moving ConsoleControl::Control into the interface to reduce the amount of churn and better separate interface responsibilities. References #14119 (cherry picked from commit 62ffa4b) Service-Card-Id: 87207850 Service-Version: 1.15
DHowett
added a commit
that referenced
this pull request
Dec 12, 2022
This is a partial fix for the Get-Credential issue. While investigating it, I found that the pseudoconsole window is not marked as being "owned" (in NTUSER) by the PID/TID of the console application that is "hosted" "in" it. Doing this does not (and cannot) fix `Get-Credential` failing in DefTerm scenarios. ConsoleSetWindowOwner is one of the operations that can be done by a conhost to any window, and so the RemoteConsoleControl can call through to the Win32 ConsoleControl to pull it off. I chose to add SetWindowOwner to the IConsoleControl interface instead of moving ConsoleControl::Control into the interface to reduce the amount of churn and better separate interface responsibilities. References #14119 (cherry picked from commit 62ffa4b) Service-Card-Id: 87207849 Service-Version: 1.16
|
🎉 Handy links: |
|
🎉 Handy links: |
4 tasks
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 is a partial fix for the Get-Credential issue. While investigating it, I found that the pseudoconsole window is not marked as being "owned" (in NTUSER) by the PID/TID of the console application that is "hosted" "in" it. Doing this does not (and cannot) fix
Get-Credentialfailing in DefTerm scenarios.ConsoleSetWindowOwner is one of the operations that can be done by a conhost to any window, and so the RemoteConsoleControl can call through to the Win32 ConsoleControl to pull it off.
I chose to add SetWindowOwner to the IConsoleControl interface instead of moving ConsoleControl::Control into the interface to reduce the amount of churn and better separate interface responsibilities.
References #14119