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

Hi Mark,

On Thu, 16 Jul 2026 23:36:02 -0700 (PDT)
Mark Geisert wrote:
> Hi Takashi,
> I'm offline for a while due to a system boot malfunction and subsequent 
> rebuilding from backups so I cannot test patches as I normally would do. 
> I do have some comments below...
> 
> On Fri, 17 Jul 2026, Takashi Yano wrote:
> > Since the commit 31bf91f867c5, opening fifo causes a deadlock. This
> > is because, open_with_arch() for fifo can be blocked until the other
> > side of the fifo is opened. The commit 31bf91f867c5 moves the creating
> > cygheap_fdnew before open_with_arch() to address the issue:
> > https://cygwin.com/pipermail/cygwin/2026-May/259664.html
> > However, cygheap_fdnew locks fdtab, so open() for the other side of
> > fifo cannot create cygheap_fdnew until fdtab is unlocked. This is
> > the cause of the deadlock.
> 
> Thank you for diagnosing the problem and coming up with a fix!
> 
> > With this patch, fdtab is unlocked before open_with_arch(), but marked
> > as used using tentative fhandler. The summary of open() is as follows.
> > 1) Lock fdtab.
> > 2) Create new fd.
> > 3) Mark fd as used using tentative fhandler.
> > 4) Unlock fdtab.
> > 5) Call open_with_arch().
> > 6) Set final fhandler to fd.
> >
> > The important point is that create fd before open_with_arch() to
> > address https://cygwin.com/pipermail/cygwin/2026-May/259664.html,
> > but unlock fdtab before open_with_arch() to address
> > https://cygwin.com/pipermail/cygwin/2026-July/259884.html.
> >
> > Fixes: 31bf91f867c5 ("Cygwin: Ensure unused fd available for open()")
> > Addresses: https://cygwin.com/pipermail/cygwin/2026-July/259884.html
> > Reported-by: kikairoya <[email protected]>
> > Signed-off-by: Takashi Yano <[email protected]>
> > Reviewed-by:
> > ---
> > v2: Add lock/unlock when modifying the fdtab, just to be safe.
> >
> > winsup/cygwin/syscalls.cc | 20 +++++++++++++++++---
> > 1 file changed, 17 insertions(+), 3 deletions(-)
> >
> > 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 */
> 
> Not sure about the above two lines.. did one of us use TABs and the other 
> did not?  A minor thing.

Yes, your previous patch uses 8 spaces at begining of the line.
It was replaced with one tab, when I use auto-indent.
> > +      cygheap->fdtab[fd] = fh; /* tentative setting to mark as used */
> 
> When I was looking into this area of code I couldn't determine if 'fh' was 
> non-null in every code path.  That's why I had proposed a distinctive 
> value to use (-1 IIRC).  Would be great if you know for sure it's safe.

syscalls.cc:
1485       /* If we're opening a FIFO, we will call device_access_denied
1486          below.  This leads to a call to fstat, which can use the
1487          path_conv handle. */
1488       opt |= PC_KEEP_HANDLE;
1489       if (!(fh = build_fh_name (unix_path, opt, stat_suffixes)))
1490         __leave;                /* errno already set */

fh is initialized here, or __leave.

> > +      cygheap->fdtab.unlock();
> >
> >       if (fh->dev () == FH_PROCESSFD && fh->pc.follow_fd_symlink ())

Pointer fh is used just after the patch lines (above fh->dev()) without
check.

> > 	{
> > @@ -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 ();
> 
> I had wondered about using InterlockedExchange() but your code is more 
> explicit, so I go with you on this.

I also consider using InterlockedExchangePointer(), or not
using a guard here because open() still not return this fd,
and no other thread using this fd because find_unused_handle ()
returned this fd as unused.
However, it is better to use fdtab.lock() as a precaution for
overlooking something.

> > +      }
> >   if (res < 0 && fh)
> >     delete fh;
> >   syscall_printf ("%R = open(%s, %y)", res, unix_path, flags);
> > -- 
> > 2.51.0
> >
> 
> Thanks again Takashi for diving in so quickly on this report.

Thanks for reviewing so quickly!

-- 
Takashi Yano <[email protected]>