From fc611be411e1d380f1258bb0b225d99757b4ef59 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 12 Sep 2021 19:59:53 +0900
Subject: [PATCH] Cygwin: pipe: Return to query_hdl strategy with counting r/w
 handles.

---
 winsup/cygwin/fhandler.h       |  5 ++
 winsup/cygwin/fhandler_pipe.cc | 98 ++++++++++++++++++++++++----------
 winsup/cygwin/select.cc        | 50 ++++++++++++++---
 winsup/cygwin/spawn.cc         |  2 +
 4 files changed, 121 insertions(+), 34 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 13fba9a14..f09af2c37 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1176,10 +1176,15 @@ class fhandler_pipe_fifo: public fhandler_base
 {
  protected:
   size_t pipe_buf_size;
+  HANDLE query_hdl;
 
  public:
   fhandler_pipe_fifo ();
 
+  HANDLE get_query_handle () const { return query_hdl; }
+  void close_query_handle () { CloseHandle (query_hdl); query_hdl = NULL; }
+  bool reader_closed ();
+
   ssize_t __reg3 raw_write (const void *ptr, size_t len);
 
 };
diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
index 9b4255cfd..b051f5c03 100644
--- a/winsup/cygwin/fhandler_pipe.cc
+++ b/winsup/cygwin/fhandler_pipe.cc
@@ -56,6 +56,8 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
   fpi.ReadMode = FILE_PIPE_BYTE_STREAM_MODE;
   fpi.CompletionMode = nonblocking ? FILE_PIPE_COMPLETE_OPERATION
     : FILE_PIPE_QUEUE_OPERATION;
+  if (query_hdl)
+    fpi.CompletionMode = FILE_PIPE_COMPLETE_OPERATION;
   status = NtSetInformationFile (get_handle (), &io, &fpi, sizeof fpi,
 				 FilePipeInformation);
   if (!NT_SUCCESS (status))
@@ -202,6 +204,8 @@ fhandler_pipe::open_setup (int flags)
       if (!read_mtx)
 	debug_printf ("CreateMutex failed: %E");
     }
+  if (get_dev () == FH_PIPEW && !query_hdl)
+    set_pipe_non_blocking (is_nonblocking ());
 }
 
 off_t
@@ -268,39 +272,22 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
   while (nbytes < len)
     {
       ULONG_PTR nbytes_now = 0;
-      size_t left = len - nbytes;
-      ULONG len1 = (ULONG) left;
+      ULONG len1 = (ULONG) (len - nbytes);
       waitret = WAIT_OBJECT_0;
 
       if (evt)
 	ResetEvent (evt);
-      if (!is_nonblocking ())
+      FILE_PIPE_LOCAL_INFORMATION fpli;
+      status = NtQueryInformationFile (get_handle (), &io,
+				       &fpli, sizeof (fpli),
+				       FilePipeLocalInformation);
+      if (NT_SUCCESS (status))
 	{
-	  FILE_PIPE_LOCAL_INFORMATION fpli;
-
-	  /* If the pipe is empty, don't request more bytes than pipe
-	     buffer size - 1. Pending read lowers WriteQuotaAvailable on
-	     the write side and thus affects select's ability to return
-	     more or less reliable info whether a write succeeds or not. */
-	  ULONG chunk = pipe_buf_size - 1;
-	  status = NtQueryInformationFile (get_handle (), &io,
-					   &fpli, sizeof (fpli),
-					   FilePipeLocalInformation);
-	  if (NT_SUCCESS (status))
-	    {
-	      if (fpli.ReadDataAvailable > 0)
-		chunk = left;
-	      else if (nbytes != 0)
-		break;
-	      else
-		chunk = fpli.InboundQuota - 1;
-	    }
-	  else if (nbytes != 0)
-	    break;
-
-	  if (len1 > chunk)
-	    len1 = chunk;
+	if (fpli.ReadDataAvailable == 0 && nbytes != 0)
+	  break;
 	}
+      else if (nbytes != 0)
+	break;
       status = NtReadFile (get_handle (), evt, NULL, NULL, &io, ptr,
 			   len1, NULL, NULL);
       if (evt && status == STATUS_PENDING)
@@ -385,6 +372,16 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
   len = nbytes;
 }
 
+bool
+fhandler_pipe_fifo::reader_closed ()
+{
+  if (!query_hdl)
+    return false;
+  int n_reader = get_obj_handle_count (query_hdl);
+  int n_writer = get_obj_handle_count (get_handle ());
+  return n_reader == n_writer;
+}
+
 ssize_t __reg3
 fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
 {
@@ -493,7 +490,20 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
 			      get_obj_handle_count (select_sem), NULL);
 	  /* 0 bytes returned?  EAGAIN.  See above. */
 	  if (NT_SUCCESS (status) && nbytes == 0)
-	    set_errno (EAGAIN);
+	    {
+	      if (reader_closed ())
+		{
+		  set_errno (EPIPE);
+		  raise (SIGPIPE);
+		}
+	      else if (is_nonblocking ())
+		set_errno (EAGAIN);
+	      else
+		{
+		  cygwait (select_sem, 10);
+		  continue;
+		}
+	    }
 	}
       else if (STATUS_PIPE_IS_CLOSED (status))
 	{
@@ -522,6 +532,10 @@ fhandler_pipe::fixup_after_fork (HANDLE parent)
     fork_fixup (parent, read_mtx, "read_mtx");
   if (select_sem)
     fork_fixup (parent, select_sem, "select_sem");
+  /* Do not duplicate query_hdl if it has been already inherited. */
+  if (query_hdl && !get_obj_handle_count (query_hdl))
+    fork_fixup (parent, query_hdl, "query_hdl");
+
   fhandler_base::fixup_after_fork (parent);
 }
 
