From 41ade2c57e9e554dd2b7c63df22cec13afb6ec01 Mon Sep 17 00:00:00 2001
From: Michael Niksa <miniksa@microsoft.com>
Date: Thu, 22 Jul 2021 05:51:30 -0700
Subject: [PATCH] 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

## 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.
---
 src/host/srvinit.cpp | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp
index 5aaab774e..4d17ddfe1 100644
--- a/src/host/srvinit.cpp
+++ b/src/host/srvinit.cpp
@@ -336,8 +336,27 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
         RETURN_IF_FAILED(g.pDeviceComm->SetServerInformation(&ServerInformation));
     }
 
+    // Ensure that whatever we're giving to the new thread is on the heap so it cannot
+    // go out of scope by the time that thread starts.
+    // (e.g. if someone sent us a pointer to stack memory... that could happen
+    //  ask me how I know... :| )
+    std::unique_ptr<CONSOLE_API_MSG> heapConnectMessage;
+    if (connectMessage)
+    {
+        // Allocate and copy onto the heap
+        heapConnectMessage = std::make_unique<CONSOLE_API_MSG>(*connectMessage);
+
+        // Set the pointer that `CreateThread` uses to the heap space
+        connectMessage = heapConnectMessage.get();
+    }
+
     HANDLE const hThread = CreateThread(nullptr, 0, ConsoleIoThread, connectMessage, 0, nullptr);
     RETURN_HR_IF(E_HANDLE, hThread == nullptr);
+
+    // If we successfully started the other thread, it's that guy's problem to free the connect message.
+    // (If we didn't make one, it should be no problem to release the empty unique_ptr.)
+    heapConnectMessage.release();
+
     LOG_IF_FAILED(SetThreadDescription(hThread, L"Console Driver Message IO Thread"));
     LOG_IF_WIN32_BOOL_FALSE(CloseHandle(hThread)); // The thread will run on its own and close itself. Free the associated handle.
 
@@ -871,7 +890,11 @@ DWORD WINAPI ConsoleIoThread(LPVOID lpParameter)
     // If we were given a message on startup, process that in our context and then continue with the IO loop normally.
     if (lpParameter)
     {
-        ReceiveMsg = *(PCONSOLE_API_MSG)lpParameter;
+        // Capture the incoming lpParameter into a unique_ptr so we can appropriately
+        // 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);