From 5e98c296689bd59e21e804f1273f27a1b829733f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 30 Jun 2025 10:08:55 +0200 Subject: [PATCH 1/4] fixup! pipe: fix SSH hang (again) This reverts the fix in preparation for integrating a newer iteration of the fix. Signed-off-by: Johannes Schindelin --- winsup/cygwin/fhandler/pipe.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc index 169c0bbf51..506dd09249 100644 --- a/winsup/cygwin/fhandler/pipe.cc +++ b/winsup/cygwin/fhandler/pipe.cc @@ -561,7 +561,7 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) ULONG len1; DWORD waitret = WAIT_OBJECT_0; - if (left > chunk && !real_non_blocking_mode) + if (left > chunk && !is_nonblocking ()) len1 = chunk; else len1 = (ULONG) left; From 0ae6a6fa743c0adca21e16bf8bc7baee6c3c2d0b Mon Sep 17 00:00:00 2001 From: Takashi Yano Date: Fri, 27 Jun 2025 20:40:13 +0900 Subject: [PATCH 2/4] Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader 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 Reviewed-by: Signed-off-by: Takashi Yano Signed-off-by: Johannes Schindelin --- 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 506dd09249..219ae328fa 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 bb141b0655..0b9afb3590 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; From 034c762b936996552fdb2602a803bdbac446eece Mon Sep 17 00:00:00 2001 From: Takashi Yano Date: Fri, 27 Jun 2025 20:40:14 +0900 Subject: [PATCH 3/4] Cygwin: pipe; Update source comment align with previous commit The commit "Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader" modifies how the amount of writable data is evaluated. This patch updates the source comments to align with that change. Reviewed-by: Signed-off-by: Takashi Yano Signed-off-by: Johannes Schindelin --- winsup/cygwin/fhandler/pipe.cc | 8 ++--- winsup/cygwin/select.cc | 54 ++++++++++++++++++---------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc index 219ae328fa..4d5e685410 100644 --- a/winsup/cygwin/fhandler/pipe.cc +++ b/winsup/cygwin/fhandler/pipe.cc @@ -497,8 +497,8 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) { /* 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. */ + In this case, WriteQuotaAvailable indicates real pipe space. + Otherwise, the pipe is empty. */ status = fh->set_pipe_non_blocking (true); if (NT_SUCCESS (status)) /* Pipe should be empty because reader is waiting for data. */ @@ -655,9 +655,7 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) if (io.Information > 0 || len <= PIPE_BUF || short_write_once) break; /* Independent of being blocking or non-blocking, if we're here, - the pipe has less space than requested. If the pipe is a - non-Cygwin pipe, just try the old strategy of trying a half - write. If the pipe has at + the pipe has less space than requested. If the pipe has at least PIPE_BUF bytes available, try to write all matching PIPE_BUF sized blocks. If it's less than PIPE_BUF, try the next less power of 2 bytes. This is not really the Linux diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 0b9afb3590..701f4d9a63 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -623,32 +623,34 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode) if (mode == PDA_WRITE) { /* 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, there are two possibilities when WriteQuotaAvailable - is 0. One is that the buffer is really full. The other is that the - 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. - 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. */ + 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 less than + actual space available in the pipe, even if the inbound buffer is + empty. 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, there are two possibilities when WriteQuotaAvailable + is less than pipe size. One is that the buffer is really not empty. + The other is that the 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. + 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 the value of + WriteQuotaAvailable as is. */ if (fh->get_device () == FH_PIPEW && fpli.WriteQuotaAvailable < fpli.InboundQuota) { From 2f6e58433af2826e890cedbda94479610c78375d Mon Sep 17 00:00:00 2001 From: Takashi Yano Date: Fri, 27 Jun 2025 20:40:15 +0900 Subject: [PATCH 4/4] Cygwin: pipe: Make pipe_data_available() return PDA_UNKNOWN ... rather than 1 when the pipe space estimation fails, so that select() and raw_wrie() can perform appropriate fallback handling. In select(), even if pipe space is unknown, return writable to avoid deadlock. Even with select() returns writable, write() can blocked anyway, if data is larger than pipe space. In raw_write(), if the pipe is real non-blocking mode, attempting to write larger data than pipe space is safe. Otherwise, return error. Reviewed-by: Signed-off-by: Takashi Yano Signed-off-by: Johannes Schindelin --- winsup/cygwin/fhandler/pipe.cc | 7 ++++--- winsup/cygwin/local_includes/select.h | 3 +++ winsup/cygwin/select.cc | 18 +++++++++--------- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc index 4d5e685410..f1cfcfc35b 100644 --- a/winsup/cygwin/fhandler/pipe.cc +++ b/winsup/cygwin/fhandler/pipe.cc @@ -663,12 +663,13 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) in a very implementation-defined way we can't emulate, but it resembles it closely enough to get useful results. */ avail = pipe_data_available (-1, this, get_handle (), PDA_WRITE); - if (avail < 1) /* error or pipe closed */ + if (avail == PDA_UNKNOWN && real_non_blocking_mode) + avail = len1; + else if (avail == 0 || !PDA_NOERROR (avail)) + /* error or pipe closed */ break; if (avail > len1) /* somebody read from the pipe */ avail = len1; - if (avail == 1) /* 1 byte left or non-Cygwin pipe */ - len1 >>= 1; else if (avail >= PIPE_BUF) len1 = avail & ~(PIPE_BUF - 1); else diff --git a/winsup/cygwin/local_includes/select.h b/winsup/cygwin/local_includes/select.h index 43ceb1d7ea..afc05e186b 100644 --- a/winsup/cygwin/local_includes/select.h +++ b/winsup/cygwin/local_includes/select.h @@ -143,5 +143,8 @@ ssize_t pipe_data_available (int, fhandler_base *, HANDLE, int); #define PDA_READ 0x00 #define PDA_WRITE 0x01 +#define PDA_ERROR -1 +#define PDA_UNKNOWN -2 +#define PDA_NOERROR(x) (x >= 0) #endif /* _SELECT_H_ */ diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 701f4d9a63..050221a9f3 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -601,7 +601,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode) if (mode == PDA_READ && PeekNamedPipe (h, NULL, 0, NULL, &nbytes_in_pipe, NULL)) return nbytes_in_pipe; - return -1; + return PDA_ERROR; } IO_STATUS_BLOCK iosb = {{0}, 0}; @@ -618,7 +618,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode) access on the write end. */ select_printf ("fd %d, %s, NtQueryInformationFile failed, status %y", fd, fh->get_name (), status); - return (mode == PDA_WRITE) ? 1 : -1; + return (mode == PDA_WRITE) ? PDA_UNKNOWN : PDA_ERROR; } if (mode == PDA_WRITE) { @@ -660,7 +660,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode) return fpli.WriteQuotaAvailable; /* Not empty */ else if (!NT_SUCCESS (status)) /* We cannot know actual write pipe space. */ - return 1; + return PDA_UNKNOWN; /* Restore pipe mode to blocking mode */ ((fhandler_pipe *) fh)->set_pipe_non_blocking (false); /* Empty */ @@ -684,7 +684,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h, int mode) return fpli.ReadDataAvailable; } if (fpli.NamedPipeState & FILE_PIPE_CLOSING_STATE) - return -1; + return PDA_ERROR; return 0; } @@ -734,7 +734,7 @@ peek_pipe (select_record *s, bool from_select) if (n == 0 && fh->get_echo_handle ()) n = pipe_data_available (s->fd, fh, fh->get_echo_handle (), PDA_READ); - if (n < 0) + if (n == PDA_ERROR) { select_printf ("read: %s, n %d", fh->get_name (), n); if (s->except_selected) @@ -775,8 +775,8 @@ peek_pipe (select_record *s, bool from_select) } ssize_t n = pipe_data_available (s->fd, fh, h, PDA_WRITE); select_printf ("write: %s, n %d", fh->get_name (), n); - gotone += s->write_ready = (n > 0); - if (n < 0 && s->except_selected) + gotone += s->write_ready = (n > 0 || n == PDA_UNKNOWN); + if (n == PDA_ERROR && s->except_selected) gotone += s->except_ready = true; } return gotone; @@ -989,7 +989,7 @@ peek_fifo (select_record *s, bool from_select) ssize_t n = pipe_data_available (s->fd, fh, fh->get_handle (), PDA_WRITE); select_printf ("write: %s, n %d", fh->get_name (), n); gotone += s->write_ready = (n > 0); - if (n < 0 && s->except_selected) + if (n == PDA_ERROR && s->except_selected) gotone += s->except_ready = true; } return gotone; @@ -1415,7 +1415,7 @@ peek_pty_slave (select_record *s, bool from_select) ssize_t n = pipe_data_available (s->fd, fh, h, PDA_WRITE); select_printf ("write: %s, n %d", fh->get_name (), n); gotone += s->write_ready = (n > 0); - if (n < 0 && s->except_selected) + if (n == PDA_ERROR && s->except_selected) gotone += s->except_ready = true; } return gotone;