[PATCH v4.2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut

Ken Brown kbrown@cornell.edu
Sat Sep 14 20:09:19 GMT 2024


On 9/6/2024 10:47 PM, Takashi Yano wrote:

defaut should be default in the subject.

> Previously, cygwin read pipe used non-blocking mode althogh non-
                                                       although

> cygwin app uses blocking-mode by default. Despite this requirement,
> if a cygwin app is executed from a non-cygwwin app and the cygwin
                                           cygwin

> app exits, read pipe remains on non-blocking mode because of the
> commit fc691d0246b9. Due to this behaviour, the non-cygwin app
> cannot read the pipe correctly after that. Similarly, if a non-
> cygwin app is executed from a cygwin app and the non-cygwin app
> exits, the read pipe mode remains on blocking mode although cygwin
> read pipe should be non-blocking mode.
> 
> These bugs were provoked by pipe mode toggling between cygwin and
> non-cygwin apps. To make management of pipe mode simpler, this
> patch has re-designed the pipe implementation. In this new
> implementation, both read and wrie pipe basically use only blocking
                                write

> mode and the behaviour corresponding to the pipe mode is simulated
> in raw_read() and raw_write(). Only when NtQueryInformationFile(
                                           put the ( on the next line

> FilePipeLocalInformation) fails for some reasons, the raw_write()
> cannot simulate non-blocking access. Therefore, the pipe mode is
> temporarily changed to non-blocking mode.
> 
> Moreover, because the fact that NtSetInformationFile() in
> set_pipe_non_blocking(true) fails with STATUS_PIPE_BUSY if the pipe
> is not empty has been founhd, query handle is not necessary anymore.
                         found

> This allows the implementation much simpler than before.

Yes.  Great work!

> 
> Addresses: https://github.com/git-for-windows/git/issues/5115
> Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
> Reported-by: isaacag, Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>   winsup/cygwin/dtable.cc                 |   5 +-
>   winsup/cygwin/fhandler/pipe.cc          | 482 ++++--------------------
>   winsup/cygwin/local_includes/fhandler.h |  42 +--
>   winsup/cygwin/local_includes/sigproc.h  |   1 -
>   winsup/cygwin/select.cc                 |  25 +-
>   winsup/cygwin/sigproc.cc                |  10 -
>   winsup/cygwin/spawn.cc                  |   4 -
>   7 files changed, 95 insertions(+), 474 deletions(-)

[...]

> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index c686df650..f99cbbc56 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc

[...]

> @@ -339,37 +306,11 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>   				       FilePipeLocalInformation);
>         if (NT_SUCCESS (status))
>   	{
> -	  if (fpli.ReadDataAvailable == 0 && nbytes != 0)
> -	    break;
> -	}
> -      else if (nbytes != 0)
> -	break;
> -      status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, ptr,
> -			   len1, NULL, NULL);
> -      if (isclosed ())  /* A signal handler might have closed the fd. */
> -	{
> -	  set_errno (EBADF);
> -	  nbytes = (size_t) -1;
> -	}
> -      else if (NT_SUCCESS (status) || status == STATUS_BUFFER_OVERFLOW)
> -	{
> -	  nbytes_now = io.Information;
> -	  ptr = ((char *) ptr) + nbytes_now;
> -	  nbytes += nbytes_now;
> -	  if (select_sem && nbytes_now > 0)
> -	    release_select_sem ("raw_read");
> -	}
> -      else
> -	{
> -	  /* Some errors are not really errors.  Detect such cases here.  */
> -	  switch (status)
> +	  if (fpli.ReadDataAvailable == 0)
>   	    {
> -	    case STATUS_END_OF_FILE:
> -	    case STATUS_PIPE_BROKEN:
> -	      /* This is really EOF.  */
> -	      break;
> -	    case STATUS_PIPE_LISTENING:
> -	    case STATUS_PIPE_EMPTY:
> +	      if (fpli.NamedPipeState == FILE_PIPE_CLOSING_STATE)
> +		/* Broken pipe ? */

Doesn't "broken pipe" only make sense for writers?  For a reader, 
wouldn't this be EOF?

> +		break;
>   	      if (nbytes != 0)
>   		break;
>   	      if (is_nonblocking ())
> @@ -399,6 +340,34 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>   		  break;
>   		}
>   	      continue;
> +	    }
> +	}
> +      else if (nbytes != 0)
> +	break;

What if the call to NtQueryInformationFile failed and nbytes == 0?  In 
the non-blocking case, I think you need to temporarily set the pipe to 
be non-blocking before calling NtReadFile.

