starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-04-13 01:31:09 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Refactor ConsoleProcessList (#14421)

Browse Source 
This commit is just a slight refactor of `ConsoleProcessList` which I've noticed
was in a poor shape. It replaces iterators with for-range loops, etc.

Additionally this fixes a bug in `SetConsoleWindowOwner`, where it used to
default to the newest client process instead of the oldest.

Finally, it changes the process container type from a doubly linked list
over to a simple array/vector, because using linked lists for heap allocated
elements felt quite a bit silly. To preserve the previous behavior of
`GetProcessList`, it simply iterates through the vector backwards.

## Validation Steps Performed
* All unit/feature tests pass ✅
* Launching a TUI application inside pwsh inside cmd
  and exiting kills all 3 applications ✅
This commit is contained in:
Leonard Hecker
2022-12-03 00:15:57 +01:00
committed by GitHub
parent 3c78e01ab5
commit 391abafc2e
8 changed files with 146 additions and 206 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
src/host/ft_fuzzer/fuzzmain.cpp
View File

@@ -74,7 +74,6 @@ struct NullDeviceComm : public IDeviceComm
RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(GetCurrentProcessId(),
GetCurrentThreadId(),
0,
nullptr,
&pProcessHandle));
pProcessHandle->fRootProcess = true;

38
src/host/input.cpp
View File

