GenerateConsoleCtrlEvent should not succeed when dwProcessGroupId is not a group ID

[eryksun]

The dwProcessGroupId parameter of GenerateConsoleCtrlEvent should be limited to process groups or the special group 0 that means all process attached to the console. If no process belongs to the given group ID, the call should fail with ERROR_INVALID_PARAMETER. This used to be the case (and the code I've read in ReactOS seems as simple as this), but at some point (XP?) it was changed to succeed for any process ID if it happens to be a child of a process that's attached to the console. I have to assume this was intentional, though I don't think any explanation could get me to agree with the intent.

If the target process is attached to the console, it behaves the same as the group 0 case, which contributes to misunderstandings about this function. Otherwise it's relatively benign. The truly weird and buggy aspect of this is that it succeeds for a child of a process that's attached to the console even if the child itself is not attached to the console (e.g. a non-console process, or one created with DETACHED_PROCESS, CREATE_NEW_CONSOLE, or CREATE_NO_WINDOW). In this case the call appears to do nothing. But on closer inspection, we see that the target process gets added to the console's process list (i.e. GetConsoleProcessList) even though it's not attached to the console, and in Process Explorer we see that conhost.exe has a handle for the process. If the process terminates, the console doesn't get notified, so this handle for a defunct process stays in the console's list. Apparently this puts the console's process list in a bad state. All processes that were added before the defunct process are subsequently invisible to GenerateConsoleCtrlEvent, even if we target group 0.

[eryksun]

some point (XP?) it was changed to succeed for any process ID if it happens to be a child of a process that's attached to the console. I have to assume this was intentional

Now that the source is published, I can finally see that the change was clearly intentional, but I can't imagine who this was meant to help, since it fails in all cases.

If ApiDispatchers::ServerGenerateConsoleCtrlEvent() can't find a process with the given group ID, it reinterprets the group ID as a process ID (undocumented behavior) and checks whether the parent process is connected. If so, it attempts to add a dummy process record via AllocProcessData(), using the process ID as the group ID. (If this process is already attached to the console, then AllocProcessData() does nothing.)

Next it sets the given group ID as the LimitingProcessId (a confused name since it's actually a process group ID) and calls HandleCtrlEvent() to set the global CtrlFlags to CONSOLE_CTRL_C_FLAG or CONSOLE_CTRL_BREAK_FLAG. This is handled in the next call to ProcessCtrlEvents(), which calls ConsoleControl::EndTask() on the list of matching processes. In turn, this makes an LPC call to the Windows session server, "csrss.exe" (in particular the user server in "winsrv[ext].dll"), via CsrClientCallServer(). The server creates a thread in each process that executes kernelbase!CtrlRoutine(dwCtrlEvent).

If the target process is attached to the console, it behaves the same as the group 0 case, which contributes to misunderstandings about this function.

In the case where we target a process ID that's not a group ID but both it and its parent are attached to the console, then LimitingProcessId matches no process group, as determined by GetTerminationRecordsByGroupId(). In this case, ProcessCtrlEvents() simply clears LimitingProcessId to 0 and returns. However, it neglects to clear CtrlFlags. So the next time ProcessCtrlEvents() is called, it mistakenly broadcasts the control event to every process (i.e. group 0).

The truly weird and buggy aspect of this is that it succeeds for a child of a process that's attached to the console even if the child itself is not attached to the console (e.g. a non-console process, or one created with DETACHED_PROCESS, CREATE_NEW_CONSOLE, or CREATE_NO_WINDOW). In this case the call appears to do nothing. But on closer inspection, we see that the target process gets added to the console's process list (i.e. GetConsoleProcessList) even though it's not attached to the console, and in Process Explorer we see that conhost.exe has a handle for the process.

As mentioned above, if the target isn't attached to the console but its parent is, then a dummy process record is added via AllocProcessData(), which uses the target process ID as the group ID. In this case nothing fails immediately. The CsrClientCallServer() call even succeeds, albeit the Windows session server doesn't actually send the control event since the process isn't attached to the console. That's not a problem in itself. However, bugs are lying in wait.

If the process terminates, the console doesn't get notified, so this handle for a defunct process stays in the console's list. Apparently this puts the console's process list in a bad state. All processes that were added before the defunct process are subsequently invisible to GenerateConsoleCtrlEvent, even if we target group 0.

Since the process isn't connected to the console, IoDispatchers::ConsoleClientDisconnectRoutine() is never called to remove the record via RemoveConsole().

Once the process exists, subsequent calls to ProcessCtrlEvents() that include the process (e.g. the limiting process ID is 0) will stop processing as soon as they reach it because the CsrClientCallServer() call fails with STATUS_UNSUCCESSFUL (0xC0000001). Subsequent processes are skipped due to an outdated assumption that CsrClientCallServer() fails if a process vetoes console shutdown. That hasn't been supported since Vista. (If the console is shutting down, by default an attached process has 5 seconds to exit on its own. After a timeout, or if the control handler of the process returns TRUE, the Windows server forcefully terminates the process.)

[malxau]

Rephrasing a little in terms of impact: if GenerateConsoleCtrlEvent is called on a GUI process, the result will mess up signal handling in the console. This is a problem when programs don't know whether they're launching a console or GUI process, eg. when operating on arbitrary executables.

The following is a simple repro which will launch notepad and call GenerateConsoleCtrlEvent.
test.c.txt

To use:

1. Run the test app, preferably in a new console to reduce interference
2. It will launch notepad
3. Switch back to its console, hit Ctrl+C
4. This will terminate notepad and relaunch
5. ...but you can't use Ctrl+C after that, and you can't close the console window.

Based on eryksun's analysis, I made the following change, which "fixes" the behavior by not aborting signal handling when it encounters a bogus process. I don't think this is the "real" fix but it confirms that this is the problem.

diff --git a/src/host/input.cpp b/src/host/input.cpp
index 6ce904b..13d9e80 100644
--- a/src/host/input.cpp
+++ b/src/host/input.cpp
@@ -330,9 +330,7 @@ void ProcessCtrlEvents()
                 ->EndTask((HANDLE)rgProcessHandleList[i].dwProcessID,
                           EventType,
                           CtrlFlags);
-            if (rgProcessHandleList[i].hProcess == nullptr) {
-                Status = STATUS_SUCCESS;
-            }
+            Status = STATUS_SUCCESS;
         }

         if (rgProcessHandleList[i].hProcess != nullptr)

[DaanDeMeyer]

I can verify the repro works on my Windows 10 installation as well. This seems like a pretty serious bug as Python relies on the exact same GenerateConsoleCtrlEvent to stop processes on Windows (See: https://github.com/python/cpython/blob/c4cacc8c5eab50db8da3140353596f38a01115ca/Modules/posixmodule.c#L7191)