Re: [PATCH v2] Cygwin: open: Unlock fdtab before open_with_arch()

Hi Takashi,

Thank you for trying the experiment so quickly. To answer your closing
question first: yes, both outputs are exactly what a correct fix should
produce. Let me walk through why, and then flag two caveats about the
reserved-marker design before we build it out.

On Wed, 22 Jul 2026, Takashi Yano wrote:

> On Wed, 22 Jul 2026 20:10:12 +0900
> Takashi Yano wrote:
> > Hi Johannes,
> >=20
> > Thanks for reviewing!
> >=20
> > On Tue, 21 Jul 2026 19:16:11 +0200 (CEST)
> > Johannes Schindelin wrote:
> > > This is the first point where the slot and its reference count agree
> > > again; Too late for anything that looked in between. And the cleanup=
 only
> > > runs for non-negative descriptors, so it never covers the case above=
.
> > >=20
> > > Since it is already in `master`, a follow-up patch probably makes mo=
st
> > > sense. Two things to fix: release the lock when no descriptor is
> > > available, and stop a reserved-but-not-yet-open descriptor from look=
ing
> > > like a fully open one to the rest of the fdtable. Reviving the old i=
nteger
> > > marker would mean teaching every consumer of the table about it, so =
it is
> > > not a drop-in.
> >=20
> > Ah, I got it. The user program cannot know that, but cygwin1.dll can
> > refere fdtab inside it. What about adding reserved flag to fdtab?

Adding a reserved marker is the right approach, and it is what I had in
mind too. Keeping the reserved slot hidden from the rest of the table lets
`open()` remain the sole owner of the handler and delete it on any error
path, including the `FH_PROCESSFD` reopen, without any reference-count
juggling.

The first caveat, and the bigger one: this is not a drop-in change. Every
place that treats a non-NULL slot as a live handler has to learn to skip
the reserved marker. Besides `cygheap_fdget`, that includes
`cygheap_fdenum::next`, `not_open`, and the five fork/exec fixup loops
(`set_file_pointers_for_exec`, `fixup_after_exec`, `fixup_after_fork`,
`fixup_before_fork`, and `fixup_before_exec`). If a fork or exec runs on
another thread while a FIFO open is still holding the reservation, those
loops would dereference the marker.

To keep that safe, I would encode the marker as a single distinguished
pointer value stored in the slot itself, rather than as a separate flag.
Then the lock-free `cygheap_fdget` keeps working with one added
comparison, and there is no risk of a torn read.

>=20
> I tried it as quick experiment.
>=20
> The result of the first reproducer is:
> baseline close(-1): result=3D-1 errno=3D9
> descriptor allocation stopped after 3197 opens: errno=3D24
> second-thread close(-1): result=3D-1 errno=3D9

The decisive line here is the second thread's `close(-1)` returning
`EBADF` (errno 9) instead of blocking. That is the proof that the table
lock is no longer held when no descriptor is available; before the fix,
the second thread would have waited forever on the process-wide lock. The
allocation stopping after 3197 opens with `EMFILE` (errno 24) is right as
well: that is `OPEN_MAX` (3200, spelled `__OPEN_MAX` in the headers) minus
the three standard descriptors, so the ceiling is reached cleanly.

> The result of the second reproducer is:
> expected FIFO writer descriptor: 4
> provisional descriptor query: result=3D-1 writer_done=3D0

This one is exactly right too. The writer taking descriptor 4 is just the
expected setup; the line that matters is the provisional-descriptor query,
which returns -1 (`EBADF`) with `writer_done` still 0 while the FIFO open
is still blocked waiting for the other end. The reserved descriptor now
reports a bad descriptor instead of triggering the access violation
(status 0xC0000005) I saw before, where `cygheap_fdget` raised the
reference count from zero to one and its destructor then lowered it back
to zero and deleted the handler while `open()` was still blocked inside
`open_with_arch()`.

> Are these results as you expected?

Yes, as detailed above. The second caveat is about coverage: the two
reproducers drive `close()` and the descriptor lookup on a single known
number, but not the paths that walk the whole table. `close_range` is the
clearest of those. It iterates every descriptor in the range and looks
each one up, so it reaches a reserved slot without ever being told its
number. That, incidentally, answers the question you raised earlier about
how the racing thread could reference a descriptor that `open()` has not
returned yet: it does not _have_ to. The internal iterators reach the slot
regardless. A `close_range` call and a fork or exec, each racing an
in-flight open, would exercise the paths the current reproducers miss.

Independently of all that, the lock-leak fix stands on its own and is
trivial, namely releasing the table lock when no descriptor is available,
so it could land first as a separate commit while we work out the marker
design.

Ciao,
Johannes