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

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.
>=20
> 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[]`.

>=20
> 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(-)
>=20
> 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] !=3D FDTAB_RESERVED)
> +    {
> +      if (fds[fd]->need_fixup_before ())
> +	dec_need_fixup_before ();
> +      fds[fd]->dec_refcnt ();
> +    }
>    fds[fd] =3D NULL;
>    if (fd <=3D 2)
>      set_std_handle (fd);
> @@ -267,7 +270,7 @@ cygwin_attach_handle_to_fd (char *name, int fd, HAND=
LE handle, mode_t bin,
>      fd =3D -1;
>    else
>      {
> -      cygheap->fdtab[fd] =3D 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 ha=
ndle)
>        if (!fh->open_setup (openflags))
>  	api_fatal ("open_setup failed, %E");
>        fh->usecount =3D 0;
> -      cygheap->fdtab[fd] =3D 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/loca=
l_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_pi=
pe *> (cygheap->fdtab[fd]);}
> -  void operator =3D (fhandler_base *fh) {cygheap->fdtab[fd] =3D fh;}
> +  void operator =3D (fhandler_base *fh) {cygheap->fdtab.set_fhandler (f=
d, 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 =3D (fhandler_base *fh) {cygheap->fdtab[fd] =3D fh;}
> +  void operator =3D (fhandler_base *fh) {cygheap->fdtab.set_fhandler (f=
d, fh);}
>  };
> =20
>  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;
> =20
>  #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 =3D 8;
>    size_t first_fd_for_open;
>    int cnt_need_fixup_before;
> +  fhandler_base * const null_fds;
>  public:
>    size_t size;
> =20
> -  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 =3D 3;}
> =20
>    void dec_need_fixup_before ()
> @@ -51,7 +54,8 @@ public:
>    inline int not_open (int fd)
>    {
>      lock ();
> -    int res =3D fd < 0 || fd >=3D (int) size || fds[fd] =3D=3D NULL;
> +    int res =3D fd < 0 || fd >=3D (int) size
> +      || fds[fd] =3D=3D NULL || fds[fd] =3D=3D 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] =3D fh;=
}
> +  inline fhandler_base *operator [](int fd) const
> +  {
> +    return (fds[fd] =3D=3D 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.

Ciao,
Johannes

>    bool select_read (int fd, select_stuff *);
>    bool select_write (int fd, select_stuff *);
>    bool select_except (int fd, select_stuff *);
> @@ -76,6 +84,7 @@ public:
>    void fixup_before_fork (DWORD win_proc_id);
>    void lock () {lock_process::locker.acquire ();}
>    void unlock () {lock_process::locker.release ();}
> +  void reserve (int fd) { fds[fd] =3D FDTAB_RESERVED; }
>  };
> =20
>  fhandler_base *build_fh_dev (const device&, const char * =3D NULL);
> diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
> index 5465d6c09..af7471a7f 100644
> --- a/winsup/cygwin/syscalls.cc
> +++ b/winsup/cygwin/syscalls.cc
> @@ -1558,8 +1558,8 @@ open (const char *unix_path, int flags, ...)
>  	  cygheap->fdtab.unlock ();
>  	  __leave;		/* errno already set */
>  	}
> -      cygheap->fdtab[fd] =3D fh; /* tentative setting to mark as used *=
/
> -      cygheap->fdtab.unlock();
> +      cygheap->fdtab.reserve (fd);
> +      cygheap->fdtab.unlock ();
> =20
>        if (fh->dev () =3D=3D FH_PROCESSFD && fh->pc.follow_fd_symlink ()=
)
>  	{
> @@ -1588,7 +1588,7 @@ open (const char *unix_path, int flags, ...)
>  		    FILE_OPEN_FOR_BACKUP_INTENT);
> =20
>        cygheap->fdtab.lock ();
> -      cygheap->fdtab[fd] =3D fh;
> +      cygheap->fdtab.set_fhandler (fd, fh);
>        fh->inc_refcnt ();
>        cygheap->fdtab.unlock ();
> =20
> @@ -1601,7 +1601,7 @@ open (const char *unix_path, int flags, ...)
>      if (res < 0 && fd >=3D 0)
>        {
>  	cygheap->fdtab.lock ();
> -	cygheap->fdtab[fd] =3D NULL; /* Mark as unused */
> +	cygheap->fdtab.release (fd);
>  	cygheap->fdtab.unlock ();
>        }
>    if (res < 0 && fh)
> --=20
> 2.51.0
>=20
>=20