[newlib-cygwin] Cygwin: FIFO: don't leave a pending listen request

[newlib-cygwin] Cygwin: FIFO: don't leave a pending listen request

[Ken Brown]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=c12053a793246742f7c7a177b9e7656c602c52d2

commit c12053a793246742f7c7a177b9e7656c602c52d2
Author: Ken Brown <kbrown@cornell.edu>
Date:   Thu May 9 11:55:30 2019 -0400

    Cygwin: FIFO: don't leave a pending listen request
    
    On exit from the listen_client thread, make sure there's no pending
    FSCTL_PIPE_LISTEN request.  Otherwise we might get a client connection
    after restarting the thread, and we won't have a handle for
    communicating with that client.
    
    Remove the retry loop in the case of STATUS_PIPE_LISTENING; that case
    shouldn't occur.
    
    Remove the now-unused fc_connecting value from
    fifo_client_connect_state.

Diff:
---
 winsup/cygwin/fhandler.h       |   1 -
 winsup/cygwin/fhandler_fifo.cc | 111 ++++++++++++++++++++++-------------------
 2 files changed, 59 insertions(+), 53 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 683aae1..f4c0f03 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1240,7 +1240,6 @@ public:
 enum fifo_client_connect_state
   {
    fc_unknown,
-   fc_connecting,
    fc_connected,
    fc_invalid
   };
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index 4d05727..0d4a8b8 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -272,10 +272,7 @@ listen_client_func (LPVOID param)
   return fh->listen_client_thread ();
 }
 
-/* Start a thread that listens for client connections.  Whenever a new
-   client connects, it creates a new pipe_instance if necessary.
-   (There may already be an available instance if a client has
-   disconnected.)  */
+/* Start a thread that listens for client connections. */
 bool
 fhandler_fifo::listen_client ()
 {
@@ -331,70 +328,80 @@ fhandler_fifo::listen_client_thread ()
 
       /* Create a new client handler. */
       if (add_client_handler () < 0)
-	goto out;
+	{
+	  fifo_client_unlock ();
+	  goto out;
+	}
 
       /* Allow a writer to open. */
       if (!arm (read_ready))
 	{
+	  fifo_client_unlock ();
 	  __seterrno ();
 	  goto out;
 	}
-      fifo_client_unlock ();
 
       /* Listen for a writer to connect to the new client handler. */
       fifo_client_handler& fc = fc_handler[nhandlers - 1];
-      do
-	{
-	  NTSTATUS status;
-	  IO_STATUS_BLOCK io;
+      NTSTATUS status;
+      IO_STATUS_BLOCK io;
 
-	  status = NtFsControlFile (fc.fh->get_handle (), fc.connect_evt,
-				    NULL, NULL, &io, FSCTL_PIPE_LISTEN,
-				    NULL, 0, NULL, 0);
-	  if (status == STATUS_PENDING)
-	    {
-	      HANDLE w[2] = { fc.connect_evt, lct_termination_evt };
-	      DWORD waitret = WaitForMultipleObjects (2, w, false, INFINITE);
-	      switch (waitret)
-		{
-		case WAIT_OBJECT_0:
-		  status = io.Status;
-		  break;
-		case WAIT_OBJECT_0 + 1:
-		  ret = 0;
-		  status = STATUS_THREAD_IS_TERMINATING;
-		  break;
-		default:
-		  __seterrno ();
-		  debug_printf ("WaitForMultipleObjects failed, %E");
-		  status = STATUS_THREAD_IS_TERMINATING;
-		  break;
-		}
-	    }
-	  switch (status)
+      status = NtFsControlFile (fc.fh->get_handle (), fc.connect_evt,
+				NULL, NULL, &io, FSCTL_PIPE_LISTEN,
+				NULL, 0, NULL, 0);
+      fifo_client_unlock ();
+      if (status == STATUS_PENDING)
+	{
+	  HANDLE w[2] = { fc.connect_evt, lct_termination_evt };
+	  DWORD waitret = WaitForMultipleObjects (2, w, false, INFINITE);
+	  switch (waitret)
 	    {
-	    case STATUS_SUCCESS:
-	    case STATUS_PIPE_CONNECTED:
-	      record_connection (fc);
+	    case WAIT_OBJECT_0:
+	      status = io.Status;
 	      break;
-	    case STATUS_PIPE_LISTENING:
-	      /* Retry. */
-	      fc.state = fc_connecting;
-	      ResetEvent (fc.connect_evt);
+	    case WAIT_OBJECT_0 + 1:
+	      ret = 0;
+	      status = STATUS_THREAD_IS_TERMINATING;
 	      break;
-	    case STATUS_THREAD_IS_TERMINATING:
-	      fifo_client_lock ();
-	      delete_client_handler (nhandlers - 1);
-	      fifo_client_unlock ();
-	      goto out;
 	    default:
-	      __seterrno_from_nt_status (status);
-	      fifo_client_lock ();
-	      delete_client_handler (nhandlers - 1);
-	      fifo_client_unlock ();
-	      goto out;
+	      __seterrno ();
+	      debug_printf ("WaitForMultipleObjects failed, %E");
+	      status = STATUS_THREAD_IS_TERMINATING;
+	      break;
 	    }
-	} while (fc.state == fc_connecting);
+	}
+      HANDLE ph = NULL;
+      switch (status)
+	{
+	case STATUS_SUCCESS:
+	case STATUS_PIPE_CONNECTED:
+	  record_connection (fc);
+	  break;
+	case STATUS_THREAD_IS_TERMINATING:
+	  /* Try to cancel the pending listen.  Otherwise the first
+	     writer to connect after the thread is restarted will be
+	     invisible.
+
+	     FIXME: Is there a direct way to do this?  We do it by
+	     opening and closing a write handle to the client side. */
+	  open_pipe (ph);
+	  /* We don't care about the return value of open_pipe.  Even
+             if the latter failed, a writer might have connected. */
+	  if (WaitForSingleObject (fc.connect_evt, 0) == WAIT_OBJECT_0
+	      && (NT_SUCCESS (io.Status) || io.Status == STATUS_PIPE_CONNECTED))
+	    record_connection (fc);
+	  else
+	    fc.state = fc_invalid;
+	  /* By closing ph we ensure that if fc connected to ph, fc
+	     will be declared invalid on the next read attempt. */
+	  if (ph)
+	    CloseHandle (ph);
+	  goto out;
+	default:
+	  __seterrno_from_nt_status (status);
+	  fc.state = fc_invalid;
+	  goto out;
+	}
       /* Check for thread termination in case WaitForMultipleObjects
 	 didn't get called above. */
       if (IsEventSignalled (lct_termination_evt))