Fix order and robustness of CTRL_*_EVENTs

microsoft · Nov 21, 2024 · ef96d03 · ef96d03
1 parent 282670a
commit ef96d03
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 26 deletions.
diff --git a/OpenConsole.sln b/OpenConsole.sln
@@ -2304,10 +2304,14 @@ Global
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x64.Build.0 = Release|x64
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.ActiveCfg = Release|Win32
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.Build.0 = Release|Win32
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|Any CPU.ActiveCfg = Debug|x64
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.ActiveCfg = Debug|ARM64
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.Build.0 = Debug|ARM64

diff --git a/src/host/input.cpp b/src/host/input.cpp
@@ -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 at this point:
+        // * 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 - Windows 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 CRSS
+        // (which handles EndTask) now waits 5s and then force-kills the process for us.
+        // Going back to the old 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));
     }
 }
diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp
@@ -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)