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 <[email protected]> instead. Other than that, this patch looks good to me. Ciao, Johannes > Reviewed-by: Johannes Schindelin <[email protected]> > Signed-off-by: Takashi Yano <[email protected]> > --- > 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 > >
