[PATCH v7 5/7] Cygwin: pty: Guard get_winpid_to_hand_over() with attach_mutex

[Johannes Schindelin]

Hi Takashi,

This patch is correct and minimal. I have two observations, one about the
commit message and one about the exec path.

On Wed, 25 Mar 2026, Takashi Yano wrote:

> Currently, attach_mutex is shared only in the same process. As a
> result, if the master process of pty attaches to pseudo console
> temporarily, get_winpid_to_hand_over() may wrongly find the master
> process to hand over the pseudo console. make attach_mutex shared
> within the PTY and guard get_winpid_to_hand_over() with it.

The commit message would benefit from a bit more context. As it stands, a
future reader has to guess _why_ the master temporarily attaches and _what_
makes `get_winpid_to_hand_over()` sensitive to that attachment.

Concretely: the master process (e.g. mintty) temporarily attaches to the
pseudo console's conhost in `transfer_input()` (the `to_cyg` path) so it
can read INPUT_RECORDs via `ReadConsoleInputA()`. During that brief window,
`get_console_process_id()` inside `get_winpid_to_hand_over()` calls
`GetConsoleProcessList()`, which now sees the master among the console's
attached processes and may select it as the handover target. That is wrong
because the master will detach immediately after the read.

Making `attach_mutex` a cross-process named mutex lets
`get_winpid_to_hand_over()` in the slave serialize with the master's
temporary attachment, so the `GetConsoleProcessList()` enumeration never
observes the master while it is temporarily attached.

I think spelling that out in the commit message (even briefly) would make
the patch much easier to revisit later.

Also, minor: "make attach_mutex shared" at the start of a new sentence
should be capitalized ("Make attach_mutex shared").

Something like this maybe?

    The master process (e.g. mintty) temporarily attaches to the pseudo
    console's conhost in `transfer_input()` so it can read
    INPUT_RECORDs via `ReadConsoleInputA()`. During that brief window,
    `get_console_process_id()` inside `get_winpid_to_hand_over()` calls
    `GetConsoleProcessList()`, which sees the master among the console's
    attached processes and may select it as the handover target. That is
    wrong because the master will detach immediately after the read.

    Until now, `attach_mutex` was a process-local unnamed mutex, so
    the slave's `get_winpid_to_hand_over()` could not serialize with
    the master's temporary attachment. Make `attach_mutex` a
    cross-process named mutex (`ATTACH_MUTEX`) shared within the PTY,
    and acquire it around the `get_console_process_id()` calls in
    `get_winpid_to_hand_over()`. This ensures the console process list
    enumeration never observes the master while it is temporarily
    attached.

> 
> Fixes: 1e6c51d74136 ("Cygwin: pty: Reorganize the code path of setting up and closing pcon.")
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> Reviewed-by:
> ---
>  winsup/cygwin/fhandler/pty.cc      | 14 ++++++++++++--
>  winsup/cygwin/local_includes/tty.h |  1 +
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
> index 2a0e0d2f7..0de6ec007 100644
> --- a/winsup/cygwin/fhandler/pty.cc
> +++ b/winsup/cygwin/fhandler/pty.cc
> @@ -774,6 +774,12 @@ fhandler_pty_slave::open (int flags, mode_t)
>        errmsg = "open pipe switch mutex failed, %E";
>        goto err;
>      }
> +  if (!(attach_mutex
> +	= get_ttyp ()->open_mutex (ATTACH_MUTEX, MAXIMUM_ALLOWED)))
> +    {
> +      errmsg = "open attach mutex failed, %E";
> +      goto err;
> +    }
>    shared_name (buf, INPUT_AVAILABLE_EVENT, get_minor ());
>    if (!(input_available_event = OpenEvent (MAXIMUM_ALLOWED, TRUE, buf)))
>      {
> @@ -2533,6 +2539,7 @@ void
>  fhandler_pty_slave::fixup_after_fork (HANDLE parent)
>  {
>    create_invisible_console ();
> +  attach_mutex = get_ttyp ()->open_mutex (ATTACH_MUTEX, MAXIMUM_ALLOWED);

The patch opens `attach_mutex` in `open()` and `fixup_after_fork()` but
not in `fixup_after_exec()`. Since `fixup_after_exec()` calls
`fixup_after_fork()`, the mutex _is_ reopened after exec as well, so this
is fine. The dependency is implicit, though. A one-line comment in
`fixup_after_fork()` noting that this also covers the exec path (or a note
in the commit message) would save the next reader a detour.

The code change itself is sound. The named mutex correctly serializes the
cross-process operation, and the critical section is kept tight around the
`get_console_process_id()` calls.

Thanks,
Johannes

>  
>    // fork_fixup (parent, inuse, "inuse");
>    // fhandler_pty_common::fixup_after_fork (parent);
> @@ -3164,8 +3171,9 @@ fhandler_pty_master::setup ()
>    if (!(pipe_sw_mutex = CreateMutex (&sa, FALSE, buf)))
>      goto err;
>  
> -  if (!attach_mutex)
> -    attach_mutex = CreateMutex (&sec_none_nih, FALSE, NULL);
> +  errstr = shared_name (buf, ATTACH_MUTEX, unit);
> +  if (!(attach_mutex = CreateMutex (&sa, FALSE, buf)))
> +    goto err;
>  
>    /* Create master control pipe which allows the master to duplicate
>       the pty pipe handles to processes which deserve it. */
> @@ -3725,6 +3733,7 @@ fhandler_pty_slave::get_winpid_to_hand_over (tty *ttyp,
>        DWORD current_pid = myself->exec_dwProcessId ?: myself->dwProcessId;
>        if (ttyp->nat_pipe_owner_pid == GetCurrentProcessId ())
>  	current_pid = GetCurrentProcessId ();
> +      acquire_attach_mutex (mutex_timeout);
>        switch_to = get_console_process_id (current_pid,
>  					  false, true, true, true);
>        if (!switch_to)
> @@ -3733,6 +3742,7 @@ fhandler_pty_slave::get_winpid_to_hand_over (tty *ttyp,
>        if (!switch_to && ttyp->pcon_activated)
>  	switch_to = get_console_process_id (current_pid,
>  					    false, false, false, false);
> +      release_attach_mutex ();
>      }
>    return switch_to;
>  }
> diff --git a/winsup/cygwin/local_includes/tty.h b/winsup/cygwin/local_includes/tty.h
> index cd1e202f1..962697782 100644
> --- a/winsup/cygwin/local_includes/tty.h
> +++ b/winsup/cygwin/local_includes/tty.h
> @@ -22,6 +22,7 @@ details. */
>  #define OUTPUT_MUTEX		"cygtty.output.mutex"
>  #define INPUT_MUTEX		"cygtty.input.mutex"
>  #define PIPE_SW_MUTEX		"cygtty.pipe_sw.mutex"
> +#define ATTACH_MUTEX		"cygtty.attach.mutex"
>  #define TTY_SLAVE_ALIVE		"cygtty.slave_alive"
>  #define TTY_SLAVE_READING	"cygtty.slave_reading"
>  
> -- 
> 2.51.0
> 
>