Re: MPS: Win64 testers?

[Eli Zaretskii]

> From: Quang Kien Nguyen <kien.n.quang@gmail.com>
> Date: Tue, 6 Aug 2024 13:52:27 -0700
> Cc: emacs-devel@gnu.org, pipcet@protonmail.com
> 
> > I'm not sure I understand what you are asking.  Are you asking how to
> > test that the standard handles aren't inherited by subprocesses?
> 
> I can check that the SetHandleInformation is returning TRUE so the handles
> shouldn't be inherited already.  However, is there any Emacs feature
> that will be
> broken if the handles are inherited?  I would like to test that to make sure 
> the
> E2E flow is still working.

This is not about some Emacs feature, or at least not only about that.
This is about having Emacs on Windows behave the same as Emacs on
Posix systems, wrt inheriting handles to child sub-processes.  We want
this to be done so that anyone who writes a Lisp program that launches
sub-processes could be certain they will behave the same wrt
inheritance of standard handles, on all supported systems.

To test that, one would need to demonstrate that the parent Emacs
process can do something with its standard handles without affecting
the same handles of the child process.  For example, moving the file
pointer in one process doesn't move it in the other.  As another
example, if Emacs's standard output is redirected to a file, and
writing to stdout in the child process writes to that same file, then
the standard output handle _was_ inherited.  Yet another possibility
is to use a tool such as ProcessExplorer to show the target of each
handle, in both parent Emacs and its child sub-process, and verify the
target is different.

> > Btw, I still don't think I've seen the patch you are want to test, so
> > I don't even know if it's clean and correct by itself.
> 
> The patch is here
> https://raw.githubusercontent.com/kiennq/emacs-build/main/patches/0004-init_ntproc-Use-SetHandleInformation-to-set-NOINHERI.patch
> It's also attached in the mail.

Thanks, see some comments below.

> --- a/src/w32.c
> +++ b/src/w32.c
> @@ -7783,10 +7783,6 @@ init_winsock (int load_now)
>    if (winsock_lib != NULL)
>      return TRUE;
>  
> -  pfn_SetHandleInformation
> -    = (void *) get_proc_addr (GetModuleHandle ("kernel32.dll"),
> -                                     "SetHandleInformation");
> -
>    winsock_lib = LoadLibrary ("Ws2_32.dll");
>  
>    if (winsock_lib != NULL)
> @@ -10471,6 +10467,10 @@ init_ntproc (int dumping)
>       Conveniently, init_environment is called before us, so
>       PRELOAD_WINSOCK can be set in the registry. */
>  
> +  pfn_SetHandleInformation
> +    = (void *) get_proc_addr (GetModuleHandle ("kernel32.dll"),
> +                                     "SetHandleInformation");
> +

This hunk is not needed, for the reasons explained below.

> @@ -10480,60 +10480,73 @@ init_ntproc (int dumping)
>    /* Initial preparation for subprocess support: replace our standard
>       handles with non-inheritable versions. */
>    {
> -    HANDLE parent;
> -    HANDLE stdin_save =  INVALID_HANDLE_VALUE;
> -    HANDLE stdout_save = INVALID_HANDLE_VALUE;
> -    HANDLE stderr_save = INVALID_HANDLE_VALUE;
> -
> -    parent = GetCurrentProcess ();
> -
> -    /* ignore errors when duplicating and closing; typically the
> -       handles will be invalid when running as a gui program. */
> -    DuplicateHandle (parent,
> -                  GetStdHandle (STD_INPUT_HANDLE),
> -                  parent,
> -                  &stdin_save,
> -                  0,
> -                  FALSE,
> -                  DUPLICATE_SAME_ACCESS);
> -
> -    DuplicateHandle (parent,
> -                  GetStdHandle (STD_OUTPUT_HANDLE),
> -                  parent,
> -                  &stdout_save,
> -                  0,
> -                  FALSE,
> -                  DUPLICATE_SAME_ACCESS);
> -
> -    DuplicateHandle (parent,
> -                  GetStdHandle (STD_ERROR_HANDLE),
> -                  parent,
> -                  &stderr_save,
> -                  0,
> -                  FALSE,
> -                  DUPLICATE_SAME_ACCESS);
> -
> -    fclose (stdin);
> -    fclose (stdout);
> -    fclose (stderr);
> -
> -    if (stdin_save != INVALID_HANDLE_VALUE)
> -      _open_osfhandle ((intptr_t) stdin_save, O_TEXT);
> +    /* For UCRT, the _fdopen will try to find free stream from
> +       _IOB_ENTRIES (= 3), thus we can't reopen the standard handles
> +       with it. Using SetHandleInformation to make the handle not
> +       inheritable to child process is a better way. */

This is not what I think we need.  First, there's no need to change
anything in the MSVCRT build, because AFAIK the current code "just
works" in such a build, both in 32-bit builds and in 64-bit builds.
The problem happens only in UCRT builds.

Second, UCRT builds are only supported on versions of Windows where
SetHandleInformation is always available; on older Windows systems,
the UCRT build will refuse to run, because UCRT is not available.

So what I think is needed is a compile-time condition that makes the
UCRT build use SetHandleInformation instead of the
DuplicateHandle/fclose/_fdopen dance, and leaves the existing code
intact in non-UCRT builds.  I think "#ifdef _UCRT" is such a
condition, but I'm not 100% sure.