Re: [PATCH v3] Cygwin: pty: Do not transfer input to nat-pipe while masked

Hi Takashi,

Thank you for v3. Reverting the shared storage cleanly closes the
cross-process HANDLE hazard, and wrapping the CreateEvent/CloseHandle
block in acquire_attach_mutex/release_attach_mutex closes the
OpenEvent-based TOCTOU I had flagged on v2. One substantive concern
remains, around lock ordering.

On Mon, 6 Jul 2026, Takashi Yano wrote:

> On the command "cat | non-cygwin-app", `cat` sometimes fails to read
> key input. This happens when `cat` starts to read input before `non-
> cygwin-app` configures pseudo console. This is because pipe state is
> switched to nat-pipe when pseudo console is configured.
>=20
> This patch prevent the pipe state from changing to nat-pipe state if
> some cygwin process is reading input from the cyg-pipe.
>=20
> Fixes: f20641789427 ("Cygwin: pty: Reduce unecessary input transfer.")
> Signed-off-by: Takashi Yano <[email protected]>
> Reviewed-by: Johannes Schindelin <[email protected]>
> ---
> v2: Release all masks owned by myself on cleanup()
> v3: Reverts the change that made num_reader and slave_reading shared
>=20
>  winsup/cygwin/fhandler/pty.cc | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>=20
> diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.=
cc
> index ca85ae679..963f95801 100644
> --- a/winsup/cygwin/fhandler/pty.cc
> +++ b/winsup/cygwin/fhandler/pty.cc
> @@ -1282,6 +1282,10 @@ fhandler_pty_slave::open_setup (int flags)
>  void
>  fhandler_pty_slave::cleanup ()
>  {
> +  fhandler_pty_slave *arch =3D (fhandler_pty_slave *) archetype ? : thi=
s;
> +  while (arch->num_reader)
> +    mask_switch_to_nat_pipe (false, false);
> +
>    if (get_ttyp ()->pcon_activated && get_ttyp ()->getpgid () =3D=3D mys=
elf->pgid)
>      req_fixup_pcon_state ();
> =20
> @@ -1543,19 +1547,22 @@ fhandler_pty_slave::write (const void *ptr, size=
_t len)
>  void
>  fhandler_pty_slave::mask_switch_to_nat_pipe (bool mask, bool xfer)
>  {
> +  acquire_attach_mutex (mutex_timeout);
>    char name[MAX_PATH];
>    shared_name (name, TTY_SLAVE_READING, get_minor ());
>    HANDLE masked =3D OpenEvent (READ_CONTROL, FALSE, name);
>    CloseHandle (masked);
> =20
> +  fhandler_pty_slave *arch =3D (fhandler_pty_slave *) archetype ? : thi=
s;
>    WaitForSingleObject (input_mutex, mutex_timeout);
>    if (mask)
>      {
> -      if (InterlockedIncrement (&num_reader) =3D=3D 1)
> -	slave_reading =3D CreateEvent (&sec_none_nih, TRUE, FALSE, name);
> +      if (InterlockedIncrement (&arch->num_reader) =3D=3D 1)
> +	arch->slave_reading =3D CreateEvent (&sec_none_nih, TRUE, FALSE, name)=
;
>      }
> -  else if (InterlockedDecrement (&num_reader) =3D=3D 0)
> -    CloseHandle (slave_reading);
> +  else if (InterlockedDecrement (&arch->num_reader) =3D=3D 0)
> +    CloseHandle (arch->slave_reading);
> +  release_attach_mutex ();

The acquisition order here is attach_mutex first, then input_mutex.
Elsewhere in pty.cc the established order is the opposite: input_mutex
first, and attach_mutex only around the transfer_input call inside.
See for example setpgid_aux and cleanup_for_non_cygwin_app, both of
which do

        WaitForSingleObject (input_mutex, mutex_timeout);
        ...
        acquire_attach_mutex (mutex_timeout);
        transfer_input (...);
        release_attach_mutex ();
        ...
        ReleaseMutex (input_mutex);

and the same pattern appears at the open_setup path as well as two
further sites in pty.cc.

Since attach_mutex is a per-process unnamed mutex, declared and lazily
created at winsup/cygwin/fhandler/console.cc:1012-1015 in cygwin-3.6.9 as

        extern HANDLE attach_mutex;
        if (!attach_mutex)
          attach_mutex =3D CreateMutex (&sec_none_nih, FALSE, NULL);

this cannot deadlock across processes. It _can_ deadlock intra-process,
though: thread T1 in mask_switch_to_nat_pipe holds attach_mutex and waits
for input_mutex, while thread T2 in setpgid_aux (or any of the other
input_mutex-then-attach_mutex sites) holds input_mutex and waits for
attach_mutex. mutex_timeout is INFINITE on the normal paths (the 0-timeout
variants only appear on the GDB paths), so the resulting hang would be
hard rather than a graceful timeout. Threaded cygwin readers on a pty
(python with threads, tmux, gdb driving an inferior) are the plausible
triggers.

For the record, I had Claude check that no direct caller of
mask_switch_to_nat_pipe already holds input_mutex when calling it (the
read path even explicitly releases input_mutex before the
mask_switch_to_nat_pipe (false, false) call); so the risk is entirely from
a _concurrent_ thread in the same process, not from the caller itself.

Two possible shapes. The simplest is to swap the order inside
mask_switch_to_nat_pipe so it matches the rest of the file: acquire
input_mutex first, then attach_mutex around the CreateEvent/CloseHandle
block, and release in reverse. The interlock against transfer_input's
OpenEvent existence check should still hold, since transfer_input's
callers already take input_mutex before attach_mutex. If instead the new
order is required for some reason I am missing, it would be worth a
comment near the acquire explaining why, plus an audit that no
input_mutex-holding path can want attach_mutex from a concurrent thread.
Does the reasoning make sense to you, and does the swap sound right?

Ciao,
Johannes

> =20
>    if (!!masked !=3D mask && xfer && get_ttyp ()->switch_to_nat_pipe)
>      {
> @@ -4460,6 +4467,18 @@ fhandler_pty_slave::transfer_input (tty::xfer_dir=
 dir, HANDLE from, tty *ttyp,
>  				    HANDLE input_available_event,
>  				    HANDLE input_transferred_to_cyg)
>  {
> +  if (dir =3D=3D tty::to_nat)
> +    {
> +      char name[MAX_PATH];
> +      shared_name (name, TTY_SLAVE_READING, ttyp->get_minor ());
> +      HANDLE masked =3D OpenEvent (READ_CONTROL, FALSE, name);
> +      CloseHandle (masked);
> +      if (masked)
> +	/* Cygwin process is reading cyg-pipe.
> +	   Do not transfer input to nat-pipe. */
> +	return;
> +    }
> +
>    HANDLE to;
>    if (dir =3D=3D tty::to_nat)
>      to =3D ttyp->to_slave_nat ();
> --=20
> 2.51.0
>=20
>=20