Skip to content

Commit

Permalink
lemme tell you, this is NOT a HANDLE
Browse files Browse the repository at this point in the history
  • Loading branch information
zadjii-msft committed Jun 8, 2022
1 parent e970f7c commit 7d82c81
Show file tree
Hide file tree
Showing 12 changed files with 36 additions and 21 deletions.
2 changes: 1 addition & 1 deletion src/host/input.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions src/inc/HostSignals.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
9 changes: 4 additions & 5 deletions src/interactivity/base/HostSignalInputThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,9 @@ T HostSignalInputThread::_ReceiveTypedPacket()
{
auto msg = _ReceiveTypedPacket<HostSignalSetForegroundData>();

// 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;
Expand All @@ -108,7 +107,7 @@ T HostSignalInputThread::_ReceiveTypedPacket()
{
auto msg = _ReceiveTypedPacket<HostSignalEndTaskData>();

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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/base/RemoteConsoleControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ template<typename T>
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;

Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/base/RemoteConsoleControl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/inc/IConsoleControl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}
4 changes: 2 additions & 2 deletions src/interactivity/onecore/ConsoleControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/onecore/ConsoleControl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}
4 changes: 2 additions & 2 deletions src/interactivity/win32/ConsoleControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/win32/ConsoleControl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 13 additions & 1 deletion src/server/ProcessHandle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -52,14 +63,15 @@ ConsoleProcessHandle::ConsoleProcessHandle(const DWORD dwProcessId,

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*/,
0 /* dwDesiredAccess, ignored */,
false,
DUPLICATE_CLOSE_SOURCE));
}
Expand Down
6 changes: 5 additions & 1 deletion src/server/ProcessList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

1 comment on commit 7d82c81

@github-actions

This comment was marked as duplicate.

Please sign in to comment.