[PATCH v2] Cygwin: pipe: Fix unexpected blocking mode change by pipe_data_available()

[Johannes Schindelin]

Hi Takashi,

On Thu, 26 Jun 2025, Takashi Yano wrote:

> pipe_data_available() is called from raw_write(). If the pipe is in
> real_non_blocking_mode at that time, calling pipe_data_available()
> can, in some cases, inadvertently revert the pipe to blocking mode.
> Here is the background: pipe_data_available() checks the amount of
> writable space in the pipe by calling NtQueryInformationFile() with
> the FilePipeLocalInformation parameter. However, if the read side of
> the pipe is simultaneously consuming data with a large buffer,
> NtQueryInformationFile() may return 0 for WriteQuotaAvailable.
> As a workaround for this behavior, pipe_data_available() temporarily
> attempts to change the pipe-mode to blocking. If the pipe contains
> data, this operation fails-indicating that the pipe is full. If it
> succeeds, the pipe is considered empty. The problem arises from the
> assumption that the pipe is always in real blocking mode before
> attempting to flip the mode. However, if raw_write() has already set
> the pipe to non-blocking mode due to its failure to determine available
> space, two issues occur:
> 1) Changing to non-blocking mode in pipe_data_available() always
>    succeeds, since the pipe is already in non-blocking mode.
> 2) After this, pipe_data_available() sets the pipe back to blocking
>    mode, unintentionally overriding the non-blocking state required
>    by raw_write().
> 
> This patch addresses the issue by having pipe_data_available() check
> the current real blocking mode, temporarily flip the pipe-mode and
> then restore the pipe-mode to its original state.

Thank you for writing this commit message. It describes the problem and
the solution well.

> Addresses: https://github.com/git-for-windows/git/issues/5682#issuecomment-2997428207
> Fixes: 7ed9adb356df ("Cygwin: pipe: Switch pipe mode to blocking mode by default")
> Reported-by: Andrew Ng (nga888 @github)

You may want to use Andrew Ng <andrew.ng@sony.com> instead.

Other than that, this patch looks good to me.

Ciao,
Johannes

> Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pipe.cc          |  2 --
>  winsup/cygwin/local_includes/fhandler.h |  3 +++
>  winsup/cygwin/select.cc                 | 11 ++++++-----
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index e35d523bb..e7dc8850f 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -326,7 +326,6 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
>        ULONG_PTR nbytes_now = 0;
>        ULONG len1 = (ULONG) (len - nbytes);
>        DWORD select_sem_timeout = 0;
> -      bool real_non_blocking_mode = false;
>  
>        FILE_PIPE_LOCAL_INFORMATION fpli;
>        status = NtQueryInformationFile (get_handle (), &io,
> @@ -453,7 +452,6 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
>      return 0;
>  
>    ssize_t avail = pipe_buf_size;
> -  bool real_non_blocking_mode = false;
>  
>    /* Workaround for native ninja. Native ninja creates pipe with size == 0,
>       and starts cygwin process with that pipe. */
> diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
> index 3d9bc9fa5..04e2ca4c3 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -1203,6 +1203,7 @@ class fhandler_pipe_fifo: public fhandler_base
>   protected:
>    size_t pipe_buf_size;
>    HANDLE pipe_mtx; /* Used only in the pipe case */
> +  bool real_non_blocking_mode; /* Used only in the pipe case */
>    virtual void release_select_sem (const char *) {};
>  
>    IMPLEMENT_STATUS_FLAG (bool, isclosed)
> @@ -1212,6 +1213,8 @@ class fhandler_pipe_fifo: public fhandler_base
>  
>    virtual bool reader_closed () { return false; };
>    ssize_t raw_write (const void *ptr, size_t len);
> +
> +  friend ssize_t pipe_data_available (int, fhandler_base *, HANDLE, int);
>  };
>  
>  class fhandler_pipe: public fhandler_pipe_fifo
> diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> index bb141b065..32c73fd0c 100644
> --- a/winsup/cygwin/select.cc
> +++ b/winsup/cygwin/select.cc
> @@ -644,22 +644,23 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode)
>  	reader is currently trying to read the pipe and it is pending.
>  	In the latter case, the fact that the reader cannot read the data
>  	immediately means that the pipe is empty. In the former case,
> -	NtSetInformationFile() in set_pipe_non_blocking(true) will fail
> -	with STATUS_PIPE_BUSY, while it succeeds in the latter case.
> +	NtSetInformationFile() in set_pipe_non_blocking(!orig_mode) will
> +	fail with STATUS_PIPE_BUSY, while it succeeds in the latter case.
>  	Therefore, we can distinguish these cases by calling set_pipe_non_
>  	blocking(true). If it returns success, the pipe is empty, so we
>  	return the pipe buffer size. Otherwise, we return 0. */
>        if (fh->get_device () == FH_PIPEW && fpli.WriteQuotaAvailable == 0)
>  	{
> +	  bool orig_mode = ((fhandler_pipe *) fh)->real_non_blocking_mode;
>  	  NTSTATUS status =
> -	    ((fhandler_pipe *) fh)->set_pipe_non_blocking (true);
> +	    ((fhandler_pipe *) fh)->set_pipe_non_blocking (!orig_mode);
>  	  if (status == STATUS_PIPE_BUSY)
>  	    return 0; /* Full */
>  	  else if (!NT_SUCCESS (status))
>  	    /* We cannot know actual write pipe space. */
>  	    return 1;
> -	  /* Restore pipe mode to blocking mode */
> -	  ((fhandler_pipe *) fh)->set_pipe_non_blocking (false);
> +	  /* Restore pipe mode to original blocking mode */
> +	  ((fhandler_pipe *) fh)->set_pipe_non_blocking (orig_mode);
>  	  /* Empty */
>  	  fpli.WriteQuotaAvailable = fpli.InboundQuota;
>  	}
> -- 
> 2.45.1
> 
>