Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix order and robustness of CTRL_*_EVENTs #18233

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions src/host/input.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -321,28 +321,35 @@ void ProcessCtrlEvents()
return;
}

auto Status = STATUS_SUCCESS;
const auto ctrl = ServiceLocator::LocateConsoleControl();

for (const auto& r : termRecords)
{
/*
* Status will be non-successful if a process attached to this console
* vetoes shutdown. In that case, we don't want to try to kill any more
* processes, but we do need to make sure we continue looping so we
* can close any remaining process handles. The exception is if the
* process is inaccessible, such that we can't even open a handle for
* query. In this case, use best effort to send the close event but
* ignore any errors.
*/
if (SUCCEEDED_NTSTATUS(Status))
{
Status = ServiceLocator::LocateConsoleControl()
->EndTask(r.dwProcessID,
EventType,
CtrlFlags);
if (!r.hProcess)
{
Status = STATUS_SUCCESS;
}
}
// Older versions of Windows would do various things if the EndTask() call failed:
// * XP: Pops up a "Windows can't end this program" dialog for every already-dead process.
// * Vista - Win 7: Simply skips over already-dead processes.
// * Win 8 - Win 11 26100: Aborts on an already-dead process (you have to WM_CLOSE conhost multiple times).
//
// That last period had the following comment:
// Status will be non-successful if a process attached to this console
// vetoes shutdown. In that case, we don't want to try to kill any more
// processes, but we do need to make sure we continue looping so we
// can close any remaining process handles. The exception is if the
// process is inaccessible, such that we can't even open a handle for
// query. In this case, use best effort to send the close event but
// ignore any errors.
//
// The corresponding logic worked like this:
// if (FAILED(EndTask(...)) && r.hProcess) {
// break;
// }
//
// That logic was removed around the Windows 11 26100 time frame, because CSRSS
// (which handles EndTask) now waits 5s and then force-kills the process for us.
// Going back to the Win 7 behavior then should make shutdown a lot more robust.
// The bad news is that EndTask() returns STATUS_UNSUCCESSFUL no matter whether
// the process was already dead, or if the request actually failed for some reason.
// Hopefully there aren't any regressions, but we can't know without trying.
LOG_IF_NTSTATUS_FAILED(ctrl->EndTask(r.dwProcessID, EventType, CtrlFlags));
}
}
8 changes: 7 additions & 1 deletion src/server/ProcessList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,9 +211,15 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const
{
termRecords.clear();

// The caller (ProcessCtrlEvents) expects them in newest-to-oldest order,
// because that's how Windows has historically always dispatched these events.
auto it = _processes.crbegin();
const auto end = _processes.crend();

// Dig through known processes looking for a match
for (const auto& p : _processes)
for (; it != end; ++it)
{
const auto p = *it;
// If no limit was specified OR if we have a match, generate a new termination record.
if (!dwLimitingProcessId ||
p->_ulProcessGroupId == dwLimitingProcessId)
Expand Down
Loading