Hi Takashi, this patch now looks good to go to me. It has a very good commit message that explains the context well.
Thank you for working on this, Johannes On Fri, 27 Jun 2025, Takashi Yano wrote: > If ssh is used with non-cygwin pipe reader, ssh some times hangs. > This happens when non-cygwin git (Git for Windows) starts cygwin > ssh. The background of the bug is as follows. > > Before attempting to NtWriteFile() in raw_write() in non-blocking > mode, the amount of writable space in the pipe is checked by calling > NtQueryInformationFile with FilePipeLocalInformation parameter. > The same is also done by pipe_data_available() in select.cc. > > However, if the read side of the pipe is simultaneously consuming > data, NtQueryInformationFile() returns less value than the amount > of writable space, i.e. the amount of writable space minus the size > of buffer to be read. This does not happen when the reader is a > cygwin app because cygwin read() for the pipe attempts to read > the amount of the data in the pipe at most. This means NtReadFile() > never enters a pending state. However, if the reader is non-cygwin > app, this cannot be expected. As a workaround for this problem, > the code checking the pipe space temporarily attempts to toggle > the pipe-mode. If the pipe contains data, this operation fails > with STATUS_PIPE_BUSY indicating that the pipe is not empty. If > it succeeds, the pipe is considered empty. The current code uses > this technic only when NtQueryInformationFile() retuns zero. > > Therefore, if NtQueryInformationFile() returns 1, the amount of > writable space is assumed to be 1 even in the case that e.g. the > pipe size is 8192 bytes and reader is pending to read 8191 bytes. > Even worse, the current code fails to write more than 1 byte > to 1 byte pipe space due to the remnant of the past design. > Then the reader waits for data with 8191 bytes buffer while the > writer continues to fail to write to 1 byte space of the pipe. > This is the cause of the deadlock. > > In practice, when using Git for Windows in combination with Cygwin > SSH, it has been observed that a read of 8191 bytes is occasionally > issued against a pipe with 8192 bytes of available space. > > With this patch, the blocking-mode-toggling-check is performed > even if NtQueryInformationFile() returns non-zero value so that > the amount of the writable space in the pipe is always estimated > correctly. > > Addresses: https://github.com/git-for-windows/git/issues/5682 > Fixes: 7ed9adb356df ("Cygwin: pipe: Switch pipe mode to blocking mode by > default") > Reported-by: Vincent-Liem (@github), Johannes Schindelin > <[email protected]> > Reviewed-by: > Signed-off-by: Takashi Yano <[email protected]> > --- > winsup/cygwin/fhandler/pipe.cc | 15 ++++++++++----- > winsup/cygwin/select.cc | 5 +++-- > 2 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc > index e35d523bb..bda0a9b25 100644 > --- a/winsup/cygwin/fhandler/pipe.cc > +++ b/winsup/cygwin/fhandler/pipe.cc > @@ -491,9 +491,9 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > FilePipeLocalInformation); > if (NT_SUCCESS (status)) > { > - if (fpli.WriteQuotaAvailable != 0) > + if (fpli.WriteQuotaAvailable == fpli.InboundQuota) > avail = fpli.WriteQuotaAvailable; > - else /* WriteQuotaAvailable == 0 */ > + else /* WriteQuotaAvailable != InboundQuota */ > { /* 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. > @@ -506,9 +506,14 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > fh->set_pipe_non_blocking (false); > else if (status == STATUS_PIPE_BUSY) > { > - /* Full */ > - set_errno (EAGAIN); > - goto err; > + if (fpli.WriteQuotaAvailable == 0) > + { > + /* Full */ > + set_errno (EAGAIN); > + goto err; > + } > + avail = fpli.WriteQuotaAvailable; > + status = STATUS_SUCCESS; > } > } > } > diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc > index bb141b065..0b9afb359 100644 > --- a/winsup/cygwin/select.cc > +++ b/winsup/cygwin/select.cc > @@ -649,12 +649,13 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE > h, int mode) > 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) > + if (fh->get_device () == FH_PIPEW > + && fpli.WriteQuotaAvailable < fpli.InboundQuota) > { > NTSTATUS status = > ((fhandler_pipe *) fh)->set_pipe_non_blocking (true); > if (status == STATUS_PIPE_BUSY) > - return 0; /* Full */ > + return fpli.WriteQuotaAvailable; /* Not empty */ > else if (!NT_SUCCESS (status)) > /* We cannot know actual write pipe space. */ > return 1; > -- > 2.45.1 > >
