Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread #10751

miniksa · 2021-07-21T20:59:11Z

Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread

PR Checklist

Closes [DefApp] PowerShell opened via Win+X errors 50% of the time #10251
I work here.
Manually verified somewhat

Detailed Description of the Pull Request / Additional comments

OpenConsole.exe is started in response to the OS conhost.exe request for a handoff and prepares an Out Of Proc Multithreaded COM server.
A COM thread from the pool inside OpenConsole.exe picks up the inbound message and allocates some stack space for the CONSOLE_API_MSG coming in
That COM thread calls down to set up the I/O thread that will pump the console driver handle and passes a pointer to the stack-allocated CONSOLE_API_MSG as the LPVOID parameter for starting the thread.

Now one of two things happen:

The I/O thread is scheduled pretty much immediately (or soon enough that the COM thread hasn't messed with the stack space), picks up the pointer to the COM thread's stack with CONSOLE_API_MSG, and processes the initial message correctly.
The COM thread continues and finalizes the handoff message to conhost.exe declaring success. It then pops stack and "frees" the memory space. If it doesn't manage to overwrite it, we're still good. If it does, then things go crazy.

This fix changes it so that the CONSOLE_API_MSG is sent into the heap before being passed to the other thread so it's in a known location that won't be freed or overwritten unexpectedly.

Validation Steps Performed

- Confirmed that many handoffs from the run box seem to work alright on my system after this change.
- Confirmed that many tab creations/splits seem to work alright on my system after this change.
- Would prefer if @ianjoneill could try to F5 this branch to build/deploy it, set it as default, and see if it makes it go away completely... but I'm pretty confident it is this based on the dumps provided either way.

…by the time it reaches the ConsoleIoThread.

…y. push things around a bit to deal with that.

DHowett · 2021-07-21T21:15:49Z

src/host/srvinit.cpp

+        // free the heap memory when we're done getting the important bits out of it below.
+        std::unique_ptr<CONSOLE_API_MSG> capturedMessage{ static_cast<PCONSOLE_API_MSG>(lpParameter) };
+
+        ReceiveMsg = *capturedMessage.get();
        ReceiveMsg._pApiRoutines = &globals.api;
        ReceiveMsg._pDeviceComm = globals.pDeviceComm;
        IoSorter::ServiceIoOperation(&ReceiveMsg, &ReplyMsg);


Meh, can we just patch capturedMessage directly and hand it off as a pointer instead of copying it?

Suggested change

IoSorter::ServiceIoOperation(&ReceiveMsg, &ReplyMsg);

IoSorter::ServiceIoOperation(capturedMessage.get(), &ReplyMsg);

Having the leftover stack state in ReceiveMsg and ReplyMsg when the dump I was given was hung later on at HRESULT hr = ServiceLocator::LocateGlobals().pDeviceComm->ReadIo(ReplyMsg, &ReceiveMsg); was what lead me to understand this bug to begin with.

As such, I tried to maintain keeping the "most recent message before hang" in the local variables. If I use the heap one directly, it'll disappear to space if I get another similar hang dump and I'll only have the reply to go on. Additionally, the api and devicecomm bits need to be patched up in ReceiveMsg anyway, no matter what, as they're reused message over message.

ianjoneill

Looks good to me - tried about 20 times and it didn't fail 🎉

ghost · 2021-07-22T12:51:03Z

Hello @zadjii-msft!

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.

@ianjoneill

…by the time it reaches the ConsoleIoThread (microsoft#10751) Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread ## PR Checklist * [x] Closes microsoft#10251 * [x] I work here. * [x] Manually verified somewhat ## Detailed Description of the Pull Request / Additional comments - `OpenConsole.exe` is started in response to the OS `conhost.exe` request for a handoff and prepares an Out Of Proc Multithreaded COM server. - A COM thread from the pool inside `OpenConsole.exe` picks up the inbound message and allocates some stack space for the `CONSOLE_API_MSG` coming in - That COM thread calls down to set up the I/O thread that will pump the console driver handle and passes a pointer to the stack-allocated `CONSOLE_API_MSG` as the `LPVOID` parameter for starting the thread. Now one of two things happen: 1. The I/O thread is scheduled pretty much immediately (or soon enough that the COM thread hasn't messed with the stack space), picks up the pointer to the COM thread's stack with `CONSOLE_API_MSG`, and processes the initial message correctly. 2. The COM thread continues and finalizes the handoff message to `conhost.exe` declaring success. It then pops stack and "frees" the memory space. If it doesn't manage to overwrite it, we're still good. If it does, then things go crazy. This fix changes it so that the `CONSOLE_API_MSG` is sent into the heap before being passed to the other thread so it's in a known location that won't be freed or overwritten unexpectedly. ## Validation Steps Performed - [x] - Confirmed that many handoffs from the run box seem to work alright on my system after this change. - [x] - Confirmed that many tab creations/splits seem to work alright on my system after this change. - [x] - Would prefer if @ianjoneill could try to F5 this branch to build/deploy it, set it as default, and see if it makes it go away completely... but I'm pretty confident it is this based on the dumps provided either way.

@ianjoneill

…by the time it reaches the ConsoleIoThread (#10751) Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread ## PR Checklist * [x] Closes #10251 * [x] I work here. * [x] Manually verified somewhat ## Detailed Description of the Pull Request / Additional comments - `OpenConsole.exe` is started in response to the OS `conhost.exe` request for a handoff and prepares an Out Of Proc Multithreaded COM server. - A COM thread from the pool inside `OpenConsole.exe` picks up the inbound message and allocates some stack space for the `CONSOLE_API_MSG` coming in - That COM thread calls down to set up the I/O thread that will pump the console driver handle and passes a pointer to the stack-allocated `CONSOLE_API_MSG` as the `LPVOID` parameter for starting the thread. Now one of two things happen: 1. The I/O thread is scheduled pretty much immediately (or soon enough that the COM thread hasn't messed with the stack space), picks up the pointer to the COM thread's stack with `CONSOLE_API_MSG`, and processes the initial message correctly. 2. The COM thread continues and finalizes the handoff message to `conhost.exe` declaring success. It then pops stack and "frees" the memory space. If it doesn't manage to overwrite it, we're still good. If it does, then things go crazy. This fix changes it so that the `CONSOLE_API_MSG` is sent into the heap before being passed to the other thread so it's in a known location that won't be freed or overwritten unexpectedly. ## Validation Steps Performed - [x] - Confirmed that many handoffs from the run box seem to work alright on my system after this change. - [x] - Confirmed that many tab creations/splits seem to work alright on my system after this change. - [x] - Would prefer if @ianjoneill could try to F5 this branch to build/deploy it, set it as default, and see if it makes it go away completely... but I'm pretty confident it is this based on the dumps provided either way.

ghost · 2021-08-31T17:03:48Z

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

Handy links:

ghost · 2021-08-31T17:09:09Z

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

Handy links:

miniksa added 2 commits July 21, 2021 10:16

Pass inbound handoff message via heap so it cannot race out of scope …

da66004

…by the time it reaches the ConsoleIoThread.

oof. you can't capture the contents of a null pointer into a heap cop…

34becf4

…y. push things around a bit to deal with that.

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 Jul 21, 2021

DHowett reviewed Jul 21, 2021

View reviewed changes

DHowett approved these changes Jul 21, 2021

View reviewed changes

ianjoneill approved these changes Jul 22, 2021

View reviewed changes

zadjii-msft approved these changes Jul 22, 2021

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 22, 2021

ghost merged commit 41ade2c into main Jul 22, 2021

ghost deleted the dev/miniksa/flaky_handoff branch July 22, 2021 12:51

This was referenced Jul 23, 2021

[DefApp] Sometimes, the first launch of the day results in 0xc0000142 #10453

Closed

Set Windows Terminal preview as default terminal not working #10229

Closed

ghost mentioned this pull request Aug 31, 2021

[DefApp] PowerShell opened via Win+X errors 50% of the time #10251

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread #10751

Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread #10751

miniksa commented Jul 21, 2021 •

edited by zadjii-msft

Loading

DHowett Jul 21, 2021

miniksa Jul 21, 2021

ianjoneill left a comment

ghost commented Jul 22, 2021

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

	IoSorter::ServiceIoOperation(&ReceiveMsg, &ReplyMsg);
	IoSorter::ServiceIoOperation(capturedMessage.get(), &ReplyMsg);

Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread #10751

Pass inbound handoff message via heap so it cannot race out of scope by the time it reaches the ConsoleIoThread #10751

Conversation

miniksa commented Jul 21, 2021 • edited by zadjii-msft Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

DHowett Jul 21, 2021

Choose a reason for hiding this comment

miniksa Jul 21, 2021

Choose a reason for hiding this comment

ianjoneill left a comment

Choose a reason for hiding this comment

ghost commented Jul 22, 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.

ghost commented Aug 31, 2021

ghost commented Aug 31, 2021

miniksa commented Jul 21, 2021 •

edited by zadjii-msft

Loading

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.