> +      status = NtReadFile (get_handle (), NULL, NULL, NULL, &io, ptr,
> +			   len1, NULL, NULL);
> +      if (isclosed ())  /* A signal handler might have closed the fd. */
> +	{
> +	  set_errno (EBADF);
> +	  nbytes = (size_t) -1;
> +	}
> +      else if (NT_SUCCESS (status) || status == STATUS_BUFFER_OVERFLOW)
> +	{
> +	  nbytes_now = io.Information;
> +	  ptr = ((char *) ptr) + nbytes_now;
> +	  nbytes += nbytes_now;
> +	  if (select_sem && nbytes_now > 0)
> +	    release_select_sem ("raw_read");
> +	}
> +      else
> +	{
> +	  /* Some errors are not really errors.  Detect such cases here.  */
> +	  switch (status)
> +	    {
> +	    case STATUS_END_OF_FILE:
> +	    case STATUS_PIPE_BROKEN:
> +	      /* This is really EOF.  */
> +	      break;
>   	    default:
>   	      __seterrno_from_nt_status (status);
>   	      nbytes = (size_t) -1 > @@ -414,18 +383,6 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>     len = nbytes;
>   }
>   
> -bool
> -fhandler_pipe::reader_closed ()
> -{
> -  if (!query_hdl)
> -    return false;
> -  WaitForSingleObject (hdl_cnt_mtx, INFINITE);
> -  int n_reader = get_obj_handle_count (query_hdl);
> -  int n_writer = get_obj_handle_count (get_handle ());
> -  ReleaseMutex (hdl_cnt_mtx);
> -  return n_reader == n_writer;
> -}
> -

Some of the changes below only make sense for pipes, not fifos.  Maybe 
we need separate fhandler_pipe::raw_write and fhandle_fifo::raw_write?

>   ssize_t
>   fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>   {
> @@ -439,19 +396,45 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>     if (!len)
>       return 0;
>   
> -  if (reader_closed ())
> +  ssize_t avail = pipe_buf_size;
> +  bool real_non_blocking_mode = false;
> +  if (is_nonblocking ())
>       {
> -      set_errno (EPIPE);
> -      raise (SIGPIPE);
> -      return -1;
> +      FILE_PIPE_LOCAL_INFORMATION fpli;
> +      status = NtQueryInformationFile (get_handle (), &io, &fpli, sizeof fpli,
> +				       FilePipeLocalInformation);
> +      if (NT_SUCCESS (status))
> +	{
> +	  if (fpli.WriteQuotaAvailable != 0)
> +	    avail = fpli.WriteQuotaAvailable;
> +	  else /* WriteQuotaAvailable == 0 */
> +	    { /* Refer to the comment in select.cc: pipe_data_available(). */
> +	      /* NtSetInformationFile() in set_pipe_non_blocking(true) seems
> +		 to fail with STATUS_PIPE_BUSY if the pipe is not empty.
> +		 In this case, the pipe is really full if WriteQuotaAvailable
> +		 is zero. Otherwise, the pipe is empty. */
> +	      if (!((fhandler_pipe *)this)->set_pipe_non_blocking (true))
> +		{
> +		  /* Full */
> +		  set_errno (EAGAIN);
> +		  return -1;
> +		}
> +	      /* Restore the pipe mode to blocking. */
> +	      ((fhandler_pipe *)this)->set_pipe_non_blocking (false);
> +	      /* Pipe should be empty because reader is waiting the data. */
> +	    }
> +	}
> +      else if (((fhandler_pipe *)this)->set_pipe_non_blocking (true))
> +	/* The pipe space is unknown. */
> +	real_non_blocking_mode = true;

What if set_pipe_non_blocking (true) fails.  Do we really want to 
continue, in which case we'll do a blocking write below?

>       }
>   
> -  if (len <= pipe_buf_size || pipe_buf_size == 0)
> +  if (len <= (size_t) avail || pipe_buf_size == 0)
>       chunk = len;
>     else if (is_nonblocking ())
> -    chunk = len = pipe_buf_size;
> +    chunk = len = avail;
>     else
> -    chunk = pipe_buf_size;
> +    chunk = avail;
>   
>     if (!(evt = CreateEvent (NULL, false, false, NULL)))
>       {

[...]

> diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> index bc02c3f9d..9d47ff3b0 100644
> --- a/winsup/cygwin/select.cc
> +++ b/winsup/cygwin/select.cc
> @@ -642,7 +642,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int flags)
>           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 InboundQuote on the write side,
> +        interesting value, which is the InboundQuota 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

The whole comment needs to be rewritten to reflect the fact that there's 
no longer a query handle.

Ken


More information about the Cygwin-patches mailing list