[PATCH 0/1] MR3022: kernelbase: In AllocConsole(), dispose existing unix console.

[eric pouech (＠epo)]

Unix consoles (created from initial process) are inherited by child
processes for various reasons, like keeping std handles bound to that
unix console.
This differs from Windows as the default for a GUI is not to have
console attached.

If a GUI programs asks for a console, it will succeed on Windows
but fail under Wine as the Unix console is still present.

So, allow AllocConsole() to succeed when called from a GUI program
and tied to a Unix console.
(don't do it for CUI as they are already attached to a console).

This fixes Scrap Mechanic when run with '-dev' option.
(based on suggestion from Zhiyi Zhang)

Signed-off-by: Eric Pouech

[PATCH 1/1] kernelbase: In AllocConsole(), dispose existing unix console.

[Eric Pouech]

From: Eric Pouech

Unix consoles (created from initial process) are inherited by child
processes for various reasons, like keeping std handles bound to that
unix console.
This differs from Windows as the default for a GUI is not to have
console attached.

If a GUI programs asks for a console, it will succeed on Windows
but fail under Wine as the Unix console is still present.

So, allow AllocConsole() to succeed when called from a GUI program
and tied to a Unix console.
(don't do it for CUI as they are already attached to a console).

This fixes Scrap Mechanic when run with '-dev' option.
(based on suggestion from Zhiyi Zhang)

Signed-off-by: Eric Pouech 
---
 dlls/kernelbase/console.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/dlls/kernelbase/console.c b/dlls/kernelbase/console.c
index 831e358c692..787d87b937b 100644
--- a/dlls/kernelbase/console.c
+++ b/dlls/kernelbase/console.c
@@ -476,6 +476,15 @@ error:
  */
 BOOL WINAPI AllocConsole(void)
 {
+    RTL_USER_PROCESS_PARAMETERS *params = RtlGetCurrentPeb()->ProcessParameters;
+
+    /* allow gui applications to create a genuine console over a unix one */
+    if (RtlImageNtHeader( GetModuleHandleW( NULL ) )->OptionalHeader.Subsystem == IMAGE_SUBSYSTEM_WINDOWS_GUI &&
+        (params->ConsoleHandle == CONSOLE_HANDLE_SHELL_NO_WINDOW ||
+         console_ioctl( params->ConsoleHandle, IOCTL_CONDRV_IS_UNIX, NULL, 0, NULL, 0, NULL )))
+    {
+        FreeConsole();
+    }
     return alloc_console( FALSE );
 }

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/kernelbase/console.c:
> */
>  BOOL WINAPI AllocConsole(void)
>  {
> - RTL_USER_PROCESS_PARAMETERS *params = RtlGetCurrentPeb()->ProcessParameters;
> -
> - /* allow gui applications to create a genuine console over a unix one */
> - if (RtlImageNtHeader( GetModuleHandleW( NULL ) )->OptionalHeader.Subsystem == IMAGE_SUBSYSTEM_WINDOWS_GUI &&
> -
>    (params->ConsoleHandle == CONSOLE_HANDLE_SHELL_NO_WINDOW ||
> -
>     console_ioctl( params->ConsoleHandle, IOCTL_CONDRV_IS_UNIX, NULL, 0, NULL, 0, NULL )))

I'm afraid of breaking a use case where an application calls `AllocConsole` just to make sure there is a console, but it expects the call to fail if it's already attached to a console. Would limiting it to `CONSOLE_HANDLE_SHELL_NO_WINDOW` be enough for the problematic case?

[Zhiyi Zhang (＠zhiyi)]

On Thu Jun  8 14:23:36 2023 +0000, Jacek Caban wrote:
> I'm afraid of breaking a use case where an application calls
> `AllocConsole` just to make sure there is a console, but it expects the
> call to fail if it's already attached to a console. Would limiting it to
> `CONSOLE_HANDLE_SHELL_NO_WINDOW` be enough for the problematic case?

CONSOLE_HANDLE_SHELL_NO_WINDOW is enough for the game. For example, see https://gitlab.winehq.org/wine/wine/-/merge_requests/2986

[eric pouech (＠epo)]

On Thu Jun  8 14:40:57 2023 +0000, Zhiyi Zhang wrote:
> CONSOLE_HANDLE_SHELL_NO_WINDOW is enough for the game. For example, see https://gitlab.winehq.org/wine/wine/-/merge_requests/2986

@Jacek: no it doesn't suffice to restrict to shell_no_window. 
The game fails also when the launcher is run attached to the unix console ( == conhost --unix)

AFAICS under windows, a GUI doesn't inherit its parent process' console (whatever the createprocess flags) and its process param's ConsoleHandle is always to NULL (cf kernel32/tests/console.c test_CreateProcessCUI)

we do inherit in wine the console handle for keeping the link to unix console (or its absence for shell_no_window).

AllocConsole will fail if already attached to a "genuine" console, but will fake that the unix console (conhost --unix or shell_no_window) shouldn't have been inherited

[Jacek Caban (＠jacek)]

On Thu Jun  8 15:12:44 2023 +0000, eric pouech wrote:
> @Jacek: no it doesn't suffice to restrict to shell_no_window. 
> The game fails also when the launcher is run attached to the unix
> console ( == conhost --unix)
> AFAICS under windows, a GUI doesn't inherit its parent process' console
> (whatever the createprocess flags) and its process param's ConsoleHandle
> is always to NULL (cf kernel32/tests/console.c test_CreateProcessCUI)
> we do inherit in wine the console handle for keeping the link to unix
> console (or its absence for shell_no_window).
> AllocConsole will fail if already attached to a "genuine" console, but
> will fake that the unix console (conhost --unix or shell_no_window)
> shouldn't have been inherited

Maybe we can revisit inheriting console by GUI applications. Regardless of Unix console handling, it seems that GUI process should not be attached to parent's console. How about the attached [patch](/uploads/49af7084b2f9f92c244e175b883e6b03/patch.diff)?

While I think that taking shell-no-windows into account in `AttachConsole`, like MR2986, makes sense on its own, I think that we should generally treat proper Unix console just like proper Windows consoles when possible.

[eric pouech (＠epo)]

On Mon Jun 12 15:54:02 2023 +0000, Jacek Caban wrote:
> Maybe we can revisit inheriting console by GUI applications. Regardless
> of Unix console handling, it seems that GUI process should not be
> attached to parent's console. How about the attached [patch](/uploads/49af7084b2f9f92c244e175b883e6b03/patch.diff)?
> While I think that taking shell-no-windows into account in
> `AttachConsole`, like MR2986, makes sense on its own, I think that we
> should generally treat proper Unix console just like proper Windows
> consoles when possible.

I didn't go that far as it seems to me that some apps use the std handles for logging and debug output (when the handles are present) (I discussed this offline with @gofman recently and it seems he had some concerns about this)

If we want to go the way of erasing the console for GUI processes as your patch proposes, I think we'd need more tests for checking inheritedhandles to parent's console without connecting console handle (is the console object inherited ? and are the std handles still usable? when passed in process info) (what your patch does is likely inherit console's object in server, but doesn't set it as current console in kernelbase ; so more tests required) (iow shall we close incoming ->ConsoleHandle? and/or std handles). I'll write some more tests for that and post the results in this MR.

I fully agree for considering unix console as genuine consoles for CUI programs. For GUI, is we erase the console the question is closed. If we don't, that's more an opened question.

[eric pouech (＠epo)]

On Mon Jun 12 16:39:46 2023 +0000, eric pouech wrote:
> I didn't go that far as it seems to me that some apps use the std
> handles for logging and debug output (when the handles are present) (I
> discussed this offline with @gofman recently and it seems he had some
> concerns about this)
> If we want to go the way of erasing the console for GUI processes as
> your patch proposes, I think we'd need more tests for checking
> inheritedhandles to parent's console without connecting console handle
> (is the console object inherited ? and are the std handles still usable?
> when passed in process info) (what your patch does is likely inherit
> console's object in server, but doesn't set it as current console in
> kernelbase ; so more tests required) (iow shall we close incoming
> ->ConsoleHandle? and/or std handles). I'll write some more tests for
> that and post the results in this MR.
> I fully agree for considering unix console as genuine consoles for CUI
> programs. For GUI, is we erase the console the question is closed. If we
> don't, that's more an opened question.

posted https://gitlab.winehq.org/wine/wine/-/merge_requests/3057 with extended tests

it shows:
- for GUI, the std handles open to console are not inherited
- but can be forced to be even if ConsoleHandle is NULL (didn't check if the handle was anyway opened in child process)

so your patch proposal looks ok on ConsoleHandle management side (even if it would be better to close the console handle before zero:ing it out), but fails to close & zero out std handles for consoles in gui subsystem

A) I'm not sure we need to go that far (and simply detaching from console, but keeping the std handles open should suffice - and it should keep the logging active as listed above)
B) if we want to be 100% compatible with native, needed semantics are the ones of DETACHED_PROCESS (so perhaps native does the checks on parent or kernel side and forces DETACHED_PROCESS for GUI children)

so for the time being, I would favor A over B.