[PATCH v3 0/5] Fix for BZ52048

[Eric Pouech]

As described in bugzilla ticket, when calling CreateProcess:
- with "default" console inheritance (no CREATE_NEW_CONSOLE
  nor DETACHED_PROCESS flags),
- when parent is not attached to a console
- and when the child's subsystem is CUI
, native behaves as if CREATE_NEW_CONSOLE was passed.

builtin doesn't...

this serie:
- adds a test case for showing the CUI/GUI discrepancies
- fixes kernelbase accordingly
- but also requires to fix all usage of CreateProcess in Wine Code
  that would now generate the creation of a console
  + one patch of wow64 redirection (only for CUI executables)
  + when looking to the other usage of CreateProcess, I only convinced
    myself to adapt one of them

V1 => V2:
- don't mess up with the number of patches in the serie
- no longer modify makedep for creating cui vs gui test executable, but
  copy current *_test.exe executable and adapt its subsystem to the need

V2 => V3:
- removed the hacks in wow64 redirect, and let programs/uninstaller and
  programs/wusa belong to the GUI subsystem instead
- force console creation for wine initial process when a) none of the unix 
  fd are attached to a TTY, b) the initial process is in CUI subsystem

Note to Jacek:
- I still think we may have side effects of this serie. a) with spurious
  window creation (eg 'wine ping >& out < /dev/null' will generate one,
  which could be typical in shell scripting), b) will prevent program to
  run on headless setup (while they used to)
- so I put the change for initial process in a separate patch
- I'm not convinced at all that the evolution to the initial process is
  a good choice (if caller isn't attached to a Unix console, it's a bit
  awkward to end up with creating a graphical console). Furthermore, 
  wineconsole provides thes ability to create a console when needed.

And Marvin will generate errors for Chinese and Japanese VM in console
testing that are independant of this serie.

A+
---

Eric Pouech (5):
      dlls/kernel32/tests: add some console tests about creating cui vs gui processes
      programs/wusa: set subsystem to GUI
      programs/uninstaller: set subsystem to GUI
      dlls/kernelbase: handle corner case in CreateProcess
      dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

dlls/kernel32/tests/console.c    | 85 ++++++++++++++++++++++++++++++++
 dlls/kernelbase/process.c        |  6 ++-
 dlls/ntdll/unix/env.c            |  2 +
 programs/services/services.c     |  2 +-
 programs/uninstaller/Makefile.in |  2 +-
 programs/wusa/Makefile.in        |  2 +-
 6 files changed, 95 insertions(+), 4 deletions(-)

[PATCH v3 1/5] dlls/kernel32/tests: add some console tests about creating cui vs gui processes

[Eric Pouech]

adding helper to copy argv[0] into an executable with CUI subsystem

Signed-off-by: Eric Pouech

---
 dlls/kernel32/tests/console.c |   85 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
index 5f5f0698040..e0cecbe9ec0 100644
--- a/dlls/kernel32/tests/console.c
+++ b/dlls/kernel32/tests/console.c
@@ -4662,6 +4662,85 @@ static void test_pseudo_console(void)
     pClosePseudoConsole(pseudo_console);
 }

