* DevNotes: Add entry cgf-000011.

* fhandler.h (fhandler_base::refcnt): Delete.
(fhandler_base::inc_refcnt): New function.
(fhandler_base::dec_refcnt): New function.
* cygheap.h (cygheap_fdnew::~cygheap_fdnew): Accommodate split of refcnt to
inc_refcnt/dec_refcnt.
(cygheap_fdget::cygheap_fdget): Ditto.
(cygheap_fdget::~cygheap_fdget::cygheap_fdget): Ditto.
* dtable.cc (dtable::release): Ditto.
(cygwin_attach_handle_to_fd): Ditto.
(dtable::init_std_file_from_handle): Ditto.
(dtable::dup3): On success, return with fdtab locked.
* dtable.h (dtable): Add dup_finish as a friend.
* syscalls.cc (dup_finish): Define new function.  Increment refcnt while fdtab
is locked.
(dup2): Use common dup_finish() to perform dup operation.
(dup3): Ditto.

7 files changed, 74 insertions, 20 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 02e6931bf..39247a970 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,23 @@
+2012-06-03  Christopher Faylor  <me.cygwin2012@cgf.cx>
+
+	* DevNotes: Add entry cgf-000011.
+	* fhandler.h (fhandler_base::refcnt): Delete.
+	(fhandler_base::inc_refcnt): New function.
+	(fhandler_base::dec_refcnt): New function.
+	* cygheap.h (cygheap_fdnew::~cygheap_fdnew): Accommodate split of
+	refcnt to inc_refcnt/dec_refcnt.
+	(cygheap_fdget::cygheap_fdget): Ditto.
+	(cygheap_fdget::~cygheap_fdget::cygheap_fdget): Ditto.
+	* dtable.cc (dtable::release): Ditto.
+	(cygwin_attach_handle_to_fd): Ditto.
+	(dtable::init_std_file_from_handle): Ditto.
+	(dtable::dup3): On success, return with fdtab locked.
+	* dtable.h (dtable): Add dup_finish as a friend.
+	* syscalls.cc (dup_finish): Define new function.  Increment refcnt
+	while fdtab is locked.
+	(dup2): Use common dup_finish() to perform dup operation.
+	(dup3): Ditto.
+
 2012-06-03  Corinna Vinschen  <corinna@vinschen.de>
 
 	* globals.cc (ro_u_refs): New R/O unicode string.
diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes
index 778a5d059..a311256e2 100644
--- a/winsup/cygwin/DevNotes
+++ b/winsup/cygwin/DevNotes
@@ -1,3 +1,28 @@
+2012-06-02  cgf-000011
+
+The refcnt handling was tricky to get right but I had convinced myself
+that the refcnt's were always incremented/decremented under a lock.
+Corinna's 2012-05-23 change to refcnt exposed a potential problem with
+dup handling where the fdtab could be updated while not locked.
+
+That should be fixed by this change but, on closer examination, it seems
+ilke there are many places where it is possible for the refcnt to be
+updated while the fdtab is not locked since the default for
+cygheap_fdget is to not lock the fdtab (and that should be the default -
+you can't have read holding a lock).
+
+Since refcnt was only ever called with 1 or -1, I broke it up into two
+functions but kept the Interlocked* operation.  Incrementing a variable
+should not be as racy as adding an arbitrary number to it but we have
+InterlockedIncrement/InterlockedDecrement for a reason so I kept the
+Interlocked operation here.
+
+In the meantime, I'll be mulling over whether the refcnt operations are
+actually safe as they are.  Maybe just ensuring that they are atomically
+updated is enough since they control the destruction of an fh.  If I got
+the ordering right with incrementing and decrementing then that should
+be adequate.
+
 2012-06-02  cgf-000010
 
 <1.7.16>
diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h
index 4895019aa..f2b0cd9ec 100644
--- a/winsup/cygwin/cygheap.h
+++ b/winsup/cygwin/cygheap.h
@@ -458,7 +458,7 @@ class cygheap_fdnew : public cygheap_fdmanip
   ~cygheap_fdnew ()
   {
     if (cygheap->fdtab[fd])
-      cygheap->fdtab[fd]->refcnt (1);
+      cygheap->fdtab[fd]->inc_refcnt ();
   }
   void operator = (fhandler_base *fh) {cygheap->fdtab[fd] = fh;}
 };
@@ -476,7 +476,7 @@ public:
 	this->fd = fd;
 	locked = lockit;
 	fh = cygheap->fdtab[fd];
