Persist inbox conhost; delegate control activities to it via a pipe #10415

miniksa · 2021-06-11T22:43:16Z

Persist inbox conhost; delegate control activities to it via a pipe

PR Checklist

Closes Windows terminal as default terminal application breaks WSL2 launch when WSL2 debugConsole is enabled #10194 - WSL Debug Tap doesn't work
Closes [DefApp] launching WSL inside DefTerm yields “the parameter is incorrect” #10134 - WSL Parameter is Incorrect
Closes [DefApp] Ctrl+C doesn't work to cancel ping from cmd #10413 - Ctrl+C not passed to client
Closes [DefApp] Assorted leftover processes when abruptly terminating portions of app chain #10414 - Leftover processes on abrupt termination
Might help [DefApp] PowerShell opened via Win+X errors 50% of the time #10251 - Win+X Powershell sometimes fails to attach
I work here
Manually tested with assorted launch scenarios

Detailed Description of the Pull Request / Additional comments

It turns out that there's a bit of ownership that goes on with the original inbox conhost.exe and the operating system/driver. The PID of that original conhost.exe is stowed when the initial connection is established and it is verified for several activities. This means that the plan of letting it go completely away and having the OpenConsole.exe take over all of its activities must be slightly revised.

I have tested the following two alternatives to keeping conhost.exe around and they do not work:

Replacing the original owner conhost.exe with OpenConsole.exe - A.) The driver does not allow this. Once the owner is registered, it cannot be replaced. B.) There's no way of updating this information inside the client process space and it is kept there too in the kernelbase/conclnt data from its initial connection.
Attempting to pick up the first packet (to determine headed/headless and other initial connection information that we use to determine whether handoff is appropriate or not) prior to registering any owner at all. - The driver doesn't allow this either. The owner must be registered prior to a packet coming through.

Put this mental model in your head:
CMD --> Conhost (inbox) --> OpenConsole (WT Package) --> Terminal (WT Package)

So since the conhost.exe needs to stick around, here's what I'm doing in this PR:

conhost.exe in the OS will receive back the OpenConsole.exe process handle on a successful handoff and is expected to remain alive until the OpenConsole.exe exits. It's now waiting on that before it terminates itself.
conhost.exe in the OS will establish a signal channel pipe and listen for control commands from OpenConsole.exe in a very similar fashion to how the ConPTY signal pipe operates between the Terminal and the PTY (provided by OpenConsole.exe in this particular example.) When OpenConsole.exe needs to do something that would be verified by the OS and rejected... it will instead signal the original conhost.exe to do that thing and it will go through.
conhost.exe will give its own handle through to OpenConsole.exe so it can monitor its lifetime and cleanup. If the owner is gone, the session should end.
Assorted handle cleanup that was leading to improper exits. I was confused between .reset() and .release() for some of the wil::unique_any<T> handling and it lead to leaked handles. The leaked handles meant that threads weren't aware of the other sides collapsing and wouldn't cleanup/terminate appropriately.

How does this fix things?

For the WSL cases... WSL was specifically looking up the owner PID of the console session from the driver. That was the conhost.exe PID. If it exits, that PID isn't valid and is recycled. Thus the parameter is incorrect or other inappropriate WSL setup behaviors.
Ctrl+C not passed... this is a signal the operating system rejects from a PID that is not the owner. This is now relayed through the original owner and it works.
Leftover processes... I believe I explained this was both not-enough-monitoring of each others' process lifetimes coupled with mishandling of release/resetting handles and leaking them.
Powershell sometimes fails to attach... my theory on this one is that it's a race that became upset when the conhost.exe disappeared while something about Powershell/.NET was still starting, much like the WSL one. I believe now that it is sticking around, it will be fine.

Also, this WILL require an OS update to complete improvement of functionality and I have revised the interface ID. This is considered an acceptable breaking change with no mitigation because we said this feature was an alpha preview.

Validation Steps Performed

Launched WSL with defapp set, it works
Launched WSL with defapp set and the debug tap on, it works and opens in two tabs
Launched CMD, ran ping, did Ctrl+C, it now receives it
Launched Win+X powershell a ton of times. It seems fine now
Launched cmd, powershell, wsl, etc. Killed assorted processes in the chain (client/conhost/openconsole/windowsterminal) and observed in Process Explorer (with a long delta timer so I could see it) that they all successfully tear down now without leftovers.

…opriate watching between the processes in the chain so they clean up correctly when any one of them exits (appropriately or otherwise.)

…ching to interactivity anyway. that helps it link better with existing tests.

