[PATCH v2 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

--
  v2: kernelbase: Only inherit consoles for CUI processes.

https://gitlab.winehq.org/wine/wine/-/merge_requests/3022

[PATCH v2 1/1] kernelbase: Only inherit consoles for CUI processes.

[Jacek Caban]

From: Jacek Caban

Signed-off-by: Eric Pouech 
---
 dlls/kernel32/tests/console.c |  2 +-
 dlls/kernelbase/console.c     | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
index 24c1e5c69fd..4e164c11b3d 100644
--- a/dlls/kernel32/tests/console.c
+++ b/dlls/kernel32/tests/console.c
@@ -4951,7 +4951,7 @@ static void test_CreateProcessCUI(void)
     },
     with_console_tests[] =
     {
-/* 0*/  {FALSE, 0,                                     NULL_STD,        0, TRUE},
+/* 0*/  {FALSE, 0,                                     NULL_STD,        0},
         {FALSE, DETACHED_PROCESS,                      NULL_STD,        0},
         {FALSE, CREATE_NEW_CONSOLE,                    NULL_STD,        0},
         {FALSE, CREATE_NO_WINDOW,                      NULL_STD,        0},
diff --git a/dlls/kernelbase/console.c b/dlls/kernelbase/console.c
index 831e358c692..7745ee7ee14 100644
--- a/dlls/kernelbase/console.c
+++ b/dlls/kernelbase/console.c
@@ -2344,14 +2344,19 @@ void init_console( void )
             init_console_std_handles( FALSE );
         }
     }
+    else if (RtlImageNtHeader( GetModuleHandleW( NULL ))->OptionalHeader.Subsystem !=
+             IMAGE_SUBSYSTEM_WINDOWS_CUI)
+    {
+        /* don't try to close pseudo handles */
+        if ((LONG_PTR)params->ConsoleHandle > 0) CloseHandle(params->ConsoleHandle);
+        params->ConsoleHandle = NULL;
+    }
     else if (params->ConsoleHandle == CONSOLE_HANDLE_ALLOC ||
              params->ConsoleHandle == CONSOLE_HANDLE_ALLOC_NO_WINDOW)
     {
         BOOL no_window = params->ConsoleHandle == CONSOLE_HANDLE_ALLOC_NO_WINDOW;
-        HMODULE mod = GetModuleHandleW( NULL );
         params->ConsoleHandle = NULL;
-        if (RtlImageNtHeader( mod )->OptionalHeader.Subsystem == IMAGE_SUBSYSTEM_WINDOWS_CUI)
-            alloc_console( no_window );
+        alloc_console( no_window );
     }
     else if (params->ConsoleHandle && params->ConsoleHandle != CONSOLE_HANDLE_SHELL_NO_WINDOW)
         create_console_connection( params->ConsoleHandle );

[eric pouech (＠epo)]

amended Jacek's proposal:
- close passed console handle
- adapt tests

[Jacek Caban (＠jacek)]

Looking closer at this, maybe we should just do that in the parent process. What do you think about something like this?
https://gitlab.winehq.org/jacek/wine/-/commit/02faf858eb13d0110cd45cc906614d...

[eric pouech (＠epo)]

On Wed Jun 21 12:09:40 2023 +0000, Jacek Caban wrote:
> Looking closer at this, maybe we should just do that in the parent
> process. What do you think about something like this?
> https://gitlab.winehq.org/jacek/wine/-/commit/02faf858eb13d0110cd45cc906614d...

yes for moving it into the parent!

but I wonder if this shouldn't rather be done in kernelbase (instead of ntdll)
- adding also the creation of the ad hoc console handles when requested from CreateProces flags, and simply having ntdll pass the console handle around (this would also get rid of the pseudo consoles CONSOLE_ALLOC*)
- this would look more symmetrical (everything in kernelbase) and ntdll transparent to it (except for unix console inheritance of course)

[Jacek Caban (＠jacek)]

On Wed Jun 21 12:09:39 2023 +0000, eric pouech wrote:
> yes for moving it into the parent!
> but I wonder if this shouldn't rather be done in kernelbase (instead of ntdll)
> - adding also the creation of the ad hoc console handles when requested
>
> from CreateProces flags, and simply having ntdll pass the console handle
> around (this would also get rid of the pseudo consoles CONSOLE_ALLOC*)
> - this would look more symmetrical (everything in kernelbase) and ntdll
>
> transparent to it (except for unix console inheritance of course)

We don't know if the child will be CUI until its executable is mapped and that's done in ntdll.

[eric pouech (＠epo)]

On Wed Jun 21 12:55:05 2023 +0000, Jacek Caban wrote:
> We don't know if the child will be CUI until its executable is mapped
> and that's done in ntdll.

what's next step? you post your patch and I close this MR?

[Jacek Caban (＠jacek)]

On Thu Jun 22 15:13:44 2023 +0000, eric pouech wrote:
> what's next step? you post your patch and I close this MR?

Sounds good to me, I created !3145.

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

[eric pouech (＠epo)]

This merge request was closed by eric pouech.

[eric pouech (＠epo)]

closing superseded by MR!3145