-	fh->refcnt (1);
+	fh->inc_refcnt ();
       }
     else
       {
@@ -491,7 +491,7 @@ public:
   }
   ~cygheap_fdget ()
   {
-    if (fh && fh->refcnt (-1) <= 0)
+    if (fh && fh->dec_refcnt () <= 0)
       {
 	debug_only_printf ("deleting fh %p", fh);
 	delete fh;
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index fc3a502f9..059d782d0 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -243,7 +243,7 @@ dtable::release (int fd)
 {
   if (fds[fd]->need_fixup_before ())
     dec_need_fixup_before ();
-  fds[fd]->refcnt (-1);
+  fds[fd]->dec_refcnt ();
   fds[fd] = NULL;
   if (fd <= 2)
     set_std_handle (fd);
@@ -259,7 +259,7 @@ cygwin_attach_handle_to_fd (char *name, int fd, HANDLE handle, mode_t bin,
   if (!fh)
     return -1;
   cygheap->fdtab[fd] = fh;
-  cygheap->fdtab[fd]->refcnt (1);
+  cygheap->fdtab[fd]->inc_refcnt ();
   fh->init (handle, myaccess, bin ?: fh->pc_binmode ());
   return fd;
 }
@@ -398,7 +398,7 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
       fh->open_setup (openflags);
       fh->usecount = 0;
       cygheap->fdtab[fd] = fh;
-      cygheap->fdtab[fd]->refcnt (1);
+      cygheap->fdtab[fd]->inc_refcnt ();
       set_std_handle (fd);
       paranoid_printf ("fd %d, handle %p", fd, handle);
     }
@@ -713,6 +713,7 @@ dtable::dup3 (int oldfd, int newfd, int flags)
   MALLOC_CHECK;
   debug_printf ("dup3 (%d, %d, %p)", oldfd, newfd, flags);
   lock ();
+  bool do_unlock = true;
 
   if (not_open (oldfd))
     {
@@ -760,6 +761,7 @@ dtable::dup3 (int oldfd, int newfd, int flags)
       goto done;
     }
 
+  do_unlock = false;
   fds[newfd] = newfh;
 
   if ((res = newfd) <= 2)
@@ -767,7 +769,8 @@ dtable::dup3 (int oldfd, int newfd, int flags)
 
 done:
   MALLOC_CHECK;
-  unlock ();
+  if (do_unlock)
+    unlock ();
   syscall_printf ("%R = dup3(%d, %d, %p)", res, oldfd, newfd, flags);
 
   return res;
diff --git a/winsup/cygwin/dtable.h b/winsup/cygwin/dtable.h
index 288b4767f..7b4cd5d62 100644
--- a/winsup/cygwin/dtable.h
+++ b/winsup/cygwin/dtable.h
@@ -89,6 +89,7 @@ public:
   void fixup_before_fork (DWORD win_proc_id);
   friend void dtable_init ();
   friend void __stdcall close_all_files (bool);
+  friend int dup_finish (int, int, int);
   friend class fhandler_disk_file;
   friend class cygheap_fdmanip;
   friend class cygheap_fdget;
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 84f6e4c3f..72f8d8429 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -182,15 +182,8 @@ class fhandler_base
   HANDLE read_state;
 
  public:
-  long refcnt(long i = 0)
-  {
-    debug_only_printf ("%p, %s, i %d, refcnt %ld", this, get_name (), i, _refcnt + i);
-    /* This MUST be an interlocked operation.  If multiple threads access the
-       same descriptor in quick succession, a context switch can interrupt
-       the += operation and we wrongly end up with a refcnt of 0 in the
-       cygheap_fdget destructor. */
-    return i ? InterlockedAdd (&_refcnt, i) : _refcnt;
-  }
+  long inc_refcnt () {return InterlockedIncrement (&_refcnt);}
+  long dec_refcnt () {return InterlockedDecrement (&_refcnt);} 
   class fhandler_base *archetype;
   int usecount;
 
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 657616ad9..3c0a02671 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -126,6 +126,18 @@ dup (int fd)
   return res;
 }
 
+inline int
+dup_finish (int oldfd, int newfd, int flags)
+{
+  int res;
+  if ((res = cygheap->fdtab.dup3 (oldfd, newfd, flags)) == newfd)
+    {
+      cygheap_fdget (newfd)->inc_refcnt ();
+      cygheap->fdtab.unlock ();	/* dup3 exits with lock set on success */
+    }
+  return res;
+}
+
 extern "C" int
 dup2 (int oldfd, int newfd)
 {
@@ -140,8 +152,8 @@ dup2 (int oldfd, int newfd)
       cygheap_fdget cfd (oldfd);
       res = (cfd >= 0) ? oldfd : -1;
     }
-  else if ((res = cygheap->fdtab.dup3 (oldfd, newfd, 0)) == newfd)
-    cygheap->fdtab[newfd]->refcnt (1);
+  else
+    res = dup_finish (oldfd, newfd, 0);
 
   syscall_printf ("%R = dup2(%d, %d)", res, oldfd, newfd);
   return res;
@@ -162,8 +174,8 @@ dup3 (int oldfd, int newfd, int flags)
       set_errno (cfd < 0 ? EBADF : EINVAL);
       res = -1;
     }
-  else if ((res = cygheap->fdtab.dup3 (oldfd, newfd, flags)) == newfd)
-    cygheap->fdtab[newfd]->refcnt (1);
+  else
+    res = dup_finish (oldfd, newfd, flags);
 
   syscall_printf ("%R = dup3(%d, %d, %p)", res, oldfd, newfd, flags);
   return res;