@@ -552,6 +566,15 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
       ftp->close ();
       res = -1;
     }
+  else if (query_hdl &&
+	   !DuplicateHandle (GetCurrentProcess (), query_hdl,
+			    GetCurrentProcess (), &ftp->query_hdl,
+			    0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS))
+    {
+      __seterrno ();
+      ftp->close ();
+      res = -1;
+    }
 
   debug_printf ("res %d", res);
   return res;
@@ -567,6 +590,8 @@ fhandler_pipe::close ()
       ReleaseSemaphore (select_sem, get_obj_handle_count (select_sem), NULL);
       CloseHandle (select_sem);
     }
+  if (query_hdl)
+    CloseHandle (query_hdl);
   return fhandler_base::close ();
 }
 
@@ -791,6 +816,23 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
 	DuplicateHandle (GetCurrentProcess (), fhs[0]->select_sem,
 			 GetCurrentProcess (), &fhs[1]->select_sem,
 			 0, 1, DUPLICATE_SAME_ACCESS);
+      if (!DuplicateHandle (GetCurrentProcess (), r,
+			    GetCurrentProcess (), &fhs[1]->query_hdl,
+			    GENERIC_READ, !(mode & O_CLOEXEC), 0))
+	{
+	  CloseHandle (fhs[0]->select_sem);
+	  delete fhs[0];
+	  CloseHandle (r);
+	  CloseHandle (fhs[1]->select_sem);
+	  delete fhs[1];
+	  CloseHandle (w);
+	}
+      else
+	{
+	  /* Call set_pipe_non_blocking() again after creating query_hdl. */
+	  fhs[1]->set_pipe_non_blocking (fhs[1]->is_nonblocking ());
+	  res = 0;
+	}
     }
 
   debug_printf ("%R = pipe([%p, %p], %d, %y)", res, fhs[0], fhs[1], psize, mode);
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 5e583434c..c569a059c 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -608,12 +608,43 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, bool writing)
     }
   if (writing)
     {
-      /* WriteQuotaAvailable is decremented by the number of bytes requested
-	 by a blocking reader on the other side of the pipe.  Cygwin readers
-	 are serialized and never request a number of bytes equivalent to the
-	 full buffer size.  So WriteQuotaAvailable is 0 only if either the
-	 read buffer on the other side is really full, or if we have non-Cygwin
-	 readers. */
+      /* If there is anything available in the pipe buffer then signal
+        that.  This means that a pipe could still block since you could
+        be trying to write more to the pipe than is available in the
+        buffer but that is the hazard of select().
+
+        Note that WriteQuotaAvailable is unreliable.
+
+        Usually WriteQuotaAvailable on the write side reflects the space
+        available in the inbound buffer on the read side.  However, if a
+        pipe read is currently pending, WriteQuotaAvailable on the write side
+        is decremented by the number of bytes the read side is requesting.
+        So it's possible (even likely) that WriteQuotaAvailable is 0, even
+        if the inbound buffer on the read side is not full.  This can lead to
+        a deadlock situation: The reader is waiting for data, but select
+        on the writer side assumes that no space is available in the read
+        side inbound buffer.
+
+        Consequentially, the only reliable information is available on the
+        read side, so fetch info from the read side via the pipe-specific
+        query handle.  Use fpli.WriteQuotaAvailable as storage for the actual
+        interesting value, which is the OutboundQuote on the write side,
+        decremented by the number of bytes of data in that buffer. */
+      /* Note: Do not use NtQueryInformationFile() for query_hdl because
+	 NtQueryInformationFile() seems to interfere with reading pipes
+	 in non-cygwin apps. Instead, use PeekNamedPipe() here. */
+      if (fh->get_device () == FH_PIPEW)
+	{
+	  HANDLE query_hdl = ((fhandler_pipe *) fh)->get_query_handle ();
+	  if (query_hdl)
+	    {
+	      DWORD nbytes_in_pipe;
+	      PeekNamedPipe (query_hdl, NULL, 0, NULL, &nbytes_in_pipe, NULL);
+	      fpli.WriteQuotaAvailable = fpli.OutboundQuota - nbytes_in_pipe;
+	    }
+	  else
+	    return 1;
+	}
       if (fpli.WriteQuotaAvailable > 0)
 	{
 	  paranoid_printf ("fd %d, %s, write: size %u, avail %u", fd,
@@ -712,6 +743,13 @@ out:
   h = fh->get_output_handle ();
   if (s->write_selected && dev != FH_PIPER)
     {
+      if (dev == FH_PIPEW && ((fhandler_pipe *) fh)->reader_closed ())
+	{
+	  gotone += s->write_ready = true;
+	  if (s->except_selected)
+	    gotone += s->except_ready = true;
+	  return gotone;
+	}
       gotone += s->write_ready =  pipe_data_available (s->fd, fh, h, true);
       select_printf ("write: %s, gotone %d", fh->get_name (), gotone);
     }
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 0bde0b04d..e902b5080 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -657,6 +657,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 		ptys->create_invisible_console ();
 		ptys->setup_locale ();
 	      }
+	    else if (cfd->get_dev () == FH_PIPEW)
+	      ((fhandler_pipe *)(fhandler_base *) cfd)->close_query_handle ();
 	}
 
       bool enable_pcon = false;
-- 
2.33.0

