Cygwin: FIFO: fix problems finding new owner

When the owning reader closes and there are still readers open, the
owner needs to wait for a new owner to be found before closing its
fifo_client handlers.  This involves a loop in which dec_nreaders is
called at the beginning and inc_nreaders is called at the end.  Any
other reader that tries to access shmem->_nreaders during this loop
will therefore get an inaccurate answer.

Fix this by adding an nreaders method and using it instead of
dec_nreaders and inc_nreaders.  Also add nreaders_lock to control
access to the shmem->_nreaders.

Make various other changes to improve the reliability of finding a new
owner.

2 files changed, 61 insertions, 33 deletions

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 7a28adc16..cf6daea06 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1328,7 +1328,7 @@ class fifo_shmem_t
 {
   LONG _nreaders;
   fifo_reader_id_t _owner, _prev_owner, _pending_owner;
-  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock;
+  af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, _nreaders_lock;
 
   /* Info about shared memory block used for temporary storage of the
      owner's fc_handler list. */
@@ -1336,6 +1336,7 @@ class fifo_shmem_t
     _sh_fc_handler_updated;
 
 public:
+  int nreaders () const { return (int) _nreaders; }
   int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); }
   int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); }
 
@@ -1352,6 +1353,8 @@ public:
   void reading_unlock () { _reading_lock.unlock (); }
   void reader_opening_lock () { _reader_opening_lock.lock (); }
   void reader_opening_unlock () { _reader_opening_lock.unlock (); }
+  void nreaders_lock () { _nreaders_lock.lock (); }
+  void nreaders_unlock () { _nreaders_lock.unlock (); }
 
   int get_shared_nhandlers () const { return (int) _sh_nhandlers; }
   void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); }
@@ -1420,8 +1423,11 @@ class fhandler_fifo: public fhandler_base
   int reopen_shared_fc_handler ();
   int remap_shared_fc_handler (size_t);
 
+  int nreaders () const { return shmem->nreaders (); }
   int inc_nreaders () { return shmem->inc_nreaders (); }
   int dec_nreaders () { return shmem->dec_nreaders (); }
+  void nreaders_lock () { shmem->nreaders_lock (); }
+  void nreaders_unlock () { shmem->nreaders_unlock (); }
 
   fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); }
   void set_prev_owner (fifo_reader_id_t fr_id)
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 3d34cdfab..2d4f7a97e 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -371,6 +371,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
 int
 fhandler_fifo::update_my_handlers ()
 {
+  int ret = 0;
+
   close_all_handlers ();
   fifo_reader_id_t prev = get_prev_owner ();
   if (!prev)
@@ -387,7 +389,7 @@ fhandler_fifo::update_my_handlers ()
     {
       debug_printf ("Can't open process of previous owner, %E");
       __seterrno ();
-      return -1;
+      goto out;
     }
 
   for (int i = 0; i < get_shared_nhandlers (); i++)
@@ -402,11 +404,13 @@ fhandler_fifo::update_my_handlers ()
 	  debug_printf ("Can't duplicate handle of previous owner, %E");
 	  --nhandlers;
 	  __seterrno ();
-	  return -1;
+	  goto out;
 	}
       fc.state = shared_fc_handler[i].state;
     }
-  return 0;
+out:
+  set_prev_owner (null_fr_id);
+  return ret;
 }
 
 int
@@ -1414,41 +1418,59 @@ fhandler_fifo::close ()
 {
   if (reader)
     {
-      /* If we're the owner, try to find a new owner. */
-      bool find_new_owner = false;
+      /* If we're the owner, we can't close our fc_handlers if a new
+	 owner might need to duplicate them. */
+      bool close_fc_ok = false;
 
       cancel_reader_thread ();
-      owner_lock ();
-      if (get_owner () == me)
+      nreaders_lock ();
+      if (dec_nreaders () == 0)
 	{
-	  find_new_owner = true;
+	  close_fc_ok = true;
+	  ResetEvent (read_ready);
+	  ResetEvent (owner_needed_evt);
+	  ResetEvent (owner_found_evt);
 	  set_owner (null_fr_id);
-	  set_prev_owner (me);
-	  owner_needed ();
+	  set_prev_owner (null_fr_id);
+	  set_pending_owner (null_fr_id);
+	  set_shared_nhandlers (0);
 	}
-      owner_unlock ();
-      if (dec_nreaders () == 0)
-	ResetEvent (read_ready);
-      else if (find_new_owner && !IsEventSignalled (owner_found_evt))
+      else
 	{
-	  bool found = false;
-	  do
-	    if (dec_nreaders () >= 0)
-	      {
-		/* There's still another reader open. */
-		if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
-		  found = true;
-		else
-		  {
-		    owner_lock ();
-		    if (get_owner ()) /* We missed owner_found_evt? */
-		      found = true;
-		    else
-		      owner_needed ();
-		    owner_unlock ();
-		  }
-	      }
-	  while (inc_nreaders () > 0 && !found);
+	  owner_lock ();
+	  if (get_owner () != me)
+	    close_fc_ok = true;
+	  else
+	    {
+	      set_owner (null_fr_id);
+	      set_prev_owner (me);
+	      if (!get_pending_owner ())
+		owner_needed ();
+	    }
+	  owner_unlock ();
+	}
+      nreaders_unlock ();
+      while (!close_fc_ok)
+	{
+	  if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0)
+	    close_fc_ok = true;
+	  else
+	    {
+	      nreaders_lock ();
+	      if (!nreaders ())
+		{
+		  close_fc_ok = true;
+		  nreaders_unlock ();
+		}
+	      else
+		{
+		  nreaders_unlock ();
+		  owner_lock ();
+		  if (get_owner () || get_prev_owner () != me)
+		    close_fc_ok = true;
+		  owner_unlock ();
+		}
+	    }
 	}
       close_all_handlers ();
       if (fc_handler)