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)

[GMZwinge]

This could make it impossible to send a console control event from one console to another without also sending the control even to the current console. Eg:

$ProcessID = <tbd>
$MemberDefinition = '
    [DllImport("kernel32.dll")]public static extern bool FreeConsole();
    [DllImport("kernel32.dll")]public static extern bool AttachConsole(uint p);
    [DllImport("kernel32.dll")]public static extern bool GenerateConsoleCtrlEvent(uint e, uint p);
    public static void SendCtrlC(uint p) {
        FreeConsole();
        if (AttachConsole(p)) {
            GenerateConsoleCtrlEvent(0, p);
            FreeConsole();
        }
        AttachConsole(uint.MaxValue);
    }'
Add-Type -Name 'dummyName' -Namespace 'dummyNamespace' -MemberDefinition $MemberDefinition
[dummyNamespace.dummyName]::SendCtrlC($ProcessID) }

The above would be impossible, and replacing GenerateConsoleCtrlEvent(0, p); with GenerateConsoleCtrlEvent(0, 0); sends the control event to both consoles.

[eryksun]

There's another bug in ApiDispatchers::ServerGenerateConsoleCtrlEvent(). The code that implements the undocumented (and mistaken) behavior of checking the parent process includes the following statement:

RETURN_IF_NTSTATUS_FAILED((NtPrivApi::s_GetProcessParentId(&ProcessId)));

If the internal NtOpenProcess() call fails with STATUS_INVALID_CID (0xC000_000B, invalid client ID), it gets naively mapped to 0xC007000B. On the client side, RtlNtStatusToDosError() unwraps this as the Windows error code ERROR_BAD_FORMAT (0x000B). For example:

>>> _winapi.GenerateConsoleCtrlEvent(0, 123456)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 115, in GenerateConsoleCtrlEvent
OSError: [WinError 11] An attempt was made to load a program with an incorrect format.

Removing the mistaken code that violates the 30-year-old documentation of GenerateConsoleCtrlEvent() would eliminate this error-code bug. That said, there's a more general issue to be concerned about here.

---

The code mistakenly returns a raw NTSTATUS code from a function that normally returns HRESULT values such as E_INVALIDARG. In this case, one is supposed to set the N bit (i.e. FACILITY_NT_BIT) to indicate that an NTSTATUS value is returned as an HRESULT. This should be fixed everywhere that it occurs.

To make matters worse, code in ApiSorter::ConsoleDispatchRequest() does something that's either extremely optimistic or extremely naive. It uses the macro NTSTATUS_FROM_HRESULT() under the assumption that the value is a Windows API facility error code (i.e. 0x8007xxxx). HRESULT defines dozens of facility codes, which generally have little to do with NTSTATUS facility codes since they're intended for different domains (i.e.NTSTATUS is for use by syscall/LPC interfaces and NT applications, while HRESULT is for use by COM/RPC interfaces and Windows applications). They happen to share the Windows API facility code 7 (i.e. FACILITY_WIN32), for which using NTSTATUS_FROM_HRESULT() generally makes sense.

The code in ApiSorter::ConsoleDispatchRequest() should handle status values for which the N bit is set by simply clearing the N bit in the status code value before returning to the client. If HRESULT_FACILITY(Status) == FACILITY_WIN32, it can use something like NTSTATUS_FROM_HRESULT(), but it should preserve the severity (i.e. 0x0 -> 0x0 for success, 0x8 -> 0xC for error). Any HRESULT value that's not in the Windows API facility has to be manually mapped to an NTSTATUS code that makes sense and converts sensibly on the client side via RtlNtStatusToDosError().

Also, the status value should be initialized as NTSTATUS Status = STATUS_SUCCESS, not set to the HRESULT value S_OK. They're both 0, cast to either NTSTATUS or HRESULT. But still, it's good discipline to avoid cavalierly mixing domains, such as NTSTATUS Status = S_OK.