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

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]);
   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))
     /* 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;
+    }
+    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