Cygwin: FIFO: synchronize the fifo_reader and fifosel threads

The fifo_reader thread function and the function select.cc:peek_fifo()
can both change the state of a fifo_client_handler.  These changes are
made under fifo_client_lock, so there is no race, but the changes can
still be incompatible.

Add code to make sure that only one of these functions can change the
state from its initial fc_listening state.  Whichever function does
this calls the fhandler_fifo::record_connection method, which is now
public so that peek_fifo can call it.

Slightly modify that method to make it suitable for being called by
peek_fifo.

Make a few other small changes to the fifo_reader thread function to
change how it deals with the STATUS_PIPE_CLOSING value that can
(rarely) be returned by NtFsControlFile.

Add commentary to fhandler_fifo.cc to explain fifo_client connect
states and where they can be changed.

3 files changed, 60 insertions, 9 deletions

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 40e201b0f..a577ca542 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1413,8 +1413,6 @@ class fhandler_fifo: public fhandler_base
   void cleanup_handlers ();
   void close_all_handlers ();
   void cancel_reader_thread ();
-  void record_connection (fifo_client_handler&,
-			  fifo_client_connect_state = fc_connected);
 
   int create_shmem (bool only_open = false);
   int reopen_shmem ();
@@ -1482,6 +1480,8 @@ public:
   DWORD fifo_reader_thread_func ();
   void fifo_client_lock () { _fifo_client_lock.lock (); }
   void fifo_client_unlock () { _fifo_client_lock.unlock (); }
+  void record_connection (fifo_client_handler&, bool = true,
+			  fifo_client_connect_state = fc_connected);
 
   int take_ownership (DWORD timeout = INFINITE);
   void reading_lock () { shmem->reading_lock (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 1e1140f53..2b829eb6c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -37,11 +37,42 @@
      "fifo_client_handler" structures, one for each writer.  A
      fifo_client_handler contains the handle for the pipe server
      instance and information about the state of the connection with
-     the writer.  Each writer holds the pipe instance's client handle.
+     the writer.  Access to the list is controlled by a
+     "fifo_client_lock".
 
      The reader runs a "fifo_reader_thread" that creates new pipe
      instances as needed and listens for client connections.
 
+     The connection state of a fifo_client_handler has one of the
+     following values, in which order is important:
+
+       fc_unknown
+       fc_error
+       fc_disconnected
+       fc_closing
+       fc_listening
+       fc_connected
+       fc_input_avail
+
+     It can be changed in the following places:
+
+       - It is set to fc_listening when the pipe instance is created.
+
+       - It is set to fc_connected when the fifo_reader_thread detects
+         a connection.
+
+       - It is set to a value reported by the O/S when
+         query_and_set_state is called.  This can happen in
+         select.cc:peek_fifo and a couple other places.
+
+       - It is set to fc_disconnected by raw_read when an attempt to
+         read yields STATUS_PIPE_BROKEN.
+
+       - It is set to fc_error in various places when unexpected
+         things happen.
+
+     State changes are always guarded by fifo_client_lock.
+
      If there are multiple readers open, only one of them, called the
      "owner", maintains the fifo_client_handler list.  The owner is
      therefore the only reader that can read at any given time.  If a
@@ -374,10 +405,11 @@ fhandler_fifo::cleanup_handlers ()
 
 /* Always called with fifo_client_lock in place. */
 void
-fhandler_fifo::record_connection (fifo_client_handler& fc,
+fhandler_fifo::record_connection (fifo_client_handler& fc, bool set,
 				  fifo_client_connect_state s)
 {
-  fc.set_state (s);
+  if (set)
+    fc.set_state (s);
   set_pipe_non_blocking (fc.h, true);
 }
 
@@ -583,6 +615,11 @@ owner_listen:
       NTSTATUS status1;
 
       fifo_client_lock ();
+      if (fc.get_state () != fc_listening)
+	/* select.cc:peek_fifo has already recorded a connection. */
+	;
+      else
+      {
       switch (status)
 	{
 	case STATUS_SUCCESS:
@@ -590,7 +627,12 @@ owner_listen:
 	  record_connection (fc);
 	  break;
 	case STATUS_PIPE_CLOSING:
-	  record_connection (fc, fc_closing);
+	  debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING...");
+	  /* Maybe a writer already connected, wrote, and closed.
+	     Just query the O/S. */
+	  fc.query_and_set_state ();
+	  debug_printf ("...O/S reports state %d", fc.get_state ());
+	  record_connection (fc, false);
 	  break;
 	case STATUS_THREAD_IS_TERMINATING:
 	case STATUS_WAIT_1:
@@ -610,17 +652,23 @@ owner_listen:
 		record_connection (fc);
 		break;
 	      case STATUS_PIPE_CLOSING:
-		record_connection (fc, fc_closing);
+		debug_printf ("got STATUS_PIPE_CLOSING when trying to connect bogus client...");
+		fc.query_and_set_state ();
+		debug_printf ("...O/S reports state %d", fc.get_state ());
+		record_connection (fc, false);
 		break;
 	      default:
 		debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status);
-		fc.set_state (fc_unknown);
+		fc.set_state (fc_error);
 		break;
 	      }
 	  break;
 	default:
+	  debug_printf ("NtFsControlFile got unexpected status %y", status);
+	  fc.set_state (fc_error);
 	  break;
 	}
+      }
       if (ph)
 	NtClose (ph);
       if (update)
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 9ee305f64..43f07af43 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -877,10 +877,13 @@ peek_fifo (select_record *s, bool from_select)
       for (int i = 0; i < fh->get_nhandlers (); i++)
 	{
 	  fifo_client_handler &fc = fh->get_fc_handler (i);
-	  fc.query_and_set_state ();
+	  fifo_client_connect_state prev_state = fc.query_and_set_state ();
 	  if (fc.get_state () >= fc_connected)
 	    {
 	      nconnected++;
+	      if (prev_state == fc_listening)
+		/* The connection was not recorded by the fifo_reader_thread. */
+		fh->record_connection (fc, false);
 	      if (fc.get_state () == fc_input_avail)
 		{
 		  select_printf ("read: %s, ready for read", fh->get_name ());