Re: [PATCH] Cygwin: open: Do not set tentative fhandler to fdtab (B)

Hi Johannes,

Thanks for reviewing!

On Tue, 4 Aug 2026 10:37:32 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> Thank you for implementing both variants. Having them side by side makes
> the trade-off concrete in a way the v2 thread could only gesture at.
> 
> My preference is (B), the same recommendation I made on v2. Concretely,
> this piece:
> 
> On Mon, 3 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.
> 
> The single distinguished value keeps one source of truth in the slot
> itself, so the lock-free `cygheap_fdget` fast path keeps working with just
> one added comparison, and there is no second array to keep consistent.
> (A)'s parallel `bool *reserved` array duplicates the "is this slot spoken
> for" state and has to be kept in lockstep with `fds[]` across `extend()`,
> fork, and exec. That is safe, but it is exactly the "separate flag" shape
> I wanted to avoid. So (B)'s mechanism is the one I want, provided it is
> complete.
> 
> It is not yet complete, though. The most important gap: the sentinel is
> masked _only_ inside `dtable::operator[]`.
> 
> > 
> > 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:
> > ---
> >  winsup/cygwin/dtable.cc                | 13 ++++++++-----
> >  winsup/cygwin/local_includes/cygheap.h |  6 +++---
> >  winsup/cygwin/local_includes/dtable.h  | 15 ++++++++++++---
> >  winsup/cygwin/syscalls.cc              |  8 ++++----
> >  4 files changed, 27 insertions(+), 15 deletions(-)
> > 
> > diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
> > index e4d1cdf8f..fffc9bad5 100644
> > --- a/winsup/cygwin/dtable.cc
> > +++ b/winsup/cygwin/dtable.cc
> > @@ -247,9 +247,12 @@ out:
> >  void
> >  dtable::release (int fd)
> >  {
> > -  if (fds[fd]->need_fixup_before ())
> > -    dec_need_fixup_before ();
> > -  fds[fd]->dec_refcnt ();
> > +  if (fds[fd] && fds[fd] != FDTAB_RESERVED)
> > +    {
> > +      if (fds[fd]->need_fixup_before ())
> > +	dec_need_fixup_before ();
> > +      fds[fd]->dec_refcnt ();
> > +    }
> >    fds[fd] = NULL;
> >    if (fd <= 2)
> >      set_std_handle (fd);
> > @@ -267,7 +270,7 @@ cygwin_attach_handle_to_fd (char *name, int fd, HANDLE handle, mode_t bin,
> >      fd = -1;
> >    else
> >      {
> > -      cygheap->fdtab[fd] = fh;
> > +      cygheap->fdtab.set_fhandler (fd, fh);
> >        cygheap->fdtab[fd]->inc_refcnt ();
> >        fh->init (handle, myaccess, bin ?: fh->pc_binmode ());
> >      }
> > @@ -425,7 +428,7 @@ 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);
> >        cygheap->fdtab[fd]->inc_refcnt ();
> >        set_std_handle (fd);
> >        paranoid_printf ("fd %d, handle %p", fd, handle);
> > 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..910a7e849 100644
> > --- a/winsup/cygwin/local_includes/dtable.h
> > +++ b/winsup/cygwin/local_includes/dtable.h
> > @@ -17,6 +17,7 @@ details. */
> >  class suffix_info;
> >  
> >  #define BFH_OPTS (PC_NULLEMPTY | PC_FULL | PC_POSIX)
> > +#define FDTAB_RESERVED ((fhandler_base *) -1)
> >  class dtable
> >  {
> >    fhandler_base **fds;
> > @@ -26,10 +27,12 @@ class dtable
> >    static const int initial_archetype_size = 8;
> >    size_t first_fd_for_open;
> >    int cnt_need_fixup_before;
> > +  fhandler_base * const null_fds;
> >  public:
> >    size_t size;
> >  
> > -  dtable () : archetypes (NULL), narchetypes (0), farchetype (0), first_fd_for_open(3), cnt_need_fixup_before(0) {}
> > +  dtable () : archetypes (NULL), narchetypes (0), farchetype (0),
> > +  first_fd_for_open(3), cnt_need_fixup_before(0), null_fds (NULL) {}
> >    void init () {first_fd_for_open = 3;}
> >  
> >    void dec_need_fixup_before ()
> > @@ -51,7 +54,8 @@ public:
> >    inline int not_open (int fd)
> >    {
> >      lock ();
> > -    int res = fd < 0 || fd >= (int) size || fds[fd] == NULL;
> > +    int res = fd < 0 || fd >= (int) size
> > +      || fds[fd] == NULL || fds[fd] == FDTAB_RESERVED;
> >      unlock ();
> >      return res;
> >    }
> > @@ -61,7 +65,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[fd] = fh;}
> > +  inline fhandler_base *operator [](int fd) const
> > +  {
> > +    return (fds[fd] == FDTAB_RESERVED) ? null_fds : fds[fd];
> > +  }
> 
> The five fork/exec fixup loops (`set_file_pointers_for_exec`,
> `fixup_before_fork`, `fixup_after_fork`, `fixup_before_exec`, and
> `fixup_after_exec`) read the private `fds[]` member directly and treat any
> non-NULL slot as a live handler. `FDTAB_RESERVED` is `(fhandler_base *)
> -1`, not NULL, so it passes that test and gets dereferenced (the
> `get_flags()` call in `set_file_pointers_for_exec`, for one). That is the
> consumer set I enumerated in the v2 discussion. And it is reachable in
> exactly the window the reservation exists to cover: a FIFO `open()` that
> blocks in `open_with_arch()` with the fdtab lock released, while another
> thread forks or execs. The child- and exec-side fixups run over a copied
> cygheap that still holds the sentinel, and `set_file_pointers_for_exec`,
> `fixup_after_fork`, and `fixup_after_exec` run unconditionally.
> 
> This would make the issues worse than they are right now. Today those
> loops dereference a constructed-but-not-yet-opened `fhandler`: wrong, but
> a valid object. With this patch they dereference `(fhandler_base *) -1`
> outright. The fix is to route every raw `fds[]` iterator through the same
> masking (a small private accessor that maps the sentinel to NULL, used by
> those loops) rather than masking only in `operator[]`.
> 
> And the five loops are not the only way around `operator[]`: `dtable`
> still exposes `operator fhandler_base **()`, handing out the raw `fds`
> array with the sentinel unmasked. Unlike the loops, this one is a latent
> trapdoor, not a live crash _today_; its only caller in the tree is the
> `!cygheap->fdtab` allocated-check in `init_cygheap`, which tests the base
> pointer for NULL and never indexes a slot. It matters anyway: the
> invariant this patch relies on (that every reader is funneled through
> `operator[]`) is not one the class can actually enforce while it publicly
> advertises the raw pointer, so anyone who later iterates `cygheap->fdtab`
> as the array silently gets `(fhandler_base *) -1`. This is really why the
> reservation must be hidden in the storage layer, not masked in a single
> accessor that both the five loops and this conversion sidestep.
> 
> Second, the new `operator[]` reads `fds[fd]` twice, once in the comparison
> and once in the returned value. A NULL-to-reserved transition between the
> two loads hands the sentinel back to a lock-free reader, which is the very
> thing the single distinguished value was meant to preclude (my "no risk of
> a torn read" from before). Reading the slot once into a local and
> comparing that closes the window.
> 
> Third, lower priority because it is shared with (A): explicit-target
> writers still overwrite a reservation (`dup2`/`dup3`, and
> `cygwin_attach_handle_to_fd` with an explicit fd) because `not_open()`
> reports a reserved slot as free. This is only reachable by targeting an fd
> number that `open()` has not yet returned, so it is not a blocker, but it
> is worth a guard or at least a comment.
> 
> Two nits, neither blocking. The error-cleanup path now goes through
> `release()`, which additionally calls `set_std_handle()` for the standard
> descriptors, a side effect the previous bare assignment to NULL did not
> have; a dedicated unreserve helper would avoid it. And `null_fds` is
> unnecessary now that `operator[]` returns by value; returning NULL
> directly after a single load is simpler.
> 
> For the record, the rest of the conversion from returning a reference to
> returning by value _is_ complete: every former assignment through
> `fdtab[fd]` was converted to `set_fhandler()`, so that part is solid.
> 
> Net: (B) is the design I would strongly prefer. I would like to see it
> land once the raw `fds[]` readers are covered and the double read is
> collapsed to a single load.

Ive posted the v2 patch with the issues you pointed out fixed.
Id appreciate a re-review.

Thanks in advance.

-- 
Takashi Yano <[email protected]>