From dec420eb422a0cac51ab5b1d02ad73b99db4d2bb Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Mon, 12 Dec 2022 21:17:58 -0600 Subject: [PATCH] Partially revert 111a6c0c5 (Refactor ConsoleProcessList) --- src/host/ft_fuzzer/fuzzmain.cpp | 1 + src/host/input.cpp | 40 ++-- src/interactivity/win32/windowio.cpp | 6 +- src/server/ApiDispatchersInternal.cpp | 1 + src/server/IoDispatchers.cpp | 1 + src/server/ProcessHandle.h | 18 +- src/server/ProcessList.cpp | 268 +++++++++++++++----------- src/server/ProcessList.h | 17 +- 8 files changed, 206 insertions(+), 146 deletions(-) diff --git a/src/host/ft_fuzzer/fuzzmain.cpp b/src/host/ft_fuzzer/fuzzmain.cpp index 87f2ee78897..27763ebf211 100644 --- a/src/host/ft_fuzzer/fuzzmain.cpp +++ b/src/host/ft_fuzzer/fuzzmain.cpp @@ -74,6 +74,7 @@ struct NullDeviceComm : public IDeviceComm RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(GetCurrentProcessId(), GetCurrentThreadId(), 0, + nullptr, &pProcessHandle)); pProcessHandle->fRootProcess = true; diff --git a/src/host/input.cpp b/src/host/input.cpp index ff8a4f4dd54..d67afc34666 100644 --- a/src/host/input.cpp +++ b/src/host/input.cpp @@ -274,21 +274,27 @@ void ProcessCtrlEvents() const auto LimitingProcessId = gci.LimitingProcessId; gci.LimitingProcessId = 0; - std::vector termRecords; - const auto hr = gci.ProcessHandleList - .GetTerminationRecordsByGroupId(LimitingProcessId, - WI_IsFlagSet(gci.CtrlFlags, - CONSOLE_CTRL_CLOSE_FLAG), - termRecords); - - if (FAILED(hr) || termRecords.empty()) + ConsoleProcessTerminationRecord* rgProcessHandleList; + size_t cProcessHandleList; + + auto hr = gci.ProcessHandleList + .GetTerminationRecordsByGroupId(LimitingProcessId, + WI_IsFlagSet(gci.CtrlFlags, + CONSOLE_CTRL_CLOSE_FLAG), + &rgProcessHandleList, + &cProcessHandleList); + + if (FAILED(hr) || cProcessHandleList == 0) { gci.UnlockConsole(); return; } // Copy ctrl flags. - const auto CtrlFlags = std::exchange(gci.CtrlFlags, 0); + auto CtrlFlags = gci.CtrlFlags; + FAIL_FAST_IF(!(!((CtrlFlags & (CONSOLE_CTRL_CLOSE_FLAG | CONSOLE_CTRL_BREAK_FLAG | CONSOLE_CTRL_C_FLAG)) && (CtrlFlags & (CONSOLE_CTRL_LOGOFF_FLAG | CONSOLE_CTRL_SHUTDOWN_FLAG))))); + + gci.CtrlFlags = 0; gci.UnlockConsole(); @@ -323,13 +329,10 @@ void ProcessCtrlEvents() case CONSOLE_CTRL_SHUTDOWN_FLAG: EventType = CTRL_SHUTDOWN_EVENT; break; - - default: - return; } auto Status = STATUS_SUCCESS; - for (const auto& r : termRecords) + for (size_t i = 0; i < cProcessHandleList; i++) { /* * Status will be non-successful if a process attached to this console @@ -343,13 +346,20 @@ void ProcessCtrlEvents() if (NT_SUCCESS(Status)) { Status = ServiceLocator::LocateConsoleControl() - ->EndTask(r.dwProcessID, + ->EndTask(rgProcessHandleList[i].dwProcessID, EventType, CtrlFlags); - if (!r.hProcess) + if (rgProcessHandleList[i].hProcess == nullptr) { Status = STATUS_SUCCESS; } } + + if (rgProcessHandleList[i].hProcess != nullptr) + { + CloseHandle(rgProcessHandleList[i].hProcess); + } } + + delete[] rgProcessHandleList; } diff --git a/src/interactivity/win32/windowio.cpp b/src/interactivity/win32/windowio.cpp index 6df18d0a377..12ebeeea72a 100644 --- a/src/interactivity/win32/windowio.cpp +++ b/src/interactivity/win32/windowio.cpp @@ -78,11 +78,11 @@ VOID SetConsoleWindowOwner(const HWND hwnd, _Inout_opt_ ConsoleProcessHandle* pP else { // Find a process to own the console window. If there are none then let's use conhost's. - pProcessData = gci.ProcessHandleList.GetRootProcess(); + pProcessData = gci.ProcessHandleList.FindProcessInList(ConsoleProcessList::ROOT_PROCESS_ID); if (!pProcessData) { // No root process ID? Pick the oldest existing process. - pProcessData = gci.ProcessHandleList.GetOldestProcess(); + pProcessData = gci.ProcessHandleList.GetFirstProcess(); } if (pProcessData != nullptr) @@ -986,7 +986,7 @@ LRESULT CALLBACK DialogHookProc(int nCode, WPARAM /*wParam*/, LPARAM lParam) NTSTATUS InitWindowsSubsystem(_Out_ HHOOK* phhook) { auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - auto ProcessData = gci.ProcessHandleList.GetRootProcess(); + auto ProcessData = gci.ProcessHandleList.FindProcessInList(ConsoleProcessList::ROOT_PROCESS_ID); FAIL_FAST_IF(!(ProcessData != nullptr && ProcessData->fRootProcess)); // Create and activate the main window diff --git a/src/server/ApiDispatchersInternal.cpp b/src/server/ApiDispatchersInternal.cpp index 062cb5e5aa7..caea87f527d 100644 --- a/src/server/ApiDispatchersInternal.cpp +++ b/src/server/ApiDispatchersInternal.cpp @@ -93,6 +93,7 @@ using Microsoft::Console::Interactivity::ServiceLocator; RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId, 0, a->ProcessGroupId, + ProcessHandle, nullptr)); } } diff --git a/src/server/IoDispatchers.cpp b/src/server/IoDispatchers.cpp index a0c3270e8a9..8c032a90b24 100644 --- a/src/server/IoDispatchers.cpp +++ b/src/server/IoDispatchers.cpp @@ -435,6 +435,7 @@ PCONSOLE_API_MSG IoDispatchers::ConsoleHandleConnectionRequest(_In_ PCONSOLE_API Status = NTSTATUS_FROM_HRESULT(gci.ProcessHandleList.AllocProcessData(dwProcessId, dwThreadId, Cac.ProcessGroupId, + nullptr, &ProcessData)); if (!NT_SUCCESS(Status)) diff --git a/src/server/ProcessHandle.h b/src/server/ProcessHandle.h index 02f7ad9ba05..50084f45430 100644 --- a/src/server/ProcessHandle.h +++ b/src/server/ProcessHandle.h @@ -28,15 +28,6 @@ Revision History: class ConsoleProcessHandle { public: - ConsoleProcessHandle(const DWORD dwProcessId, - const DWORD dwThreadId, - const ULONG ulProcessGroupId); - ~ConsoleProcessHandle() = default; - ConsoleProcessHandle(const ConsoleProcessHandle&) = delete; - ConsoleProcessHandle(ConsoleProcessHandle&&) = delete; - ConsoleProcessHandle& operator=(const ConsoleProcessHandle&) & = delete; - ConsoleProcessHandle& operator=(ConsoleProcessHandle&&) & = delete; - const std::unique_ptr pWaitBlockQueue; std::unique_ptr pInputHandle; std::unique_ptr pOutputHandle; @@ -56,6 +47,15 @@ class ConsoleProcessHandle const ULONG64 GetProcessCreationTime() const; private: + ConsoleProcessHandle(const DWORD dwProcessId, + const DWORD dwThreadId, + const ULONG ulProcessGroupId); + ~ConsoleProcessHandle() = default; + ConsoleProcessHandle(const ConsoleProcessHandle&) = delete; + ConsoleProcessHandle(ConsoleProcessHandle&&) = delete; + ConsoleProcessHandle& operator=(const ConsoleProcessHandle&) & = delete; + ConsoleProcessHandle& operator=(ConsoleProcessHandle&&) & = delete; + ULONG _ulTerminateCount; ULONG const _ulProcessGroupId; wil::unique_handle const _hProcess; diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index e304556692a..ab9c529a76d 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -5,7 +5,10 @@ #include "ProcessList.h" +#include "../host/conwinuserrefs.h" #include "../host/globals.h" +#include "../host/telemetry.hpp" + #include "../interactivity/inc/ServiceLocator.hpp" using namespace Microsoft::Console::Interactivity; @@ -22,31 +25,56 @@ using namespace Microsoft::Console::Interactivity; // - If not used, return code will specify whether this process is known to the list or not. // Return Value: // - S_OK if the process was recorded in the list successfully or already existed. -// - S_FALSE if we're running into an LPC port conflict by nature of the process chain. +// - E_FAIL if we're running into an LPC port conflict by nature of the process chain. // - E_OUTOFMEMORY if there wasn't space to allocate a handle or push it into the list. [[nodiscard]] HRESULT ConsoleProcessList::AllocProcessData(const DWORD dwProcessId, const DWORD dwThreadId, const ULONG ulProcessGroupId, + _In_opt_ ConsoleProcessHandle* const pParentProcessData, _Outptr_opt_ ConsoleProcessHandle** const ppProcessData) { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked())); - if (FindProcessInList(dwProcessId)) + auto pProcessData = FindProcessInList(dwProcessId); + if (nullptr != pProcessData) { - return S_FALSE; + // In the GenerateConsoleCtrlEvent it's OK for this process to already have a ProcessData object. However, the other case is someone + // connecting to our LPC port and they should only do that once, so we fail subsequent connection attempts. + if (nullptr == pParentProcessData) + { + return E_FAIL; + // TODO: MSFT: 9574803 - This fires all the time. Did it always do that? + //RETURN_HR(E_FAIL); + } + else + { + if (nullptr != ppProcessData) + { + *ppProcessData = pProcessData; + } + RETURN_HR(S_OK); + } } - std::unique_ptr pProcessData; try { - pProcessData = std::make_unique(dwProcessId, dwThreadId, ulProcessGroupId); - _processes.emplace_back(pProcessData.get()); + pProcessData = new ConsoleProcessHandle(dwProcessId, + dwThreadId, + ulProcessGroupId); + + // Some applications, when reading the process list through the GetConsoleProcessList API, are expecting + // the returned list of attached process IDs to be from newest to oldest. + // As such, we have to put the newest process into the head of the list. + _processes.push_front(pProcessData); + + if (nullptr != ppProcessData) + { + *ppProcessData = pProcessData; + } } CATCH_RETURN(); - wil::assign_to_opt_param(ppProcessData, pProcessData.release()); - - return S_OK; + RETURN_HR(S_OK); } // Routine Description: @@ -57,39 +85,47 @@ using namespace Microsoft::Console::Interactivity; // - void ConsoleProcessList::FreeProcessData(_In_ ConsoleProcessHandle* const pProcessData) { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked())); - const auto it = std::ranges::find(_processes, pProcessData); - if (it != _processes.end()) - { - _processes.erase(it); - delete pProcessData; - } - else - { - // The pointer not existing in the process list would be similar to a heap corruption, - // as the only code allowed to allocate a `ConsoleProcessHandle` is us, in AllocProcessData(). - // An assertion here would indicate a double-free or similar. - assert(false); - } + // Assert that the item exists in the list. If it doesn't exist, the end/last will be returned. + FAIL_FAST_IF(!(_processes.cend() != std::find(_processes.cbegin(), _processes.cend(), pProcessData))); + + _processes.remove(pProcessData); + + delete pProcessData; } // Routine Description: // - Locates a process handle in this list. +// - NOTE: Calling FindProcessInList(0) means you want the root process. // Arguments: -// - dwProcessId - ID of the process to search for. +// - dwProcessId - ID of the process to search for or ROOT_PROCESS_ID to find the root process. // Return Value: // - Pointer to the process handle information or nullptr if no match was found. ConsoleProcessHandle* ConsoleProcessList::FindProcessInList(const DWORD dwProcessId) const { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + auto it = _processes.cbegin(); - for (const auto& p : _processes) + while (it != _processes.cend()) { - if (p->dwProcessId == dwProcessId) + const auto pProcessHandleRecord = *it; + + if (ROOT_PROCESS_ID != dwProcessId) { - return p; + if (pProcessHandleRecord->dwProcessId == dwProcessId) + { + return pProcessHandleRecord; + } + } + else + { + if (pProcessHandleRecord->fRootProcess) + { + return pProcessHandleRecord; + } } + + it = std::next(it); } return nullptr; @@ -103,53 +139,17 @@ ConsoleProcessHandle* ConsoleProcessList::FindProcessInList(const DWORD dwProces // - Pointer to first matching process handle with given group ID. nullptr if no match was found. ConsoleProcessHandle* ConsoleProcessList::FindProcessByGroupId(_In_ ULONG ulProcessGroupId) const { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); - - for (const auto& p : _processes) - { - if (p->_ulProcessGroupId == ulProcessGroupId) - { - return p; - } - } - - return nullptr; -} - -// Routine Description: -// - Locates the root process handle in this list. -// Return Value: -// - Pointer to the process handle information or nullptr if no match was found. -ConsoleProcessHandle* ConsoleProcessList::GetRootProcess() const -{ - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + auto it = _processes.cbegin(); - for (const auto& p : _processes) + while (it != _processes.cend()) { - if (p->fRootProcess) + const auto pProcessHandleRecord = *it; + if (pProcessHandleRecord->_ulProcessGroupId == ulProcessGroupId) { - return p; + return pProcessHandleRecord; } - } - - return nullptr; -} - -// Routine Description: -// - Gets the first process in the list. -// - Used for reassigning a new root process. -// TODO: MSFT 9450737 - encapsulate root process logic. https://osgvsowi/9450737 -// Arguments: -// - -// Return Value: -// - Pointer to the first item in the list or nullptr if there are no items. -ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const -{ - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); - if (!_processes.empty()) - { - return _processes.front(); + it = std::next(it); } return nullptr; @@ -157,6 +157,7 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const // Routine Description: // - Retrieves the entire list of process IDs that is known to this list. +// - Requires caller to allocate space. If not enough space, pcProcessList will be filled with count of array necessary. // Arguments: // - pProcessList - Pointer to buffer to store process IDs. Caller allocated. // - pcProcessList - On the way in, the length of the buffer given. On the way out, the amount of the buffer used. @@ -164,30 +165,36 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const // Return Value: // - S_OK if buffer was filled successfully and resulting count of items is in pcProcessList. // - E_NOT_SUFFICIENT_BUFFER if the buffer given was too small. Refer to pcProcessList for size requirement. -[[nodiscard]] HRESULT ConsoleProcessList::GetProcessList(_Inout_updates_(*pcProcessList) DWORD* pProcessList, +[[nodiscard]] HRESULT ConsoleProcessList::GetProcessList(_Inout_updates_(*pcProcessList) DWORD* const pProcessList, _Inout_ size_t* const pcProcessList) const { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + auto hr = S_OK; - if (*pcProcessList < _processes.size()) - { - *pcProcessList = _processes.size(); - return E_NOT_SUFFICIENT_BUFFER; - } + const auto cProcesses = _processes.size(); - // Some applications, when reading the process list through the GetConsoleProcessList API, - // are expecting the returned list of attached process IDs to be from newest to oldest. - // As such, we have to put the newest process into the head of the list. - auto it = _processes.crbegin(); - const auto end = _processes.crend(); + // If we can fit inside the given list space, copy out the data. + if (cProcesses <= *pcProcessList) + { + size_t cFilled = 0; - for (; it != end; ++it) + // Loop over the list of processes and fill in the caller's buffer. + auto it = _processes.cbegin(); + while (it != _processes.cend() && cFilled < *pcProcessList) + { + pProcessList[cFilled] = (*it)->dwProcessId; + cFilled++; + it = std::next(it); + } + } + else { - *pProcessList++ = (*it)->dwProcessId; + hr = E_NOT_SUFFICIENT_BUFFER; } - *pcProcessList = _processes.size(); - return S_OK; + // Return how many items were copied (or how many values we would need to fit). + *pcProcessList = cProcesses; + + return hr; } // Routine Description @@ -203,48 +210,85 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const // - E_OUTOFMEMORY in a low memory situation. [[nodiscard]] HRESULT ConsoleProcessList::GetTerminationRecordsByGroupId(const DWORD dwLimitingProcessId, const bool fCtrlClose, - _Out_ std::vector& termRecords) const + _Outptr_result_buffer_all_(*pcRecords) ConsoleProcessTerminationRecord** prgRecords, + _Out_ size_t* const pcRecords) const { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); + *pcRecords = 0; try { - termRecords.clear(); + std::deque> TermRecords; // Dig through known processes looking for a match - for (const auto& p : _processes) + auto it = _processes.cbegin(); + while (it != _processes.cend()) { + const auto pProcessHandleRecord = *it; + // If no limit was specified OR if we have a match, generate a new termination record. - if (!dwLimitingProcessId || - p->_ulProcessGroupId == dwLimitingProcessId) + if (0 == dwLimitingProcessId || + pProcessHandleRecord->_ulProcessGroupId == dwLimitingProcessId) { - // If we're hard closing the window, increment the counter. - if (fCtrlClose) - { - p->_ulTerminateCount++; - } + auto pNewRecord = std::make_unique(); - wil::unique_handle process; // If the duplicate failed, the best we can do is to skip including the process in the list and hope it goes away. LOG_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), - p->_hProcess.get(), + pProcessHandleRecord->_hProcess.get(), GetCurrentProcess(), - &process, + &pNewRecord->hProcess, 0, 0, DUPLICATE_SAME_ACCESS)); - termRecords.emplace_back(ConsoleProcessTerminationRecord{ - .hProcess = std::move(process), - .dwProcessID = p->dwProcessId, - .ulTerminateCount = p->_ulTerminateCount, - }); + pNewRecord->dwProcessID = pProcessHandleRecord->dwProcessId; + + // If we're hard closing the window, increment the counter. + if (fCtrlClose) + { + pProcessHandleRecord->_ulTerminateCount++; + } + + pNewRecord->ulTerminateCount = pProcessHandleRecord->_ulTerminateCount; + + TermRecords.push_back(std::move(pNewRecord)); } + + it = std::next(it); } - return S_OK; + // From all found matches, convert to C-style array to return + const auto cchRetVal = TermRecords.size(); + auto pRetVal = new ConsoleProcessTerminationRecord[cchRetVal]; + + for (size_t i = 0; i < cchRetVal; i++) + { + pRetVal[i] = *TermRecords.at(i); + } + + *prgRecords = pRetVal; + *pcRecords = cchRetVal; } CATCH_RETURN(); + + return S_OK; +} + +// Routine Description: +// - Gets the first process in the list. +// - Used for reassigning a new root process. +// TODO: MSFT 9450737 - encapsulate root process logic. https://osgvsowi/9450737 +// Arguments: +// - +// Return Value: +// - Pointer to the first item in the list or nullptr if there are no items. +ConsoleProcessHandle* ConsoleProcessList::GetFirstProcess() const +{ + if (!_processes.empty()) + { + return _processes.front(); + } + + return nullptr; } // Routine Description: @@ -256,14 +300,17 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const // - NOTE: Will attempt to request a change, but it's non fatal if it doesn't work. Failures will be logged to debug channel. void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); - - for (const auto& pProcessHandle : _processes) + auto it = _processes.cbegin(); + while (it != _processes.cend()) { + const auto pProcessHandle = *it; + if (pProcessHandle->_hProcess) { _ModifyProcessForegroundRights(pProcessHandle->_hProcess.get(), fForeground); } + + it = std::next(it); } // Do this for conhost.exe itself, too. @@ -279,7 +326,6 @@ void ConsoleProcessList::ModifyConsoleProcessFocus(const bool fForeground) // - True if the list is empty. False if we have known processes. bool ConsoleProcessList::IsEmpty() const { - assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()); return _processes.empty(); } diff --git a/src/server/ProcessList.h b/src/server/ProcessList.h index e790bc42c58..2b3e2af6994 100644 --- a/src/server/ProcessList.h +++ b/src/server/ProcessList.h @@ -23,10 +23,7 @@ Revision History: // holding the console lock. struct ConsoleProcessTerminationRecord { - // Unfortunately the reason for this was lost in time, but presumably a process - // handle is held so that we can refer to a process via PID (dwProcessID) without - // holding the console lock and fearing that the PID might get reused by the OS. - wil::unique_handle hProcess; + HANDLE hProcess; DWORD dwProcessID; ULONG ulTerminateCount; }; @@ -34,21 +31,25 @@ struct ConsoleProcessTerminationRecord class ConsoleProcessList { public: + static const DWORD ROOT_PROCESS_ID = 0; + [[nodiscard]] HRESULT AllocProcessData(const DWORD dwProcessId, const DWORD dwThreadId, const ULONG ulProcessGroupId, + _In_opt_ ConsoleProcessHandle* const pParentProcessData, _Outptr_opt_ ConsoleProcessHandle** const ppProcessData); void FreeProcessData(_In_ ConsoleProcessHandle* const ProcessData); ConsoleProcessHandle* FindProcessInList(const DWORD dwProcessId) const; ConsoleProcessHandle* FindProcessByGroupId(_In_ ULONG ulProcessGroupId) const; - ConsoleProcessHandle* GetRootProcess() const; - ConsoleProcessHandle* GetOldestProcess() const; [[nodiscard]] HRESULT GetTerminationRecordsByGroupId(const DWORD dwLimitingProcessId, const bool fCtrlClose, - std::vector& termRecords) const; + _Outptr_result_buffer_all_(*pcRecords) ConsoleProcessTerminationRecord** prgRecords, + _Out_ size_t* const pcRecords) const; + + ConsoleProcessHandle* GetFirstProcess() const; [[nodiscard]] HRESULT GetProcessList(_Inout_updates_(*pcProcessList) DWORD* const pProcessList, _Inout_ size_t* const pcProcessList) const; @@ -58,7 +59,7 @@ class ConsoleProcessList bool IsEmpty() const; private: - std::vector _processes; + std::list _processes; void _ModifyProcessForegroundRights(const HANDLE hProcess, const bool fForeground) const; };