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

Hi Takashi,

On Mon, 6 Jul 2026, Takashi Yano wrote:

> On Sun, 5 Jul 2026 10:10:51 +0200 (CEST) Johannes Schindelin wrote:
>=20
> > Concretely, consider two cygwin readers on the same pty:
> >=20
> > 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 go=
es
> > 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 g=
oes
> > to 0, and B executes `CloseHandle (get_ttyp ()->slave_reading)`. But t=
hat
> > 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, tha=
t
> > 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.
> >=20
> > In v1 both fields were per-fhandler, so this could not arise: whicheve=
r
> > fhandler created the event also closed it, in the same process. For th=
e
> > specific same-process teardown path that v2's `cleanup ()` adds ("rele=
ase
> > all masks I still own"), the hazard also does not trigger by construct=
ion,
> > since the process draining `masked_cnt` is the same one that took thos=
e
> > masks. So the concern is purely about the general mask/unmask API cont=
ract
> > now that the storage is shared.
> >=20
> > Two questions, then:
> >=20
> > First, is cross-process mask ownership actually reachable via the curr=
ent
> > call sites (read paths, `cleanup ()`, `close ()`, exec/spawn transitio=
ns)?
> > If every mask is guaranteed to be released by the same process that to=
ok
> > it, the hazard is theoretical and it would suffice to document that
> > invariant near the field. I have not fully traced this myself and woul=
d
> > trust your reading here.
>=20
> This happens when,
>  1) Start `cat` and suspend it by Ctrl-Z
>  2) Start another `cat` and suspend it by Ctrl-Z
>  3) Foreground the first `cat` and press Ctrl-D
>  4) Foreground the second `cat` and press Ctrl-D
>=20
> > Second, if it _is_ reachable, would it make sense to mirror the by-nam=
e
> > lookup you already do in `transfer_input ()` on the release side as we=
ll,
> > 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 shar=
ed
> > struct would only serve as the "an event with this name exists" flag, =
and
> > no cross-process HANDLE ever gets closed.
>=20
> Looks good. Thanks! You can find the similar in v3 patch.
> It reverts the change that made num_reader and slave_reading shared.
> Even if a process close slave_reading, the other process keeps
> slave_reading opened. Therefore, checking `masked` by OpenEvent() return=
s
> true. Therefore, there was no need to make slave_reading and num_reader
> shared.

Basically: the kernel already refcounts the named event object for us.
As long as any process holds a handle obtained via CreateEvent on that
name, OpenEvent-by-name from any other process succeeds. So each
process managing its own per-fhandler num_reader and its own handle to
the shared named event is sufficient, and the "an event with this name
exists" signal is emergent from the kernel's own refcount without any
HANDLE ever needing to cross a process boundary. That is a nice
observation, and it makes v3 considerably simpler than v2.

Thanks,
Johannes