miniksa · 2021-06-11T22:56:59Z

I just realized I likely need to send a few more parameters over the IConsoleHandoff interface while I'm here for #9458. So setting to Draft until I figure that out so I don't have to rev the IID again.

miniksa · 2021-06-11T23:13:49Z

I just realized I likely need to send a few more parameters over the IConsoleHandoff interface while I'm here for #9458. So setting to Draft until I figure that out so I don't have to rev the IID again.

ConsoleInitializeConnectInfo looks like it can pick up any of this information in the OpenConsole.exe so it doesn't need to be passed. So nevermind on the #9458 business. That's between OpenConsole.exe and WindowsTerminal.exe... not the OS copy.

src/host/srvinit.cpp

o-sdn-o · 2021-06-14T16:27:35Z

Could this be related to #10400?

DHowett · 2021-06-14T16:28:48Z

Could this be related to #10400?

Unlikely; this code is only used when conhost (inside windows) spawns a secondary conhost, not when you call CreatePseudoConsole.

src/interactivity/base/ServiceLocator.cpp

DHowett · 2021-06-14T17:20:35Z

src/host/srvinit.cpp

@@ -364,6 +365,8 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
 //   from the driver... or an S_OK success.
 [[nodiscard]] HRESULT ConsoleEstablishHandoff([[maybe_unused]] _In_ HANDLE Server,
                                              [[maybe_unused]] HANDLE driverInputEvent,
+                                              [[maybe_unused]] HANDLE hostSignalPipe,
+                                              [[maybe_unused]] HANDLE hostProcessHandle,
                                              [[maybe_unused]] PCONSOLE_API_MSG connectMessage)
 try
 {


you'll probably merge conflict near here because of til::feature

src/inc/HostSignals.hpp

src/interactivity/base/HostSignalInputThread.cpp

src/interactivity/base/RemoteConsoleControl.cpp

… grammar issues in comments... oh my.

src/interactivity/base/RemoteConsoleControl.cpp

lhecker

Lotta comments... But no actual mistakes.

src/host/exe/CConsoleHandoff.cpp

src/host/srvinit.cpp

lhecker · 2021-06-15T19:05:22Z

src/interactivity/base/HostSignalInputThread.cpp

+// - Creates the PTY Signal Input Thread.
+// Arguments:
+// - hPipe - a handle to the file representing the read end of the Host Signal pipe.
+HostSignalInputThread::HostSignalInputThread(_In_ wil::unique_hfile&& hPipe) :


We don't need _In_ in modern C++ anymore, do we?
Also I think I need to change my opinion: I previously agreed with Dustin that && makes the intent of ownership transfer clear, but now that I read the code I feel like a version without any references at all is cleaner. It properly works even with prvalues, like when you want to write new HostSignalInputThread(wil::make_unique<>(...)) (if that existed), etc.
I'm not sure if it can be argued in this particular case that the && variant helps confer any additional meaning to future readers, what a handle called "unique" doesn't already.

This is between you and @DHowett then. && already confuses me and you two understand it better than me. I just want whatever works.

(I did drop SAL tho...)

src/interactivity/base/HostSignalInputThread.hpp

src/interactivity/base/HostSignalInputThread.cpp

src/interactivity/base/RemoteConsoleControl.cpp

src/interactivity/base/ServiceLocator.cpp

src/server/IoDispatchers.cpp

miniksa · 2021-06-15T23:49:39Z

Don't merge yet. I need to do a final e2e test after all the style feedback to make sure I didn't bungle a byte buffer.

…ld won't build this.

miniksa · 2021-06-16T18:58:07Z

Built against OS and fixed. Patched OS and tried it and confirmed the issues are still resolved. Setting automerge.

ghost · 2021-06-16T18:58:16Z

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2021-06-16T19:30:31Z

src/interactivity/base/HostSignalInputThread.cpp

    THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
+    THROW_HR_IF(E_HANDLE, _hFile.get() == nullptr);


huh, i thought that these were the same

moreover, however: !_hFile is the valid way to check for presence in a wil handle

DHowett · 2021-06-16T19:31:55Z

src/interactivity/base/RemoteConsoleControl.cpp

    {
-        return NTSTATUS_FROM_WIN32(::GetLastError());
+        NT_RETURN_NTSTATUS(static_cast<NTSTATUS>(NTSTATUS_FROM_WIN32(::GetLastError())));


i... thought we moved to wil because there was a macro that made this less ugly, not a macro that made it more ugly 😉

For the glory of error tracing! 🥲
It's really weird that the static_cast is necessary...

the only plus side to this mess is that it is more debuggable with the wil tracelog. You're right... wil was supposed to solve this but then on further inspection... someone implemented like 4 of the functions and not the ones I needed (NT_RETURN_NTSTATUS_IF and NT_RETURN_NTSTATUS_IF_WIN32_BOOL_FALSE etc.)

ghost · 2021-07-14T17:08:53Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

miniksa added 7 commits June 10, 2021 13:51

Make the inbox console wait/stay open forever.

853786a

Make OS wait on open one indefinitely.

f673bd2

finish interface re-id

aab183b

Well... this madness builds.

fd1333d

use correct macros, add a few comments

08ad342

Properly reset some of the handles we have handed off. Establish appr…

f99ff51

…opriate watching between the processes in the chain so they clean up correctly when any one of them exits (appropriately or otherwise.)

move host signal input thread to interactivity base since it's dispat…

14fd5f9

…ching to interactivity anyway. that helps it link better with existing tests.

ghost added Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Jun 11, 2021

miniksa added 3 commits June 11, 2021 15:45

spellcheck

a8bfc9c

codeformat!

a6ef19a

restore otherwise unchanged file

579be98

miniksa marked this pull request as draft June 11, 2021 22:56

stale header reference

d811b33

miniksa marked this pull request as ready for review June 11, 2021 23:13

lhecker reviewed Jun 11, 2021

View reviewed changes

src/host/srvinit.cpp Show resolved Hide resolved

DHowett requested changes Jun 14, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 14, 2021

miniksa added 4 commits June 14, 2021 11:34

Enums and classes and templates and namespaces and move semantics and…

4640690

… grammar issues in comments... oh my.

Code format

c65380f

Merge branch 'main' into dev/miniksa/def_wsl

b5552b1

improve naming, actually save the file with the third definition.

712f9e5

DHowett reviewed Jun 14, 2021

View reviewed changes

src/interactivity/base/RemoteConsoleControl.cpp Show resolved Hide resolved

pack the structure to ensure there aren't 0s between the code and packet

d46a67a

DHowett approved these changes Jun 15, 2021

View reviewed changes

lhecker approved these changes Jun 15, 2021

View reviewed changes

miniksa added 3 commits June 15, 2021 16:48

Assorted feedback for modernizing thread and style

ed5cb2c

code format

b54983d

Merge branch 'main' into dev/miniksa/def_wsl

48657f6

For reasons I cannot discern, I must use gsl::byte here or the OS bui…

c276eda

…ld won't build this.

miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 16, 2021

ghost merged commit 2bd5791 into main Jun 16, 2021

ghost deleted the dev/miniksa/def_wsl branch June 16, 2021 19:23

DHowett reviewed Jun 16, 2021

View reviewed changes

onomatopellan mentioned this pull request Jun 22, 2021

The parameter is incorrect. occurs when default termial application is set to windows terminal preview microsoft/WSL#7088

Closed

2 tasks

This was referenced Jul 23, 2021

Set Windows Terminal preview as default terminal not working #10229

Closed

Cannot set SetConsoleCtrlHandler #10647

Open

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist inbox conhost; delegate control activities to it via a pipe #10415

Persist inbox conhost; delegate control activities to it via a pipe #10415

miniksa commented Jun 11, 2021

miniksa commented Jun 11, 2021

miniksa commented Jun 11, 2021

o-sdn-o commented Jun 14, 2021

DHowett commented Jun 14, 2021

DHowett Jun 14, 2021

lhecker left a comment

lhecker Jun 15, 2021

miniksa Jun 15, 2021

miniksa Jun 15, 2021

miniksa commented Jun 15, 2021

miniksa commented Jun 16, 2021

ghost commented Jun 16, 2021

DHowett Jun 16, 2021

DHowett Jun 16, 2021

lhecker Jun 16, 2021

miniksa Jun 16, 2021

ghost commented Jul 14, 2021

		THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
		THROW_HR_IF(E_HANDLE, _hFile.get() == nullptr);

Persist inbox conhost; delegate control activities to it via a pipe #10415

Persist inbox conhost; delegate control activities to it via a pipe #10415

Conversation

miniksa commented Jun 11, 2021

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

miniksa commented Jun 11, 2021

miniksa commented Jun 11, 2021

o-sdn-o commented Jun 14, 2021

DHowett commented Jun 14, 2021

Choose a reason for hiding this comment

lhecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa commented Jun 15, 2021

miniksa commented Jun 16, 2021

ghost commented Jun 16, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 14, 2021

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 (`@msftbot`) and give me an instruction to get started! Learn more here.