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

Hi Johannes,

Thanks for reviewing!

On Tue, 21 Jul 2026 19:16:11 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> sorry for getting to reply only after you pushed this to `master`. I
> wanted to take the time to double-check a couple of things, and other
> responsibilities got into the way.
> 
> On Fri, 17 Jul 2026, Takashi Yano wrote:
> 
> > [...]
> > diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> > index 2bea79768..e3ba8c65c 100644
> > --- a/winsup/cygwin/syscalls.cc
> > +++ b/winsup/cygwin/syscalls.cc
> > @@ -1451,6 +1451,7 @@ extern "C" int
> >  open (const char *unix_path, int flags, ...)
> >  {
> >    int res = -1;
> > +  int fd = -1;
> >    va_list ap;
> >    mode_t mode = 0;
> >    fhandler_base *fh = NULL;
> > @@ -1550,9 +1551,12 @@ open (const char *unix_path, int flags, ...)
> >        /* Reserve an fdtable entry here, before calling open_with_arch() below.
> >           Otherwise there's a tiny chance of hitting OPEN_MAX further on which
> >           could create a new file without any way for Cygwin to refer to it. */
> > -      cygheap_fdnew fd;
> > +      cygheap->fdtab.lock();
> > +      fd = cygheap->fdtab.find_unused_handle ();
> >        if (fd < 0)
> > -        __leave;		/* errno already set */
> > +	__leave;		/* errno already set */
> > +      cygheap->fdtab[fd] = fh; /* tentative setting to mark as used */
> > +      cygheap->fdtab.unlock();
> >  
> >        if (fh->dev () == FH_PROCESSFD && fh->pc.follow_fd_symlink ())
> >  	{
> 
> I see three problems here:
> 
> When `fd` is negative, the `unlock()` right below is skipped, and
> `__endtry` only unlocks for non-negative ones. So an `open()` that ran out
> of descriptors returns with the fdtab still locked, and the next fdtab
> operation on any other thread waits for good. `cygheap_fdnew` used to
> release the lock in that case; this one does not. I reproduced [*1*] it
> three times out of three on a local build. It stays invisible while
> single-threaded, because the muto is reentrant -- which is also why the
> OPEN_MAX test still passes.

Ugh, I messed up…

> The tentative assignment puts `fh` into the table with a zero reference
> count, then drops the lock across `open_with_arch()`, which for a FIFO
> waits for the other end. Any lookup in that window -- an `fcntl()`, a
> `close()`, a `close_range()` -- raises the count from zero to one and
> lowers it straight back, freeing `fh` while `open()` is still using it and
> leaving the slot pointing at nothing valid. I reproduced [*2*] it by
> querying the descriptor from a second thread while the open was waiting:
> the process ended with an access violation (status 0xC0000005), and the
> same run without the query was clean.

How do you know the descriptor which open() does not return it yet
in your reproducer?

> 
> The `FH_PROCESSFD` branch shows the same thing with no threads at all: it
> deletes `fh` and repoints the local variable at the reopened handler, but
> the table still refers to the deleted one until the re-assignment further
> down.

The same here. The fd is not returned by open() yet, so the program
cannot think the fd is the valid file descriptor, I think...

> > @@ -1580,13 +1584,23 @@ open (const char *unix_path, int flags, ...)
> >  	try_to_bin (fh->pc, fh->get_handle (), DELETE,
> >  		    FILE_OPEN_FOR_BACKUP_INTENT);
> >  
> > -      fd = fh;
> > +      cygheap->fdtab.lock ();
> > +      cygheap->fdtab[fd] = fh;
> > +      fh->inc_refcnt ();
> > +      cygheap->fdtab.unlock ();
> > +
> >        if (fd <= 2)
> >  	set_std_handle (fd);
> >        res = fd;
> >      }
> >    __except (EFAULT) {}
> >    __endtry
> > +    if (res < 0 && fd >= 0)
> > +      {
> > +	cygheap->fdtab.lock ();
> > +	cygheap->fdtab[fd] = NULL; /* Mark as unused */
> > +	cygheap->fdtab.unlock ();
> > +      }
> >    if (res < 0 && fh)
> >      delete fh;
> >    syscall_printf ("%R = open(%s, %y)", res, unix_path, flags);
> 
> 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?

-- 
Takashi Yano <[email protected]>