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

Hi Takashi,

Thank you for v2. The overall direction (blocking the nat-pipe transfer
while a cygwin reader is active on the cyg-pipe) is correct, and the new
cleanup hook is a good defensive addition against a slave that exits
mid-read wedging the pipe state. I do want to flag one hazard, though,
which I think v2 introduces by moving `slave_reading` and `num_reader`
from the fhandler into the shared `tty` struct.

On Tue, 30 Jun 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:
> ---
> v2: Release all masks owned by myself on cleanup()
>=20
>  winsup/cygwin/fhandler/pty.cc           | 33 +++++++++++++++++++++----
>  winsup/cygwin/local_includes/fhandler.h |  3 +--
>  winsup/cygwin/local_includes/tty.h      |  2 ++
>  3 files changed, 31 insertions(+), 7 deletions(-)
>=20
> diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.=
cc
> index 35e320507..54cd64a47 100644
> --- a/winsup/cygwin/fhandler/pty.cc
> +++ b/winsup/cygwin/fhandler/pty.cc
> @@ -951,7 +951,7 @@ out:
> =20
>  fhandler_pty_slave::fhandler_pty_slave (int unit, dev_t via)
>    : fhandler_pty_common (), inuse (NULL), output_handle_nat (NULL),
> -  io_handle_nat (NULL), slave_reading (NULL), num_reader (0)
> +  io_handle_nat (NULL), masked_cnt (0)
>  {
>    dev_referred_via =3D via;
>    if (unit >=3D 0)
> @@ -1230,6 +1230,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->masked_cnt)
> +    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
> @@ -1499,11 +1503,18 @@ fhandler_pty_slave::mask_switch_to_nat_pipe (boo=
l mask, bool xfer)
>    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 (&get_ttyp ()->num_reader) =3D=3D 1)
> +	get_ttyp ()->slave_reading =3D
> +	  CreateEvent (&sec_none_nih, TRUE, FALSE, name);
>      }
> -  else if (InterlockedDecrement (&num_reader) =3D=3D 0)
> -    CloseHandle (slave_reading);
> +  else if (InterlockedDecrement (&get_ttyp ()->num_reader) =3D=3D 0)
> +    CloseHandle (get_ttyp ()->slave_reading);

`num_reader` is fine to share (it is just a counter, and the interlocked
increment/decrement pair does the right thing across processes).
`slave_reading`, however, is a `HANDLE`, and HANDLE values are
per-process: they index into the owning process's handle table and are not
portable to another process without `DuplicateHandle`. Storing the raw
value in shared memory and letting an arbitrary process call `CloseHandle`
on it later is therefore hazardous.

Concretely, consider two cygwin readers on the same pty:

Process A takes the first mask. `InterlockedIncrement` returns 1, so A
calls `CreateEvent` and writes A's handle value into `get_ttyp
()->slave_reading`. Process B then takes a second mask; the counter goes
to 2, so B does not create or store anything. A releases first: the
counter goes to 1, so A does not close. B releases last: the counter goes
to 0, and B executes `CloseHandle (get_ttyp ()->slave_reading)`. But that
HANDLE value lives in A's handle table, not B's. In the benign case B gets
`ERROR_INVALID_HANDLE` and the event object leaks (A already lost its slot
in this API, so nobody will ever close it). In the malignant case, that
same numeric HANDLE value happens to be live in B's own handle table
pointing at an unrelated object, which B then closes out from under
itself.

In v1 both fields were per-fhandler, so this could not arise: whichever
fhandler created the event also closed it, in the same process. For the
specific same-process teardown path that v2's `cleanup ()` adds ("release
all masks I still own"), the hazard also does not trigger by construction,
since the process draining `masked_cnt` is the same one that took those
masks. So the concern is purely about the general mask/unmask API contract
now that the storage is shared.

Two questions, then:

First, is cross-process mask ownership actually reachable via the current
call sites (read paths, `cleanup ()`, `close ()`, exec/spawn transitions)?
If every mask is guaranteed to be released by the same process that took
it, the hazard is theoretical and it would suffice to document that
invariant near the field. I have not fully traced this myself and would
trust your reading here.

Second, if it _is_ reachable, would it make sense to mirror the by-name
lookup you already do in `transfer_input ()` on the release side as well,
that is, `OpenEvent` the named event by name inside
`mask_switch_to_nat_pipe (false, ...)` when the counter hits zero and
close the freshly opened handle, so `CloseHandle` always operates on a
handle native to the closing process? Then `slave_reading` in the shared
struct would only serve as the "an event with this name exists" flag, and
no cross-process HANDLE ever gets closed.

One much smaller, non-blocking observation on the check you added at the
top of `transfer_input ()`:

> +
> +  fhandler_pty_slave *arch =3D (fhandler_pty_slave *) archetype ? : thi=
s;
> +  if (mask)
> +    InterlockedIncrement (&arch->masked_cnt);
> +  else
> +    InterlockedDecrement (&arch->masked_cnt);
> =20
>    if (!!masked !=3D mask && xfer && get_ttyp ()->switch_to_nat_pipe)
>      {
> @@ -4401,6 +4412,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;
> +    }

There is a small TOCTOU window here: another thread can take the first
mask (and create the event) in between our `OpenEvent` returning NULL and
our `to_nat` transfer actually starting. The bounded worst case is one
spurious transfer, not corruption, so I do not consider this a blocker;
just noting it for the record in case a tighter interlock via
`input_mutex` is cheap here.

Ciao,
Johannes

> +
>    HANDLE to;
>    if (dir =3D=3D tty::to_nat)
>      to =3D ttyp->to_slave_nat ();
> diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/loc=
al_includes/fhandler.h
> index 8e9cbef4b..d8b6f5950 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -2442,8 +2442,7 @@ class fhandler_pty_slave: public fhandler_pty_comm=
on
>  {
>    HANDLE inuse;			// used to indicate that a tty is in use
>    HANDLE output_handle_nat, io_handle_nat;
> -  HANDLE slave_reading;
> -  LONG num_reader;
> +  LONG masked_cnt;
> =20
>    /* Helper functions for fchmod and fchown. */
>    bool fch_open_handles (bool chown);
> diff --git a/winsup/cygwin/local_includes/tty.h b/winsup/cygwin/local_in=
cludes/tty.h
> index c5102eb81..407565ce9 100644
> --- a/winsup/cygwin/local_includes/tty.h
> +++ b/winsup/cygwin/local_includes/tty.h
> @@ -146,6 +146,8 @@ private:
>    bool discard_input;
>    bool stop_fwd_thread;
>    bool req_fixup_pcon_cur_pos;
> +  HANDLE slave_reading;
> +  LONG num_reader;
> =20
>  public:
>    HANDLE from_master_nat () const { return _from_master_nat; }
> --=20
> 2.51.0
>=20
>=20