@@ -275,27 +275,21 @@ void ProcessCtrlEvents()
const auto LimitingProcessId = gci.LimitingProcessId;
gci.LimitingProcessId = 0;
ConsoleProcessTerminationRecord* rgProcessHandleList;
size_t cProcessHandleList;
std::vector<ConsoleProcessTerminationRecord> termRecords;
const auto hr = gci.ProcessHandleList
.GetTerminationRecordsByGroupId(LimitingProcessId,
WI_IsFlagSet(gci.CtrlFlags,
CONSOLE_CTRL_CLOSE_FLAG),
termRecords);
auto hr = gci.ProcessHandleList
.GetTerminationRecordsByGroupId(LimitingProcessId,
WI_IsFlagSet(gci.CtrlFlags,
CONSOLE_CTRL_CLOSE_FLAG),
&rgProcessHandleList,
&cProcessHandleList);
if (FAILED(hr) || cProcessHandleList == 0)
if (FAILED(hr) || termRecords.empty())
{
gci.UnlockConsole();
return;
}
// Copy ctrl flags.
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;
const auto CtrlFlags = std::exchange(gci.CtrlFlags, 0);
gci.UnlockConsole();
@@ -330,10 +324,13 @@ void ProcessCtrlEvents()
case CONSOLE_CTRL_SHUTDOWN_FLAG:
EventType = CTRL_SHUTDOWN_EVENT;
break;
default:
return;
}
auto Status = STATUS_SUCCESS;
for (size_t i = 0; i < cProcessHandleList; i++)
for (const auto& r : termRecords)
{
/*
* Status will be non-successful if a process attached to this console
@@ -347,20 +344,13 @@ void ProcessCtrlEvents()
if (NT_SUCCESS(Status))
{
Status = ServiceLocator::LocateConsoleControl()
->EndTask(rgProcessHandleList[i].dwProcessID,
->EndTask(r.dwProcessID,
EventType,
CtrlFlags);
if (rgProcessHandleList[i].hProcess == nullptr)
if (!r.hProcess)
{
Status = STATUS_SUCCESS;
}
}
if (rgProcessHandleList[i].hProcess != nullptr)
{
CloseHandle(rgProcessHandleList[i].hProcess);
}
}
delete[] rgProcessHandleList;
}

6
src/interactivity/win32/windowio.cpp
View File

@@ -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.FindProcessInList(ConsoleProcessList::ROOT_PROCESS_ID);
pProcessData = gci.ProcessHandleList.GetRootProcess();
if (!pProcessData)
{
// No root process ID? Pick the oldest existing process.
pProcessData = gci.ProcessHandleList.GetFirstProcess();
pProcessData = gci.ProcessHandleList.GetOldestProcess();
}
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.FindProcessInList(ConsoleProcessList::ROOT_PROCESS_ID);
auto ProcessData = gci.ProcessHandleList.GetRootProcess();
FAIL_FAST_IF(!(ProcessData != nullptr && ProcessData->fRootProcess));
// Create and activate the main window

1
src/server/ApiDispatchersInternal.cpp
View File

@@ -93,7 +93,6 @@ using Microsoft::Console::Interactivity::ServiceLocator;
RETURN_IF_FAILED(gci.ProcessHandleList.AllocProcessData(a->ProcessGroupId,
0,
a->ProcessGroupId,
ProcessHandle,
nullptr));
}
}

1
src/server/IoDispatchers.cpp
View File

@@ -452,7 +452,6 @@ 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))

18
src/server/ProcessHandle.h
View File

@@ -28,6 +28,15 @@ 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<ConsoleWaitQueue> pWaitBlockQueue;
std::unique_ptr<ConsoleHandleData> pInputHandle;
std::unique_ptr<ConsoleHandleData> pOutputHandle;
@@ -47,15 +56,6 @@ public:
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;

270
src/server/ProcessList.cpp
View File

@@ -5,10 +5,7 @@
#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;
@@ -25,56 +22,31 @@ 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.
// - E_FAIL if we're running into an LPC port conflict by nature of the process chain.
// - S_FALSE 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)
{
FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
auto pProcessData = FindProcessInList(dwProcessId);
if (nullptr != pProcessData)
if (FindProcessInList(dwProcessId))
{
// 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);
}
return S_FALSE;
}
std::unique_ptr<ConsoleProcessHandle> pProcessData;
try
{
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;
}
pProcessData = std::make_unique<ConsoleProcessHandle>(dwProcessId, dwThreadId, ulProcessGroupId);
_processes.emplace_back(pProcessData.get());
}
CATCH_RETURN();
RETURN_HR(S_OK);
wil::assign_to_opt_param(ppProcessData, pProcessData.release());
return S_OK;
}
// Routine Description:
@@ -85,47 +57,39 @@ using namespace Microsoft::Console::Interactivity;
// - <none>
void ConsoleProcessList::FreeProcessData(_In_ ConsoleProcessHandle* const pProcessData)
{
FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
// 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;
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);
}
}
// 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 or ROOT_PROCESS_ID to find the root process.
// - dwProcessId - ID of the process to search for.
// Return Value:
// - Pointer to the process handle information or nullptr if no match was found.
ConsoleProcessHandle* ConsoleProcessList::FindProcessInList(const DWORD dwProcessId) const
{
auto it = _processes.cbegin();
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
while (it != _processes.cend())
for (const auto& p : _processes)
{
const auto pProcessHandleRecord = *it;
if (ROOT_PROCESS_ID != dwProcessId)
if (p->dwProcessId == dwProcessId)
{
if (pProcessHandleRecord->dwProcessId == dwProcessId)
{
return pProcessHandleRecord;
}
return p;
}
else
{
if (pProcessHandleRecord->fRootProcess)
{
return pProcessHandleRecord;
}
}
it = std::next(it);
}
return nullptr;
@@ -139,17 +103,53 @@ 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
{
auto it = _processes.cbegin();
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
while (it != _processes.cend())
for (const auto& p : _processes)
{
const auto pProcessHandleRecord = *it;
if (pProcessHandleRecord->_ulProcessGroupId == ulProcessGroupId)
if (p->_ulProcessGroupId == ulProcessGroupId)
{
return pProcessHandleRecord;
return p;
}
}
it = std::next(it);
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());
for (const auto& p : _processes)
{
if (p->fRootProcess)
{
return p;
}
}
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:
// - <none>
// 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();
}
return nullptr;
@@ -157,7 +157,6 @@ ConsoleProcessHandle* ConsoleProcessList::FindProcessByGroupId(_In_ ULONG ulProc
// 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.
@@ -165,36 +164,30 @@ ConsoleProcessHandle* ConsoleProcessList::FindProcessByGroupId(_In_ ULONG ulProc
// 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* const pProcessList,
[[nodiscard]] HRESULT ConsoleProcessList::GetProcessList(_Inout_updates_(*pcProcessList) DWORD* pProcessList,
_Inout_ size_t* const pcProcessList) const
{
auto hr = S_OK;
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
const auto cProcesses = _processes.size();
// If we can fit inside the given list space, copy out the data.
if (cProcesses <= *pcProcessList)
if (*pcProcessList < _processes.size())
{
size_t cFilled = 0;
// 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
{
hr = E_NOT_SUFFICIENT_BUFFER;
*pcProcessList = _processes.size();
return E_NOT_SUFFICIENT_BUFFER;
}
// Return how many items were copied (or how many values we would need to fit).
*pcProcessList = cProcesses;
// 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();
return hr;
for (; it != end; ++it)
{
*pProcessList++ = (*it)->dwProcessId;
}
*pcProcessList = _processes.size();
return S_OK;
}
// Routine Description
@@ -210,85 +203,48 @@ ConsoleProcessHandle* ConsoleProcessList::FindProcessByGroupId(_In_ ULONG ulProc
// - E_OUTOFMEMORY in a low memory situation.
[[nodiscard]] HRESULT ConsoleProcessList::GetTerminationRecordsByGroupId(const DWORD dwLimitingProcessId,
const bool fCtrlClose,
_Outptr_result_buffer_all_(*pcRecords) ConsoleProcessTerminationRecord** prgRecords,
_Out_ size_t* const pcRecords) const
_Out_ std::vector<ConsoleProcessTerminationRecord>& termRecords) const
{
*pcRecords = 0;
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
try
{
std::deque<std::unique_ptr<ConsoleProcessTerminationRecord>> TermRecords;
termRecords.clear();
// Dig through known processes looking for a match
auto it = _processes.cbegin();
while (it != _processes.cend())
for (const auto& p : _processes)
{
const auto pProcessHandleRecord = *it;
// If no limit was specified OR if we have a match, generate a new termination record.
if (0 == dwLimitingProcessId ||
pProcessHandleRecord->_ulProcessGroupId == dwLimitingProcessId)
if (!dwLimitingProcessId ||
p->_ulProcessGroupId == dwLimitingProcessId)
{
auto pNewRecord = std::make_unique<ConsoleProcessTerminationRecord>();
// If we're hard closing the window, increment the counter.
if (fCtrlClose)
{
p->_ulTerminateCount++;
}
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(),
pProcessHandleRecord->_hProcess.get(),
p->_hProcess.get(),
GetCurrentProcess(),
&pNewRecord->hProcess,
&process,
0,
0,
DUPLICATE_SAME_ACCESS));
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));
termRecords.emplace_back(ConsoleProcessTerminationRecord{
.hProcess = std::move(process),
.dwProcessID = p->dwProcessId,
.ulTerminateCount = p->_ulTerminateCount,
});
}
it = std::next(it);
}
// 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;
return S_OK;
}
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:
// - <none>
// 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:
@@ -300,17 +256,14 @@ ConsoleProcessHandle* ConsoleProcessList::GetFirstProcess() 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)
{
auto it = _processes.cbegin();
while (it != _processes.cend())
{
const auto pProcessHandle = *it;
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());
for (const auto& pProcessHandle : _processes)
{
if (pProcessHandle->_hProcess)
{
_ModifyProcessForegroundRights(pProcessHandle->_hProcess.get(), fForeground);
}
it = std::next(it);
}
// Do this for conhost.exe itself, too.
@@ -326,6 +279,7 @@ 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();
}

17
src/server/ProcessList.h
View File

@@ -23,7 +23,10 @@ Revision History:
// holding the console lock.
struct ConsoleProcessTerminationRecord
{
HANDLE hProcess;
// 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;
DWORD dwProcessID;
ULONG ulTerminateCount;
};
@@ -31,25 +34,21 @@ 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,
_Outptr_result_buffer_all_(*pcRecords) ConsoleProcessTerminationRecord** prgRecords,
_Out_ size_t* const pcRecords) const;
ConsoleProcessHandle* GetFirstProcess() const;
std::vector<ConsoleProcessTerminationRecord>& termRecords) const;
[[nodiscard]] HRESULT GetProcessList(_Inout_updates_(*pcProcessList) DWORD* const pProcessList,
_Inout_ size_t* const pcProcessList) const;
@@ -59,7 +58,7 @@ public:
bool IsEmpty() const;
private:
std::list<ConsoleProcessHandle*> _processes;
std::vector<ConsoleProcessHandle*> _processes;
void _ModifyProcessForegroundRights(const HANDLE hProcess, const bool fForeground) const;
};