+/* copy an executable, but changing its subsystem */
+static void copy_change_subsystem(const char* in, const char* out, DWORD subsyst)
+{
+    BOOL ret;
+    HANDLE hFile, hMap;
+    void* mapping;
+    IMAGE_NT_HEADERS *nthdr;
+
+    ret = CopyFileA(in, out, FALSE);
+    ok(ret, "Failed to copy executable %s in %s (%u)\n", in, out, GetLastError());
+
+    hFile = CreateFileA(out, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+                         OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+    ok(hFile != INVALID_HANDLE_VALUE, "Couldn't open file %s (%u)\n", out, GetLastError());
+    hMap = CreateFileMappingW(hFile, NULL, PAGE_READWRITE, 0, 0, NULL);
+    ok(hMap != NULL, "Couldn't create map (%u)\n", GetLastError());
+    mapping = MapViewOfFile(hMap, FILE_MAP_ALL_ACCESS, 0, 0, 0);
+    ok(mapping != NULL, "Couldn't map (%u)\n", GetLastError());
+    nthdr = RtlImageNtHeader(mapping);
+    ok(nthdr != NULL, "Cannot get NT headers out of %s\n", out);
+    if (nthdr) nthdr->OptionalHeader.Subsystem = subsyst;
+    ret = UnmapViewOfFile(mapping);
+    ok(ret, "Couldn't unmap (%u)\n", GetLastError());
+    CloseHandle(hMap);
+    CloseHandle(hFile);
+}
+
+static BOOL check_whether_child_attached(const char* exec, DWORD flags)
+{
+    STARTUPINFOA si = { sizeof(si) };
+    PROCESS_INFORMATION info;
+    char buf[MAX_PATH];
+    DWORD exit_code;
+    BOOL res;
+    DWORD ret;
+
+    sprintf(buf, ""%s" console check_console", exec);
+    res = CreateProcessA(NULL, buf, NULL, NULL, FALSE, flags, NULL, NULL, &si, &info);
+    ok(res, "CreateProcess failed: %u %s\n", GetLastError(), buf);
+    CloseHandle(info.hThread);
+    ret = WaitForSingleObject(info.hProcess, 30000);
+    ok(ret == WAIT_OBJECT_0, "Could not wait for the child process: %d le=%u\n",
+        (UINT)ret, (UINT)GetLastError());
+    ret = GetExitCodeProcess(info.hProcess, &exit_code);
+    ok(ret && exit_code <= 255, "Couldn't get exit_code\n");
+    CloseHandle(info.hProcess);
+    return exit_code != 0;
+}
+
+static void test_CreateProcessCUI(void)
+{
+    char guiexec[MAX_PATH];
+    char cuiexec[MAX_PATH];
+    char **argv;
+    BOOL res;
+
+    winetest_get_mainargs(&argv);
+    GetTempPathA(ARRAY_SIZE(guiexec), guiexec);
+    strcat(guiexec, "console_gui.exe");
+    copy_change_subsystem(argv[0], guiexec, IMAGE_SUBSYSTEM_WINDOWS_GUI);
+    GetTempPathA(ARRAY_SIZE(cuiexec), cuiexec);
+    strcat(cuiexec, "console_cui.exe");
+    copy_change_subsystem(argv[0], cuiexec, IMAGE_SUBSYSTEM_WINDOWS_CUI);
+
+    FreeConsole();
+
+    res = check_whether_child_attached(guiexec, DETACHED_PROCESS);
+    ok(!res, "Don't expecting child to be attached to a console\n");
+    res = check_whether_child_attached(guiexec, 0);
+    ok(!res, "Don't expecting child to be attached to a console\n");
+    res = check_whether_child_attached(cuiexec, DETACHED_PROCESS);
+    ok(!res, "Don't expecting child to be attached to a console\n");
+    res = check_whether_child_attached(cuiexec, 0);
+    todo_wine ok(res, "Expecting child to be attached to a console\n");
+
+    DeleteFileA(guiexec);
+    DeleteFileA(cuiexec);
+}
+
 START_TEST(console)
 {
     HANDLE hConIn, hConOut, revert_output = NULL, unbound_output;
@@ -4690,6 +4769,11 @@ START_TEST(console)
         return;
     }

+    if (argc == 3 && !strcmp(argv[2], "check_console"))
+    {
+        ExitProcess(GetConsoleCP() != 0);
+    }
+
     test_current = argc >= 3 && !strcmp(argv[2], "--current");
     using_pseudo_console = argc >= 3 && !strcmp(argv[2], "--pseudo-console");

@@ -4880,6 +4964,7 @@ START_TEST(console)
         test_AttachConsole(hConOut);
         test_AllocConsole();
         test_FreeConsole();
+        test_CreateProcessCUI();
     }
     else if (revert_output) SetConsoleActiveScreenBuffer(revert_output);

[PATCH v3 1/5] dlls/kernel32/tests: add some console tests about creating cui vs gui processes

[Jacek Caban]

Hi Eric,

On 2/17/22 10:10, Eric Pouech wrote:
> - sprintf(buf, ""%s" console check_console", exec);
> - res = CreateProcessA(NULL, buf, NULL, NULL, FALSE, flags, NULL, NULL, &si, &info);
> - ok(res, "CreateProcess failed: %u %s\n", GetLastError(), buf);
> - CloseHandle(info.hThread);
> - ret = WaitForSingleObject(info.hProcess, 30000);
> - ok(ret == WAIT_OBJECT_0, "Could not wait for the child process: %d le=%u\n",
> -
>    (UINT)ret, (UINT)GetLastError());

Those casts are not needed.

Thanks,

Jacek

