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

Hi Johannes,

On Wed, 22 Jul 2026 22:29:44 +0200 (CEST)
Johannes Schindelin wrote:
> 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,
> > > 
> > > Thanks for reviewing!
> > > 
> > > 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.
> > > > 
> > > > Since it is already in `master`, a follow-up patch probably makes most
> > > > sense. Two things to fix: release the lock when no descriptor is
> > > > available, and stop a reserved-but-not-yet-open descriptor from looking
> > > > like a fully open one to the rest of the fdtable. Reviving the old integer
> > > > marker would mean teaching every consumer of the table about it, so it is
> > > > not a drop-in.
> > > 
> > > 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.
> 
> > 
> > I tried it as quick experiment.
> > 
> > The result of the first reproducer is:
> > baseline close(-1): result=-1 errno=9
> > descriptor allocation stopped after 3197 opens: errno=24
> > second-thread close(-1): result=-1 errno=9
> 
> 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=-1 writer_done=0
> 
> 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.

I implemented two different ways, one uses independent reserved flag,
the other uses special value ((fhandler_base *) -1) as a reservation
marker.

I'll submit both patches, so please review the one you think is the
better approach.

-- 
Takashi Yano <[email protected]>