From 4de1ef73e9a8e0fb7582d60c6e1152fb9aeb1a77 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 7 Jun 2022 14:47:55 -0500 Subject: [PATCH 1/8] This makes sure we re-own the window appropriately, but defterm doesn't request focus mode. Weird. --- src/cascadia/TerminalApp/TerminalPage.cpp | 23 ++++++++++++++++++++ src/cascadia/TerminalControl/ControlCore.cpp | 21 ++++++++++++++++-- src/cascadia/TerminalControl/ControlCore.h | 8 +++---- 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 46889b1db99..ea1bb14a8fb 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -69,6 +69,29 @@ namespace winrt::TerminalApp::implementation // - see GH#2988 HRESULT TerminalPage::Initialize(HWND hwnd) { + if (!_hostingHwnd.has_value()) + { + // GH#13211 - if we haven't yet set the owning hwnd, reparent all the controls now. + for (const auto& tab : _tabs) + { + if (auto terminalTab{ _GetTerminalTabImpl(tab) }) + { + terminalTab->GetRootPane()->WalkTree([&](auto&& pane) { + if (const auto& term{ pane->GetTerminalControl() }) + { + term.OwningHwnd(reinterpret_cast(hwnd)); + } + }); + } + // We don't need to worry about resetting the owning hwnd for the + // SUI here. GH#13211 only repros for a defterm connection, where + // the tab is spawned before the window is created. It's not + // possible to make a SUI tab like that, before the window is + // created. The SUI could be spawned as a part of a window restore, + // but that would still work fine. The window would be created + // before restoring previous tabs in that scenario. + } + } _hostingHwnd = hwnd; return S_OK; } diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index 9d98115c912..ef0033250ff 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -267,11 +267,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation const auto height = vp.Height(); _connection.Resize(height, width); - if (_OwningHwnd != 0) + if (_owningHwnd != 0) { if (auto conpty{ _connection.try_as() }) { - conpty.ReparentWindow(_OwningHwnd); + conpty.ReparentWindow(_owningHwnd); } } @@ -1854,4 +1854,21 @@ namespace winrt::Microsoft::Terminal::Control::implementation // transparency, or our acrylic, or our image. return Opacity() < 1.0f || UseAcrylic() || !_settings->BackgroundImage().empty() || _settings->UseBackgroundImageForWindow(); } + + uint64_t ControlCore::OwningHwnd() + { + return _owningHwnd; + } + + void ControlCore::OwningHwnd(uint64_t owner) + { + if (owner != _owningHwnd && _connection) + { + if (auto conpty{ _connection.try_as() }) + { + conpty.ReparentWindow(owner); + } + } + _owningHwnd = owner; + } } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index b3c8afa8a98..5d270ee4f7b 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -179,10 +179,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation void WindowVisibilityChanged(const bool showOrHide); - // TODO:GH#1256 - When a tab can be torn out or otherwise reparented to - // another window, this value will need a custom setter, so that we can - // also update the connection. - WINRT_PROPERTY(uint64_t, OwningHwnd, 0); + uint64_t OwningHwnd(); + void OwningHwnd(uint64_t owner); RUNTIME_SETTING(double, Opacity, _settings->Opacity()); RUNTIME_SETTING(bool, UseAcrylic, _settings->UseAcrylic()); @@ -252,6 +250,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation double _panelHeight{ 0 }; double _compositionScale{ 0 }; + uint64_t _owningHwnd{ 0 }; + winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr }; std::shared_ptr> _tsfTryRedrawCanvas; std::shared_ptr> _updatePatternLocations; From 5ab079915b27438850d516e521960d71504b78d2 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 7 Jun 2022 15:09:40 -0500 Subject: [PATCH 2/8] make sure to enable win32-input-mode, to make sure we request focus events, but that doesn't fix it either --- src/host/ConsoleArguments.cpp | 6 ++++++ src/host/srvinit.cpp | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/host/ConsoleArguments.cpp b/src/host/ConsoleArguments.cpp index f89bf249732..2541cc8ab82 100644 --- a/src/host/ConsoleArguments.cpp +++ b/src/host/ConsoleArguments.cpp @@ -25,6 +25,12 @@ const std::wstring_view ConsoleArguments::FEATURE_ARG = L"--feature"; const std::wstring_view ConsoleArguments::FEATURE_PTY_ARG = L"pty"; const std::wstring_view ConsoleArguments::COM_SERVER_ARG = L"-Embedding"; const std::wstring_view ConsoleArguments::PASSTHROUGH_ARG = L"--passthrough"; +// NOTE: Thinking about adding more commandline args that control conpty, for +// the Terminal? Make sure you add them to the commandline in +// ConsoleEstablishHandoff. We use that to initialize the ConsoleArguments for a +// defterm handoff s.t. they behave like a conpty connection that was started by +// the terminal. If you forget them there, the conpty won't obey them, only for +// defterm. std::wstring EscapeArgument(std::wstring_view ac) { diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index c7a84116a5e..a7ce2e1e7c2 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -536,7 +536,12 @@ try outPipeTheirSide.reset(); signalPipeTheirSide.reset(); - const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --signal {:#x}"), (int64_t)signalPipeOurSide.release()); + // GH#13211 - Make sure we request win32input mode and that the terminal + // obeys the resizing quirk. Otherwise, defterm connections to the Terminal + // are going to have weird resizing, and aren't going to send full fidelity + // input messages. + const auto commandLine = fmt::format(FMT_COMPILE(L" --headless --resizeQuirk --win32input --signal {:#x}"), + (int64_t)signalPipeOurSide.release()); ConsoleArguments consoleArgs(commandLine, inPipeOurSide.release(), outPipeOurSide.release()); RETURN_IF_FAILED(consoleArgs.ParseCommandline()); From da55ab7c4045fab366f751be80c391c7dd3e5b9b Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Jun 2022 06:30:46 -0500 Subject: [PATCH 3/8] This was totally wrong --- .../base/RemoteConsoleControl.cpp | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 21b8155a79f..fcb644b058a 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -7,6 +7,8 @@ #include "../../inc/HostSignals.hpp" +#include "../../interactivity/inc/ServiceLocator.hpp" + using namespace Microsoft::Console::Interactivity; RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : @@ -57,12 +59,50 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) { - HostSignalSetForegroundData data{}; - data.sizeInBytes = sizeof(data); - data.processId = HandleToULong(hProcess); - data.isForeground = fForeground; + // return LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hProcess, fForeground)); + + enum ControlType + { + ConsoleSetVDMCursorBounds, + ConsoleNotifyConsoleApplication, + ConsoleFullscreenSwitch, + ConsoleSetCaretInfo, + ConsoleSetReserveKeys, + ConsoleSetForeground, + ConsoleSetWindowOwner, + ConsoleEndTask, + }; + CONSOLESETFOREGROUND Flags; + Flags.hProcess = hProcess; + Flags.bForeground = fForeground; + auto control = [](_In_ ControlType ConsoleCommand, + _In_reads_bytes_(ConsoleInformationLength) PVOID ConsoleInformation, + _In_ DWORD ConsoleInformationLength) -> NTSTATUS { + { + wil::unique_hmodule _hUser32{ LoadLibraryW(L"user32.dll") }; + if (_hUser32) + { + typedef NTSTATUS(WINAPI * PfnConsoleControl)(ControlType Command, PVOID Information, DWORD Length); + + static auto pfn = (PfnConsoleControl)GetProcAddress(_hUser32.get(), "ConsoleControl"); + + if (pfn != nullptr) + { + return pfn(ConsoleCommand, ConsoleInformation, ConsoleInformationLength); + } + } + + return STATUS_UNSUCCESSFUL; + } + }; + return LOG_IF_NTSTATUS_FAILED(control(ControlType::ConsoleSetForeground, hProcess, fForeground)); + + // HostSignalSetForegroundData data{}; + // data.sizeInBytes = sizeof(data); + // data.processId = HandleToULong(hProcess); + // data.isForeground = fForeground; - return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); + // return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); } [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) From f117fb85038d66ce4f16532c7935050de8524fad Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Jun 2022 06:31:32 -0500 Subject: [PATCH 4/8] Revert "This was totally wrong" This reverts commit da55ab7c4045fab366f751be80c391c7dd3e5b9b. --- .../base/RemoteConsoleControl.cpp | 50 ++----------------- 1 file changed, 5 insertions(+), 45 deletions(-) diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index fcb644b058a..21b8155a79f 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -7,8 +7,6 @@ #include "../../inc/HostSignals.hpp" -#include "../../interactivity/inc/ServiceLocator.hpp" - using namespace Microsoft::Console::Interactivity; RemoteConsoleControl::RemoteConsoleControl(HANDLE signalPipe) : @@ -59,50 +57,12 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) { - // return LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hProcess, fForeground)); - - enum ControlType - { - ConsoleSetVDMCursorBounds, - ConsoleNotifyConsoleApplication, - ConsoleFullscreenSwitch, - ConsoleSetCaretInfo, - ConsoleSetReserveKeys, - ConsoleSetForeground, - ConsoleSetWindowOwner, - ConsoleEndTask, - }; - CONSOLESETFOREGROUND Flags; - Flags.hProcess = hProcess; - Flags.bForeground = fForeground; - auto control = [](_In_ ControlType ConsoleCommand, - _In_reads_bytes_(ConsoleInformationLength) PVOID ConsoleInformation, - _In_ DWORD ConsoleInformationLength) -> NTSTATUS { - { - wil::unique_hmodule _hUser32{ LoadLibraryW(L"user32.dll") }; - if (_hUser32) - { - typedef NTSTATUS(WINAPI * PfnConsoleControl)(ControlType Command, PVOID Information, DWORD Length); - - static auto pfn = (PfnConsoleControl)GetProcAddress(_hUser32.get(), "ConsoleControl"); - - if (pfn != nullptr) - { - return pfn(ConsoleCommand, ConsoleInformation, ConsoleInformationLength); - } - } - - return STATUS_UNSUCCESSFUL; - } - }; - return LOG_IF_NTSTATUS_FAILED(control(ControlType::ConsoleSetForeground, hProcess, fForeground)); - - // HostSignalSetForegroundData data{}; - // data.sizeInBytes = sizeof(data); - // data.processId = HandleToULong(hProcess); - // data.isForeground = fForeground; + HostSignalSetForegroundData data{}; + data.sizeInBytes = sizeof(data); + data.processId = HandleToULong(hProcess); + data.isForeground = fForeground; - // return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); + return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); } [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) From e970f7c93c4005d8a138d4f2e6acab965fe43831 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Jun 2022 09:46:20 -0500 Subject: [PATCH 5/8] this worked --- src/inc/HostSignals.hpp | 4 +-- .../base/HostSignalInputThread.cpp | 4 +++ src/server/ProcessHandle.cpp | 29 +++++++++++++++++++ src/server/ProcessHandle.h | 3 +- src/server/ProcessList.cpp | 4 +-- 5 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index 05e1d0f3f72..17362bc4038 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -19,7 +19,7 @@ namespace Microsoft::Console struct HostSignalSetForegroundData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS NEEDS TO BE A PID, NOT A HANDLE bool isForeground; }; @@ -30,4 +30,4 @@ namespace Microsoft::Console uint32_t eventType; uint32_t ctrlFlags; }; -}; \ No newline at end of file +}; diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 397131b08e4..1734cb91e65 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -96,6 +96,10 @@ T HostSignalInputThread::_ReceiveTypedPacket() { auto msg = _ReceiveTypedPacket(); + // if (wil::unique_handle hClient{ OpenProcess(PROCESS_QUERY_INFORMATION, false, msg.processId) }) + // { + // LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hClient.get(), msg.isForeground)); + // } LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); break; diff --git a/src/server/ProcessHandle.cpp b/src/server/ProcessHandle.cpp index 7c2adfc6417..0efaadf9338 100644 --- a/src/server/ProcessHandle.cpp +++ b/src/server/ProcessHandle.cpp @@ -8,6 +8,9 @@ #include "../host/globals.h" #include "../host/telemetry.hpp" +#include "../interactivity/inc/ServiceLocator.hpp" + +using namespace Microsoft::Console::Interactivity; // Routine Description: // - Constructs an instance of the ConsoleProcessHandle Class // - NOTE: Can throw if allocation fails or if there is a console policy we do not understand. @@ -34,6 +37,32 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId, { Telemetry::Instance().LogProcessConnected(_hProcess.get()); } + + if (const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }) + { + LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), + _hProcess.get(), + conhost.get(), + _hProcessInConhost.put(), + 0 /*dwDesiredAccess, ignored*/, + false, + DUPLICATE_SAME_ACCESS)); + } +} + +ConsoleProcessHandle::~ConsoleProcessHandle() +{ + const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }; + if (_hProcessInConhost && conhost) + { + LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(conhost.get(), + _hProcessInConhost.get(), + nullptr /* hTargetProcessHandle */, + nullptr /* lpTargetHandle, ignored */, + 0 /*dwDesiredAccess, ignored*/, + false, + DUPLICATE_CLOSE_SOURCE)); + } } // Routine Description: diff --git a/src/server/ProcessHandle.h b/src/server/ProcessHandle.h index 50084f45430..5772fd12650 100644 --- a/src/server/ProcessHandle.h +++ b/src/server/ProcessHandle.h @@ -50,7 +50,7 @@ class ConsoleProcessHandle ConsoleProcessHandle(const DWORD dwProcessId, const DWORD dwThreadId, const ULONG ulProcessGroupId); - ~ConsoleProcessHandle() = default; + ~ConsoleProcessHandle(); ConsoleProcessHandle(const ConsoleProcessHandle&) = delete; ConsoleProcessHandle(ConsoleProcessHandle&&) = delete; ConsoleProcessHandle& operator=(const ConsoleProcessHandle&) & = delete; @@ -59,6 +59,7 @@ class ConsoleProcessHandle ULONG _ulTerminateCount; ULONG const _ulProcessGroupId; wil::unique_handle const _hProcess; + wil::unique_handle _hProcessInConhost; mutable ULONG64 _processCreationTime; diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index f852db7728e..15a7090642d 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -305,9 +305,9 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { const auto pProcessHandle = *it; - if (pProcessHandle->_hProcess != nullptr) + if (const auto& handleToUse = pProcessHandle->_hProcessInConhost ? pProcessHandle->_hProcessInConhost : pProcessHandle->_hProcess) { - _ModifyProcessForegroundRights(pProcessHandle->_hProcess.get(), fForeground); + _ModifyProcessForegroundRights(handleToUse.get(), fForeground); } it = std::next(it); From 7d82c81de53e92a87bb89fc05388224af1c2682f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Jun 2022 10:31:07 -0500 Subject: [PATCH 6/8] lemme tell you, this is NOT a HANDLE --- src/host/input.cpp | 2 +- src/inc/HostSignals.hpp | 6 +++--- src/interactivity/base/HostSignalInputThread.cpp | 9 ++++----- src/interactivity/base/RemoteConsoleControl.cpp | 4 ++-- src/interactivity/base/RemoteConsoleControl.hpp | 2 +- src/interactivity/inc/IConsoleControl.hpp | 2 +- src/interactivity/onecore/ConsoleControl.cpp | 4 ++-- src/interactivity/onecore/ConsoleControl.hpp | 2 +- src/interactivity/win32/ConsoleControl.cpp | 4 ++-- src/interactivity/win32/ConsoleControl.hpp | 2 +- src/server/ProcessHandle.cpp | 14 +++++++++++++- src/server/ProcessList.cpp | 6 +++++- 12 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/host/input.cpp b/src/host/input.cpp index a689a9454c7..e49dee1a8be 100644 --- a/src/host/input.cpp +++ b/src/host/input.cpp @@ -326,7 +326,7 @@ void ProcessCtrlEvents() if (NT_SUCCESS(Status)) { Status = ServiceLocator::LocateConsoleControl() - ->EndTask((HANDLE)rgProcessHandleList[i].dwProcessID, + ->EndTask(rgProcessHandleList[i].dwProcessID, EventType, CtrlFlags); if (rgProcessHandleList[i].hProcess == nullptr) diff --git a/src/inc/HostSignals.hpp b/src/inc/HostSignals.hpp index 17362bc4038..ec29e540646 100644 --- a/src/inc/HostSignals.hpp +++ b/src/inc/HostSignals.hpp @@ -13,20 +13,20 @@ namespace Microsoft::Console struct HostSignalNotifyAppData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS IS A PID }; struct HostSignalSetForegroundData { uint32_t sizeInBytes; - uint32_t processId; // THIS NEEDS TO BE A PID, NOT A HANDLE + uint32_t processId; // THIS IS A HANDLE, NOT A PID bool isForeground; }; struct HostSignalEndTaskData { uint32_t sizeInBytes; - uint32_t processId; + uint32_t processId; // THIS IS A PID uint32_t eventType; uint32_t ctrlFlags; }; diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 1734cb91e65..82f982595bb 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -96,10 +96,9 @@ T HostSignalInputThread::_ReceiveTypedPacket() { auto msg = _ReceiveTypedPacket(); - // if (wil::unique_handle hClient{ OpenProcess(PROCESS_QUERY_INFORMATION, false, msg.processId) }) - // { - // LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hClient.get(), msg.isForeground)); - // } + // GH#13211 - The OpenConsole side should have already duplicated + // the handle to us, and processId should be the value in our + // process space. LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); break; @@ -108,7 +107,7 @@ T HostSignalInputThread::_ReceiveTypedPacket() { auto msg = _ReceiveTypedPacket(); - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(ULongToHandle(msg.processId), msg.eventType, msg.ctrlFlags)); + LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->EndTask(msg.processId, msg.eventType, msg.ctrlFlags)); break; } diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 21b8155a79f..0b9283ba8bb 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -65,11 +65,11 @@ template return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); } -[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { HostSignalEndTaskData data{}; data.sizeInBytes = sizeof(data); - data.processId = HandleToULong(hProcessId); + data.processId = dwProcessId; data.eventType = dwEventType; data.ctrlFlags = ulCtrlFlags; diff --git a/src/interactivity/base/RemoteConsoleControl.hpp b/src/interactivity/base/RemoteConsoleControl.hpp index 52de8db1eb3..ee22aeef4dc 100644 --- a/src/interactivity/base/RemoteConsoleControl.hpp +++ b/src/interactivity/base/RemoteConsoleControl.hpp @@ -25,7 +25,7 @@ namespace Microsoft::Console::Interactivity // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId); [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground); - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); private: wil::unique_handle _pipe; diff --git a/src/interactivity/inc/IConsoleControl.hpp b/src/interactivity/inc/IConsoleControl.hpp index 02d83e1b97e..7129cff68ff 100644 --- a/src/interactivity/inc/IConsoleControl.hpp +++ b/src/interactivity/inc/IConsoleControl.hpp @@ -23,6 +23,6 @@ namespace Microsoft::Console::Interactivity virtual ~IConsoleControl() = default; [[nodiscard]] virtual NTSTATUS NotifyConsoleApplication(DWORD dwProcessId) = 0; [[nodiscard]] virtual NTSTATUS SetForeground(HANDLE hProcess, BOOL fForeground) = 0; - [[nodiscard]] virtual NTSTATUS EndTask(HANDLE hProcessId, DWORD dwEventType, ULONG ulCtrlFlags) = 0; + [[nodiscard]] virtual NTSTATUS EndTask(DWORD dwProcessId, DWORD dwEventType, ULONG ulCtrlFlags) = 0; }; } diff --git a/src/interactivity/onecore/ConsoleControl.cpp b/src/interactivity/onecore/ConsoleControl.cpp index 662a2b6b1e9..248d02b3ac8 100644 --- a/src/interactivity/onecore/ConsoleControl.cpp +++ b/src/interactivity/onecore/ConsoleControl.cpp @@ -22,13 +22,13 @@ using namespace Microsoft::Console::Interactivity::OneCore; return STATUS_SUCCESS; } -[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { USER_API_MSG m{}; const auto a = &m.u.EndTask; RtlZeroMemory(a, sizeof(*a)); - a->ProcessId = hProcessId; + a->ProcessId = ULongToHandle(dwProcessId); // This is actually a PID, even though the struct expects a HANDLE. a->ConsoleEventCode = dwEventType; a->ConsoleFlags = ulCtrlFlags; diff --git a/src/interactivity/onecore/ConsoleControl.hpp b/src/interactivity/onecore/ConsoleControl.hpp index 9b406a475f4..4771c6c2220 100644 --- a/src/interactivity/onecore/ConsoleControl.hpp +++ b/src/interactivity/onecore/ConsoleControl.hpp @@ -26,6 +26,6 @@ namespace Microsoft::Console::Interactivity::OneCore // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId) noexcept override; [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) noexcept override; - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) override; + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) override; }; } diff --git a/src/interactivity/win32/ConsoleControl.cpp b/src/interactivity/win32/ConsoleControl.cpp index 4cea4e3857e..78e66110df2 100644 --- a/src/interactivity/win32/ConsoleControl.cpp +++ b/src/interactivity/win32/ConsoleControl.cpp @@ -37,12 +37,12 @@ using namespace Microsoft::Console::Interactivity::Win32; sizeof(Flags)); } -[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) +[[nodiscard]] NTSTATUS ConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) { auto pConsoleWindow = ServiceLocator::LocateConsoleWindow(); CONSOLEENDTASK ConsoleEndTaskParams; - ConsoleEndTaskParams.ProcessId = hProcessId; + ConsoleEndTaskParams.ProcessId = ULongToHandle(dwProcessId); // This is actually a PID, even though the struct expects a HANDLE. ConsoleEndTaskParams.ConsoleEventCode = dwEventType; ConsoleEndTaskParams.ConsoleFlags = ulCtrlFlags; ConsoleEndTaskParams.hwnd = pConsoleWindow == nullptr ? nullptr : pConsoleWindow->GetWindowHandle(); diff --git a/src/interactivity/win32/ConsoleControl.hpp b/src/interactivity/win32/ConsoleControl.hpp index 9a8ab927fc0..fdd490a528a 100644 --- a/src/interactivity/win32/ConsoleControl.hpp +++ b/src/interactivity/win32/ConsoleControl.hpp @@ -47,7 +47,7 @@ namespace Microsoft::Console::Interactivity::Win32 // IConsoleControl Members [[nodiscard]] NTSTATUS NotifyConsoleApplication(_In_ DWORD dwProcessId); [[nodiscard]] NTSTATUS SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground); - [[nodiscard]] NTSTATUS EndTask(_In_ HANDLE hProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); + [[nodiscard]] NTSTATUS EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags); // Public Members [[nodiscard]] NTSTATUS Control(_In_ ConsoleControl::ControlType ConsoleCommand, diff --git a/src/server/ProcessHandle.cpp b/src/server/ProcessHandle.cpp index 0efaadf9338..09229f9409e 100644 --- a/src/server/ProcessHandle.cpp +++ b/src/server/ProcessHandle.cpp @@ -38,6 +38,17 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId, Telemetry::Instance().LogProcessConnected(_hProcess.get()); } + // GH#13211 - If we're running as the delegation console (someone handed off + // to us), then we need to make sure the original conhost has access to this + // process handle as well. Otherwise, the future calls to + // ConsoleControl(SetForeground,...) won't work, because the literal handle + // value doesn't exist in the original conhost process space. + // * g.handoffInboxConsoleHandle is only set when we've been delegated to + // * We can't just pass something like the PID, because the OS conhost + // already expects a literal handle value via the HostSignalInputThread. + // If we changed that in the OpenConsole version, then there'll surely be + // the chance for a mispatch between the OS conhost and the delegated + // console. if (const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }) { LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), @@ -52,6 +63,7 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId, ConsoleProcessHandle::~ConsoleProcessHandle() { + // Close out the handle in the origin conhost. const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }; if (_hProcessInConhost && conhost) { @@ -59,7 +71,7 @@ ConsoleProcessHandle::~ConsoleProcessHandle() _hProcessInConhost.get(), nullptr /* hTargetProcessHandle */, nullptr /* lpTargetHandle, ignored */, - 0 /*dwDesiredAccess, ignored*/, + 0 /* dwDesiredAccess, ignored */, false, DUPLICATE_CLOSE_SOURCE)); } diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index 15a7090642d..20f45391930 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -305,7 +305,11 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { const auto pProcessHandle = *it; - if (const auto& handleToUse = pProcessHandle->_hProcessInConhost ? pProcessHandle->_hProcessInConhost : pProcessHandle->_hProcess) + // If we're a delgated-to console (OpenConsole), then make sure to pass + // in the value of the handle in the origin conhost, not our own. + if (const auto& handleToUse = pProcessHandle->_hProcessInConhost ? + pProcessHandle->_hProcessInConhost : + pProcessHandle->_hProcess) { _ModifyProcessForegroundRights(handleToUse.get(), fForeground); } From a4cd0344d717e873018b9d60c9db1d3d73017e39 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 8 Jun 2022 10:52:54 -0500 Subject: [PATCH 7/8] Oh man only one spelling mistake --- src/server/ProcessList.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index 20f45391930..5051e305694 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -305,7 +305,7 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { const auto pProcessHandle = *it; - // If we're a delgated-to console (OpenConsole), then make sure to pass + // If we're a delegated-to console (OpenConsole), then make sure to pass // in the value of the handle in the origin conhost, not our own. if (const auto& handleToUse = pProcessHandle->_hProcessInConhost ? pProcessHandle->_hProcessInConhost : From 55a73310a075a9c63b90fb562f36fda78adf6494 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 9 Jun 2022 15:17:47 -0500 Subject: [PATCH 8/8] This is far simpler --- .../base/HostSignalInputThread.cpp | 9 ++-- .../base/RemoteConsoleControl.cpp | 10 ++--- .../base/RemoteConsoleControl.hpp | 3 ++ src/server/ProcessHandle.cpp | 41 ------------------- src/server/ProcessHandle.h | 3 +- src/server/ProcessList.cpp | 8 +--- 6 files changed, 14 insertions(+), 60 deletions(-) diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp index 82f982595bb..bed01bbd013 100644 --- a/src/interactivity/base/HostSignalInputThread.cpp +++ b/src/interactivity/base/HostSignalInputThread.cpp @@ -94,12 +94,11 @@ T HostSignalInputThread::_ReceiveTypedPacket() } case HostSignals::SetForeground: { - auto msg = _ReceiveTypedPacket(); + _ReceiveTypedPacket(); - // GH#13211 - The OpenConsole side should have already duplicated - // the handle to us, and processId should be the value in our - // process space. - LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground)); + // GH#13211 - This honestly shouldn't be called by OpenConsole + // anymore, but it's possible that a much older version of + // OpenConsole is still calling this. Just do nothing. break; } diff --git a/src/interactivity/base/RemoteConsoleControl.cpp b/src/interactivity/base/RemoteConsoleControl.cpp index 0b9283ba8bb..7cd03e36317 100644 --- a/src/interactivity/base/RemoteConsoleControl.cpp +++ b/src/interactivity/base/RemoteConsoleControl.cpp @@ -57,12 +57,10 @@ template [[nodiscard]] NTSTATUS RemoteConsoleControl::SetForeground(_In_ HANDLE hProcess, _In_ BOOL fForeground) { - HostSignalSetForegroundData data{}; - data.sizeInBytes = sizeof(data); - data.processId = HandleToULong(hProcess); - data.isForeground = fForeground; - - return _SendTypedPacket(_pipe.get(), HostSignals::SetForeground, data); + // GH#13211 - Apparently this API doesn't need to be forwarded to conhost at + // all. Instead, just perform the ConsoleControl operation here, in proc. + // This lets us avoid all sorts of strange handle duplicating weirdness. + return _control.SetForeground(hProcess, fForeground); } [[nodiscard]] NTSTATUS RemoteConsoleControl::EndTask(_In_ DWORD dwProcessId, _In_ DWORD dwEventType, _In_ ULONG ulCtrlFlags) diff --git a/src/interactivity/base/RemoteConsoleControl.hpp b/src/interactivity/base/RemoteConsoleControl.hpp index ee22aeef4dc..b922b9beaa6 100644 --- a/src/interactivity/base/RemoteConsoleControl.hpp +++ b/src/interactivity/base/RemoteConsoleControl.hpp @@ -14,6 +14,7 @@ Author(s): #pragma once #include "../inc/IConsoleControl.hpp" +#include "../win32/ConsoleControl.hpp" namespace Microsoft::Console::Interactivity { @@ -29,5 +30,7 @@ namespace Microsoft::Console::Interactivity private: wil::unique_handle _pipe; + + Win32::ConsoleControl _control; }; } diff --git a/src/server/ProcessHandle.cpp b/src/server/ProcessHandle.cpp index 09229f9409e..7c2adfc6417 100644 --- a/src/server/ProcessHandle.cpp +++ b/src/server/ProcessHandle.cpp @@ -8,9 +8,6 @@ #include "../host/globals.h" #include "../host/telemetry.hpp" -#include "../interactivity/inc/ServiceLocator.hpp" - -using namespace Microsoft::Console::Interactivity; // Routine Description: // - Constructs an instance of the ConsoleProcessHandle Class // - NOTE: Can throw if allocation fails or if there is a console policy we do not understand. @@ -37,44 +34,6 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId, { Telemetry::Instance().LogProcessConnected(_hProcess.get()); } - - // GH#13211 - If we're running as the delegation console (someone handed off - // to us), then we need to make sure the original conhost has access to this - // process handle as well. Otherwise, the future calls to - // ConsoleControl(SetForeground,...) won't work, because the literal handle - // value doesn't exist in the original conhost process space. - // * g.handoffInboxConsoleHandle is only set when we've been delegated to - // * We can't just pass something like the PID, because the OS conhost - // already expects a literal handle value via the HostSignalInputThread. - // If we changed that in the OpenConsole version, then there'll surely be - // the chance for a mispatch between the OS conhost and the delegated - // console. - if (const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }) - { - LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), - _hProcess.get(), - conhost.get(), - _hProcessInConhost.put(), - 0 /*dwDesiredAccess, ignored*/, - false, - DUPLICATE_SAME_ACCESS)); - } -} - -ConsoleProcessHandle::~ConsoleProcessHandle() -{ - // Close out the handle in the origin conhost. - const auto& conhost{ ServiceLocator::LocateGlobals().handoffInboxConsoleHandle }; - if (_hProcessInConhost && conhost) - { - LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(conhost.get(), - _hProcessInConhost.get(), - nullptr /* hTargetProcessHandle */, - nullptr /* lpTargetHandle, ignored */, - 0 /* dwDesiredAccess, ignored */, - false, - DUPLICATE_CLOSE_SOURCE)); - } } // Routine Description: diff --git a/src/server/ProcessHandle.h b/src/server/ProcessHandle.h index 5772fd12650..50084f45430 100644 --- a/src/server/ProcessHandle.h +++ b/src/server/ProcessHandle.h @@ -50,7 +50,7 @@ class ConsoleProcessHandle ConsoleProcessHandle(const DWORD dwProcessId, const DWORD dwThreadId, const ULONG ulProcessGroupId); - ~ConsoleProcessHandle(); + ~ConsoleProcessHandle() = default; ConsoleProcessHandle(const ConsoleProcessHandle&) = delete; ConsoleProcessHandle(ConsoleProcessHandle&&) = delete; ConsoleProcessHandle& operator=(const ConsoleProcessHandle&) & = delete; @@ -59,7 +59,6 @@ class ConsoleProcessHandle ULONG _ulTerminateCount; ULONG const _ulProcessGroupId; wil::unique_handle const _hProcess; - wil::unique_handle _hProcessInConhost; mutable ULONG64 _processCreationTime; diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index 5051e305694..ab9c529a76d 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -305,13 +305,9 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { const auto pProcessHandle = *it; - // If we're a delegated-to console (OpenConsole), then make sure to pass - // in the value of the handle in the origin conhost, not our own. - if (const auto& handleToUse = pProcessHandle->_hProcessInConhost ? - pProcessHandle->_hProcessInConhost : - pProcessHandle->_hProcess) + if (pProcessHandle->_hProcess) { - _ModifyProcessForegroundRights(handleToUse.get(), fForeground); + _ModifyProcessForegroundRights(pProcessHandle->_hProcess.get(), fForeground); } it = std::next(it);