Re: [PATCH v2] Cygwin: open: Do not set tentative fhandler to fdtab

Hi Johannes,

Thanks for reviewing v2 patch so quickly!

On Wed, 5 Aug 2026 11:29:54 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> This is exactly what I was hoping for. v2 addresses every point from the
> last round.
> 
> On Wed, 5 Aug 2026, Takashi Yano wrote:
> 
> > Tentative assignment of fhandler to fdtab introduced by the commit
> > 524d75ff7398 ("Cygwin: open: Unlock fdtab before open_with_arch()")
> > causes the undesired behaviour. The commit intended that fhandler
> > was just a marker for reservation of fd. However, another cygwin
> > call may assume that the fd is valid and in use, and may operate on
> > it.
> > 
> > This patch introduces a special value ((fhandler_base *) -1) for
> > fdtab that marks the fd as reserved and means it cannot be assigned
> > for another open(), etc.
> > 
> > Fixes: 524d75ff7398 ("Cygwin: open: Unlock fdtab before open_with_arch()")
> > Suggested-by: Johannes Schindelin <[email protected]>
> > Signed-off-by: Takashi Yano <[email protected]>
> > Reviewed-by: Johannes Schindelin <[email protected]>
> > ---
> >  winsup/cygwin/dtable.cc                | 29 ++++++++++++-----------
> >  winsup/cygwin/local_includes/cygheap.h |  6 ++---
> >  winsup/cygwin/local_includes/dtable.h  | 32 ++++++++++++++++++++++++--
> >  winsup/cygwin/syscalls.cc              | 13 +++++++----
> >  4 files changed, 57 insertions(+), 23 deletions(-)
> > 
> > diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
> > index e4d1cdf8f..7a4fd7dca 100644
> > --- a/winsup/cygwin/dtable.cc
> > +++ b/winsup/cygwin/dtable.cc
> > @@ -13,6 +13,7 @@ details. */
> >  #include <stdio.h>
> >  #include <unistd.h>
> >  #include <wchar.h>
> > +#include <assert.h>
> >  
> >  #define USE_SYS_TYPES_FD_SET
> >  #include <winsock.h>
> > @@ -123,7 +124,7 @@ dtable::get_debugger_info ()
> >  	    fhandler_base *fh = build_fh_name (std[i]);
> >  	    if (!fh)
> >  	      continue;
> > -	    fds[i] = fh;
> > +	    fds.set_fhandler (i, fh);
> >  	    if (!fh->open ((i ? (i == 2 ? O_RDWR : O_WRONLY) : O_RDONLY)
> >  			   | O_BINARY, 0777))
> >  	      release (i);
> > @@ -233,7 +234,7 @@ dtable::find_unused_handle (size_t start)
> >    do
> >      {
> >        for (size_t i = start; i < size; i++)
> > -	if (fds[i] == NULL)
> > +	if (fds[i] == NULL && !fds.reserved (i))
> >  	  {
> >  	    res = (int) i;
> >  	    goto out;
> > @@ -249,8 +250,9 @@ dtable::release (int fd)
> >  {
> >    if (fds[fd]->need_fixup_before ())
> >      dec_need_fixup_before ();
> > +  assert (fds[fd]);
> 
> Tiny nit, not a blocker: This assert sits _after_ the first dereference
> (`fds[fd]->need_fixup_before ()`), so it cannot catch a NULL-or-reserved
> slot before that line already faults. It wants to be the first statement
> in the function.
> 
> >    fds[fd]->dec_refcnt ();
> > -  fds[fd] = NULL;
> > +  fds.set_fhandler (fd, NULL);
> >    if (fd <= 2)
> >      set_std_handle (fd);
> >  }
> > @@ -263,11 +265,12 @@ cygwin_attach_handle_to_fd (char *name, int fd, HANDLE handle, mode_t bin,
> >    if (fd == -1)
> >      fd = cygheap->fdtab.find_unused_handle ();
> >    fhandler_base *fh = build_fh_name (name);
> > -  if (!fh)
> > +  if (!fh || cygheap->fdtab.reserved (fd))
> >      fd = -1;
> >    else
> >      {
> > -      cygheap->fdtab[fd] = fh;
> > +      cygheap->fdtab.set_fhandler (fd, fh);
> > +      assert (cygheap->fdtab[fd]);
> >        cygheap->fdtab[fd]->inc_refcnt ();
> >        fh->init (handle, myaccess, bin ?: fh->pc_binmode ());
> >      }
> > @@ -348,7 +351,7 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
> >      handle_to_fn (handle, name);
> >  
> >    if (!name[0] && !dev)
> > -    fds[fd] = NULL;
> > +    fds.set_fhandler (fd, NULL);
> >    else
> >      {
> >        fhandler_base *fh;
> > @@ -425,7 +428,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
> >        if (!fh->open_setup (openflags))
> >  	api_fatal ("open_setup failed, %E");
> >        fh->usecount = 0;
> > -      cygheap->fdtab[fd] = fh;
> > +      cygheap->fdtab.set_fhandler (fd, fh);
> > +      assert (cygheap->fdtab[fd]);
> >        cygheap->fdtab[fd]->inc_refcnt ();
> >        set_std_handle (fd);
> >        paranoid_printf ("fd %d, handle %p", fd, handle);
> > @@ -795,16 +799,15 @@ dtable::dup3 (int oldfd, int newfd, int flags)
> >  
> >    if (!not_open (newfd))
> >      close (newfd);
> > -  else if ((size_t) newfd >= size
> > -	   && find_unused_handle (newfd) < 0)
> > +  else if (((size_t) newfd >= size && find_unused_handle (newfd) < 0)
> > +	   || reserved (newfd))
> 
> With this, `dup3` (hence `dup2`) and `cygwin_attach_handle_to_fd` with an
> explicit fd refuse a reserved target instead of overwriting it. That was
> the third gap from the last round.
> 
> One raw exposure remains, and I want to record that it is fine: `dtable`
> still hands out the underlying array through `operator fhandler_base
> **()`. But with the wrapper in place its only callers are `extend()`'s own
> bookkeeping (the `memcpy`, the `cfree`, `fds = newfds`) and the
> `!cygheap->fdtab` allocated-check in `init_cygheap`, neither of which
> indexes a slot, so the trapdoor is effectively shut. I also checked the
> mechanical consequence of `operator[]` no longer yielding an lvalue: every
> former raw `fds[...] =` assignment moved to `set_fhandler`
> (`get_debugger_info`, `release`, both `init_std_file_from_handle`
> branches, `dup3`, `move_fd`), so nothing writes through the masked
> accessor.
> 
> One more unrelated nit: the new `dup3` branch that rejects a reserved
> `newfd` returns -1 without setting `errno`. The `reserved (newfd)`
> disjunct falls into the "couldn't extend fdtab" arm, which sets none. It
> is only reachable if a `dup2` targets an fd number a concurrent `open()`
> is still reserving (undefined territory), so it is cosmetic, but an
> `EBADF` there would be tidier.
> 
> One pre-existing aside, explicitly out of scope: a multithreaded `fork()`
> while another thread is blocked in a FIFO `open()` still leaves the
> reserved slot set in the child, where no thread survives to clear it, so
> that fd stays unusable there. It no longer crashes (the child's fixup now
> skips the masked slot), and it predates this patch (the same window at
> `0d3ea0ee` inherits a half-open `fhandler`, which is worse), so nothing to
> change here. Just noting it.
> 
> So the Reviewed-by stands as added. Thank you for fixing this!
> 
> Ciao,
> Johannes
> 
> >      /* couldn't extend fdtab */
> >      {
> >        newfh->close ();
> >        res = -1;
> >        goto done;
> >      }
> > -
> > -  fds[newfd] = newfh;
> > +  fds.set_fhandler (newfd, newfh);
> >  
> >    if ((res = newfd) <= 2)
> >      set_std_handle (res);
> > @@ -874,8 +877,8 @@ void
> >  dtable::move_fd (int from, int to)
> >  {
> >    // close (to); /* It is assumed that this is close-on-exec */
> > -  fds[to] = fds[from];
> > -  fds[from] = NULL;
> > +  fds.set_fhandler (to, fds[from]);
> > +  fds.set_fhandler (from, NULL);
> >  }
> >  
> >  void
> > diff --git a/winsup/cygwin/local_includes/cygheap.h b/winsup/cygwin/local_includes/cygheap.h
> > index 74cfff652..db740f03d 100644
> > --- a/winsup/cygwin/local_includes/cygheap.h
> > +++ b/winsup/cygwin/local_includes/cygheap.h
> > @@ -569,10 +569,10 @@ class cygheap_fdmanip
> >    }
> >    virtual void release () { cygheap->fdtab.release (fd); }
> >    operator int &() {return fd;}
> > -  operator fhandler_base* &() {return cygheap->fdtab[fd];}
> > +  operator fhandler_base* () {return cygheap->fdtab[fd];}
> >    operator fhandler_socket* () const {return reinterpret_cast<fhandler_socket *> (cygheap->fdtab[fd]);}
> >    operator fhandler_pipe* () const {return reinterpret_cast<fhandler_pipe *> (cygheap->fdtab[fd]);}
> > -  void operator = (fhandler_base *fh) {cygheap->fdtab[fd] = fh;}
> > +  void operator = (fhandler_base *fh) {cygheap->fdtab.set_fhandler (fd, fh);}
> >    fhandler_base *operator -> () const {return cygheap->fdtab[fd];}
> >    bool isopen () const
> >    {
> > @@ -609,7 +609,7 @@ class cygheap_fdnew : public cygheap_fdmanip
> >      if (cygheap->fdtab[fd])
> >        cygheap->fdtab[fd]->inc_refcnt ();
> >    }
> > -  void operator = (fhandler_base *fh) {cygheap->fdtab[fd] = fh;}
> > +  void operator = (fhandler_base *fh) {cygheap->fdtab.set_fhandler (fd, fh);}
> >  };
> >  
> >  class cygheap_fdget : public cygheap_fdmanip
> > diff --git a/winsup/cygwin/local_includes/dtable.h b/winsup/cygwin/local_includes/dtable.h
> > index 7803fae1b..89c776c2d 100644
> > --- a/winsup/cygwin/local_includes/dtable.h
> > +++ b/winsup/cygwin/local_includes/dtable.h
> > @@ -17,9 +17,30 @@ details. */
> >  class suffix_info;
> >  
> >  #define BFH_OPTS (PC_NULLEMPTY | PC_FULL | PC_POSIX)
> > +#define FDTAB_RESERVED ((fhandler_base *) -1)
> >  class dtable
> >  {
> > -  fhandler_base **fds;
> > +  class dtable_fds
> > +  {
> > +    fhandler_base **fds;
> > +  public:
> > +    inline void set_fhandler (int fd, fhandler_base *fh) {fds[fd] = fh;}
> > +    inline fhandler_base *operator [](int fd) const
> > +    {
> > +      fhandler_base *fh = fds[fd];
> > +      return (fh == FDTAB_RESERVED) ? NULL : fh;
> > +    }
> 
> Moving the masking into the `dtable_fds` accessor is the right move; it is
> precisely the "hide it in the storage layer" I was after. The five
> fork/exec fixup loops (`set_file_pointers_for_exec`, `fixup_before_fork`,
> `fixup_after_fork`, `fixup_before_exec`, `fixup_after_exec`) all read the
> member `fds[i]`, so they now see NULL for a reserved slot with no change
> at any site; `select_read`/`select_write`/`select_except` and the
> free-standing `set_std_handle` likewise. I traced each of them: the
> sentinel can no longer reach a dereference through the member. And the
> single load into `fh` closes the torn-read window I worried about last
> time: one read, compared and returned, so a lock-free `cygheap_fdget` can
> never observe the marker escaping the mask.
> 
> > +    operator fhandler_base **() {return fds;}
> > +    void operator = (fhandler_base **ptr) {fds = ptr;}
> > +    inline void reserve (int fd) { fds[fd] = FDTAB_RESERVED; }
> > +    inline void unreserve (int fd)
> > +    {
> > +      if (fds[fd] == FDTAB_RESERVED)
> > +	fds[fd] = NULL;
> > +    }
> > +    inline bool reserved (int fd) { return fds[fd] == FDTAB_RESERVED; }
> > +  };
> > +  dtable_fds fds;
> >    fhandler_base **archetypes;
> >    unsigned narchetypes;
> >    unsigned farchetype;
> > @@ -61,7 +82,11 @@ public:
> >    void init_std_file_from_handle (int fd, HANDLE handle);
> >    int dup3 (int oldfd, int newfd, int flags);
> >    void fixup_after_exec ();
> > -  inline fhandler_base *&operator [](int fd) const { return fds[fd]; }
> > +  inline void set_fhandler (int fd, fhandler_base *fh)
> > +  {
> > +    fds.set_fhandler (fd, fh);
> > +  }
> > +  inline fhandler_base *operator [](int fd) const { return fds[fd]; }
> >    bool select_read (int fd, select_stuff *);
> >    bool select_write (int fd, select_stuff *);
> >    bool select_except (int fd, select_stuff *);
> > @@ -76,6 +101,9 @@ public:
> >    void fixup_before_fork (DWORD win_proc_id);
> >    void lock () {lock_process::locker.acquire ();}
> >    void unlock () {lock_process::locker.release ();}
> > +  inline void reserve (int fd) { fds.reserve (fd); }
> > +  inline void unreserve (int fd) { fds.unreserve (fd); }
> > +  inline bool reserved (int fd) { return fds.reserved (fd); }
> >  };
> >  
> >  fhandler_base *build_fh_dev (const device&, const char * = NULL);
> > diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> > index 5465d6c09..5ca02c0a1 100644
> > --- a/winsup/cygwin/syscalls.cc
> > +++ b/winsup/cygwin/syscalls.cc
> > @@ -24,6 +24,7 @@ details. */
> >  #include <dirent.h>
> >  #include <ntsecapi.h>
> >  #include <iptypes.h>
> > +#include <assert.h>
> >  #include "ntdll.h"
> >  
> >  #include <cygwin/version.h>
> > @@ -146,7 +147,9 @@ dup_finish (int oldfd, int newfd, int flags)
> >    int res;
> >    if ((res = cygheap->fdtab.dup3 (oldfd, newfd, flags | O_EXCL)) == newfd)
> >      {
> > -      cygheap_fdget (newfd)->inc_refcnt ();
> > +      cygheap_fdget cfd (newfd);
> > +      assert ((fhandler_base *) cfd);
> > +      cfd->inc_refcnt ();
> >        cygheap->fdtab.unlock ();	/* dup3 exits with lock set on success */
> >      }
> >    return res;
> > @@ -1558,8 +1561,8 @@ open (const char *unix_path, int flags, ...)
> >  	  cygheap->fdtab.unlock ();
> >  	  __leave;		/* errno already set */
> >  	}
> > -      cygheap->fdtab[fd] = fh; /* tentative setting to mark as used */
> > -      cygheap->fdtab.unlock();
> > +      cygheap->fdtab.reserve (fd);
> > +      cygheap->fdtab.unlock ();
> >  
> >        if (fh->dev () == FH_PROCESSFD && fh->pc.follow_fd_symlink ())
> >  	{
> > @@ -1588,7 +1591,7 @@ open (const char *unix_path, int flags, ...)
> >  		    FILE_OPEN_FOR_BACKUP_INTENT);
> >  
> >        cygheap->fdtab.lock ();
> > -      cygheap->fdtab[fd] = fh;
> > +      cygheap->fdtab.set_fhandler (fd, fh);
> >        fh->inc_refcnt ();
> >        cygheap->fdtab.unlock ();
> >  
> > @@ -1601,7 +1604,7 @@ open (const char *unix_path, int flags, ...)
> >      if (res < 0 && fd >= 0)
> >        {
> >  	cygheap->fdtab.lock ();
> > -	cygheap->fdtab[fd] = NULL; /* Mark as unused */
> > +	cygheap->fdtab.unreserve (fd);
> >  	cygheap->fdtab.unlock ();
> >        }
> >    if (res < 0 && fh)
> > -- 
> > 2.51.0

I'll push this patch to master and cygwin-3_6-branch with minor fix.

Thank you very much again.

-- 
Takashi Yano <[email protected]>