[PATCH v3 2/5] programs/wusa: set subsystem to GUI

[Eric Pouech]

as it's on native

Signed-off-by: Eric Pouech

---
 programs/wusa/Makefile.in |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/programs/wusa/Makefile.in b/programs/wusa/Makefile.in
index 171377bc2f2..c512d3e63db 100644
--- a/programs/wusa/Makefile.in
+++ b/programs/wusa/Makefile.in
@@ -1,7 +1,7 @@
 MODULE    = wusa.exe
 IMPORTS   = cabinet shlwapi ole32 oleaut32 advapi32

-EXTRADLLFLAGS = -mconsole -municode
+EXTRADLLFLAGS = -mwindows -municode

C_SRCS = \
    main.c \

[PATCH v3 2/5] programs/wusa: set subsystem to GUI

[Jacek Caban]

Signed-off-by: Jacek Caban

[PATCH v3 3/5] programs/uninstaller: set subsystem to GUI

[Eric Pouech]

Signed-off-by: Eric Pouech

---
 programs/uninstaller/Makefile.in |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/programs/uninstaller/Makefile.in b/programs/uninstaller/Makefile.in
index f7fef51a51c..5cc35b19dc3 100644
--- a/programs/uninstaller/Makefile.in
+++ b/programs/uninstaller/Makefile.in
@@ -2,7 +2,7 @@ MODULE    = uninstaller.exe
 IMPORTS   = advapi32
 DELAYIMPORTS = shlwapi shell32 user32 gdi32 comctl32

-EXTRADLLFLAGS = -mconsole -municode
+EXTRADLLFLAGS = -mwindows -municode

C_SRCS = \
    main.c

[PATCH v3 3/5] programs/uninstaller: set subsystem to GUI

[Jacek Caban]

Signed-off-by: Jacek Caban

[PATCH v3 4/5] dlls/kernelbase: handle corner case in CreateProcess

[Eric Pouech]

in CreateProcess, if:
- parent isn't attached to a console
- CreateProcess's flag isn't set with DETACHED_PROCESS nor
  CREATE_NEW_CONSOLE
- child is a CUI program
then a console must be allocated for the child

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52048
Signed-off-by: Eric Pouech

---
 dlls/kernel32/tests/console.c |    2 +-
 dlls/kernelbase/process.c     |    6 +++++-
 programs/services/services.c  |    2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
index e0cecbe9ec0..9f27bd622a0 100644
--- a/dlls/kernel32/tests/console.c
+++ b/dlls/kernel32/tests/console.c
@@ -4735,7 +4735,7 @@ static void test_CreateProcessCUI(void)
     res = check_whether_child_attached(cuiexec, DETACHED_PROCESS);
     ok(!res, "Don't expecting child to be attached to a console\n");
     res = check_whether_child_attached(cuiexec, 0);
-    todo_wine ok(res, "Expecting child to be attached to a console\n");
+    ok(res, "Expecting child to be attached to a console\n");

DeleteFileA(guiexec);
     DeleteFileA(cuiexec);
diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c
index 35381f409e9..1cecbce9321 100644
--- a/dlls/kernelbase/process.c
+++ b/dlls/kernelbase/process.c
@@ -191,7 +191,11 @@ static RTL_USER_PROCESS_PARAMETERS *create_process_params( const WCHAR *filename

if (flags & CREATE_NEW_PROCESS_GROUP) params->ConsoleFlags = 1;
     if (flags & CREATE_NEW_CONSOLE) params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
-    else if (!(flags & DETACHED_PROCESS)) params->ConsoleHandle = NtCurrentTeb()->Peb->ProcessParameters->ConsoleHandle;
+    else if (!(flags & DETACHED_PROCESS))
+    {
+        params->ConsoleHandle = NtCurrentTeb()->Peb->ProcessParameters->ConsoleHandle;
+        if (!params->ConsoleHandle) params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
+    }

if (startup->dwFlags & STARTF_USESTDHANDLES)
     {
diff --git a/programs/services/services.c b/programs/services/services.c
index 87ab319367b..2edc02d300c 100644
--- a/programs/services/services.c
+++ b/programs/services/services.c
@@ -1094,7 +1094,7 @@ found:
     process->use_count++;
     service_unlock(service_entry);

-    r = CreateProcessW(NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT, environment, NULL, &si, &pi);
+    r = CreateProcessW(NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS, environment, NULL, &si, &pi);
     HeapFree(GetProcessHeap(), 0, path);
     if (!r)
     {

[PATCH v3 4/5] dlls/kernelbase: handle corner case in CreateProcess

[Jacek Caban]

Hi Eric,

On 2/17/22 10:10, Eric Pouech wrote:
> diff --git a/programs/services/services.c b/programs/services/services.c
> index 87ab319367b..2edc02d300c 100644
> --- a/programs/services/services.c
> +++ b/programs/services/services.c
> @@ -1094,7 +1094,7 @@ found:
>       process->use_count++;
>       service_unlock(service_entry);
> - r = CreateProcessW(NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT, environment, NULL, &si, &pi);
> - r = CreateProcessW(NULL, path, NULL, NULL, FALSE, CREATE_UNICODE_ENVIRONMENT | DETACHED_PROCESS, environment, NULL, &si, &pi);
>  HeapFree(GetProcessHeap(), 0, path);
>  if (!r)
>  {

This probably deserves a separated patch.

Thanks,

Jacek

[PATCH v3 5/5] dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

[Eric Pouech]

suggested by Jacek Caban

Signed-off-by: Eric Pouech

---
 dlls/ntdll/unix/env.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c
index c7e0674e083..a69d0f26e87 100644
--- a/dlls/ntdll/unix/env.c
+++ b/dlls/ntdll/unix/env.c
@@ -1657,6 +1657,8 @@ static void get_initial_console( RTL_USER_PROCESS_PARAMETERS *params )
     wine_server_fd_to_handle( 1, GENERIC_WRITE|SYNCHRONIZE, OBJ_INHERIT, &params->hStdOutput );
     wine_server_fd_to_handle( 2, GENERIC_WRITE|SYNCHRONIZE, OBJ_INHERIT, &params->hStdError );

+    params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
+
     /* mark tty handles for kernelbase, see init_console */
     if (params->hStdInput && isatty(0))
     {

[PATCH v3 5/5] dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

[Jacek Caban]

Hi Eric,

On 2/17/22 10:10, Eric Pouech wrote:
> suggested by Jacek Caban
>
> Signed-off-by: Eric Pouech
>
> dlls/ntdll/unix/env.c |    2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c
> index c7e0674e083..a69d0f26e87 100644
> --- a/dlls/ntdll/unix/env.c
> +++ b/dlls/ntdll/unix/env.c
> @@ -1657,6 +1657,8 @@ static void get_initial_console( RTL_USER_PROCESS_PARAMETERS *params )
>       wine_server_fd_to_handle( 1, GENERIC_WRITE|SYNCHRONIZE, OBJ_INHERIT, &params->hStdOutput );
>       wine_server_fd_to_handle( 2, GENERIC_WRITE|SYNCHRONIZE, OBJ_INHERIT, &params->hStdError );
> - params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
> -
> /* mark tty handles for kernelbase, see init_console */
> if (params->hStdInput && isatty(0))
> {

Looking more at this, I'm not sure if it's a good idea, at least in its 
current form. There are situations where user would not want the console 
to be created. For example, user may have a Unix script that runs some 
non-interactive Windows commands with Wine, which is not attached to any 
console. I guess that some heuristic like if (!params->hStdInput && 
!params->hStdOutput) should mostly catch such cases, but it's still not 
perfect. I guess users could still use something like "start /b" to be 
explicit, but it's not obvious if it's worth it.

BTW, I noticed that start.exe CREATE_NEW_CONSOLE handling will not be 
right with your patches. "/b" option should probably use DETACHED_PROCESS.

Thanks,

Jacek

[PATCH v3 5/5] dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

[Eric Pouech]

Le 20/02/2022 à 13:20, Jacek Caban a écrit :
> Hi Eric,
>
> On 2/17/22 10:10, Eric Pouech wrote:
> > suggested by Jacek Caban
> >
> > Signed-off-by: Eric Pouech
> >
> > dlls/ntdll/unix/env.c |    2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c
> > index c7e0674e083..a69d0f26e87 100644
> > --- a/dlls/ntdll/unix/env.c
> > +++ b/dlls/ntdll/unix/env.c
> > @@ -1657,6 +1657,8 @@ static void get_initial_console( 
> > RTL_USER_PROCESS_PARAMETERS *params )
> >       wine_server_fd_to_handle( 1, GENERIC_WRITE|SYNCHRONIZE, 
> > OBJ_INHERIT, &params->hStdOutput );
> >       wine_server_fd_to_handle( 2, GENERIC_WRITE|SYNCHRONIZE, 
> > OBJ_INHERIT, &params->hStdError );
> >   +    params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
> > -
> >
> > /* mark tty handles for kernelbase, see init_console */
> >       if (params->hStdInput && isatty(0))
> >       {
>
> Looking more at this, I'm not sure if it's a good idea, at least in 
> its current form. There are situations where user would not want the 
> console to be created. For example, user may have a Unix script that 
> runs some non-interactive Windows commands with Wine, which is not 
> attached to any console. I guess that some heuristic like if 
> (!params->hStdInput && !params->hStdOutput) should mostly catch such 
> cases, but it's still not perfect. I guess users could still use 
> something like "start /b" to be explicit, but it's not obvious if it's 
> worth it.

agreed (may be you skipped the bottom note in cover mail [PATCH v3 0/5] 
.... https://www.winehq.org/pipermail/wine-devel/2022-February/208328.html)
> BTW, I noticed that start.exe CREATE_NEW_CONSOLE handling will not be 
> right with your patches. "/b" option should probably use 
> DETACHED_PROCESS.

yes. and likely the CREATE_NEW_CONSOLE init is not needed either.

I'll repost

thanks for the review
> Thanks,
>
> Jacek

[PATCH v3 5/5] dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

[Jacek Caban]

On 2/20/22 15:54, Eric Pouech wrote:
> Le 20/02/2022 à 13:20, Jacek Caban a écrit :
> > Hi Eric,
> >
> > On 2/17/22 10:10, Eric Pouech wrote:
> > > suggested by Jacek Caban
> > >
> > > Signed-off-by: Eric Pouech
> > >
> > > dlls/ntdll/unix/env.c |    2 ++
> > >   1 file changed, 2 insertions(+)
> > >
> > > diff --git a/dlls/ntdll/unix/env.c b/dlls/ntdll/unix/env.c
> > > index c7e0674e083..a69d0f26e87 100644
> > > --- a/dlls/ntdll/unix/env.c
> > > +++ b/dlls/ntdll/unix/env.c
> > > @@ -1657,6 +1657,8 @@ static void get_initial_console( 
> > > RTL_USER_PROCESS_PARAMETERS *params )
> > >       wine_server_fd_to_handle( 1, GENERIC_WRITE|SYNCHRONIZE, 
> > > OBJ_INHERIT, &params->hStdOutput );
> > >       wine_server_fd_to_handle( 2, GENERIC_WRITE|SYNCHRONIZE, 
> > > OBJ_INHERIT, &params->hStdError );
> > >   +    params->ConsoleHandle = CONSOLE_HANDLE_ALLOC;
> > > -
> > >
> > > /* mark tty handles for kernelbase, see init_console */
> > >       if (params->hStdInput && isatty(0))
> > >       {
> >
> > Looking more at this, I'm not sure if it's a good idea, at least in 
> > its current form. There are situations where user would not want the 
> > console to be created. For example, user may have a Unix script that 
> > runs some non-interactive Windows commands with Wine, which is not 
> > attached to any console. I guess that some heuristic like if 
> > (!params->hStdInput && !params->hStdOutput) should mostly catch such 
> > cases, but it's still not perfect. I guess users could still use 
> > something like "start /b" to be explicit, but it's not obvious if 
> > it's worth it.
>
> agreed (may be you skipped the bottom note in cover mail [PATCH v3 
> 0/5] .... 
> https://www.winehq.org/pipermail/wine-devel/2022-February/208328.html)

Yes, I missed that, thanks. Let's skip it then.

Jacek

[PATCH v3 5/5] dlls/ntdll: allocate a console for initial CUI process when not attached to a Unix console

[Eric Pouech]

> > BTW, I noticed that start.exe CREATE_NEW_CONSOLE handling will not be 
> > right with your patches. "/b" option should probably use 
> > DETACHED_PROCESS.

that's not what testing suggests (from a W10 cmd prompt). I'll skip this 
one.

Z:>start /b tasklist

Z:>
Image Name                     PID Session Name        Session# Mem Usage
========================= ======== ================ =========== ============
System Idle Process              0 Services 0          8 K
System                           4 Services 0         20 K
Registry                       108 Services 0     29 472 K
smss.exe                       388 Services 0        156 K
csrss.exe                      480 Services 0      1 332 K
...