Backport recent fixes #296
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: git-for-windows/git#5682 Fixes: 7ed9adb ("Cygwin: pipe: Switch pipe mode to blocking mode by default") Reported-by: Vincent-Liem (@github), Johannes Schindelin <[email protected]> Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 5786a8a)
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: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 82d4ad1)
... 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. For other cases than FH_PIPEW, PDA_UNKNOWN never orrurs. Therefore, it is not necessary to handle it in that cases. Reviewed-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 6d6af65)
Currently, ENABLE_PROCESSED_INPUT is set in set_input_mode() if master_thread_suspended is true. This enables Ctrl-C handling when cons_master_thread is suspended, since Ctrl-C is normally handled by cons_master_thread. However, when disable_master_thread is true, ENABLE_PROCESSED_INPUT is not set, even though this also disables Ctrl-C handling in cons_master_thread. Due to this bug, the command C:\cygwin64\bin\sleep 10 < NUL in the Command Prompt cannot be terminated with Ctrl-C. This patch addresses the issue by setting ENABLE_PROCESSED_INPUT when either disable_master_thread or master_thread_suspended is true. This bug also affects cases where non-Cygwin Git (Git for Windows) launches Cygwin SSH. In such cases, SSH also cannot be terminated with Ctrl-C. Addresses: git-for-windows/git#5682 (comment) Fixes: 746c811 ("Cygwin: console: Allow pasting very long text input.") Reported-by: Johannes Schindelin <[email protected]> Signed-off-by: Takashi Yano <[email protected]> (cherry picked from commit 476135a) Cherry-picked-from: 61cc419 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01)) Signed-off-by: Johannes Schindelin <[email protected]>
|
lgtm |
dscho
added a commit
to dscho/MSYS2-packages
that referenced
this pull request
Jul 1, 2025
This corresponds to msys2/msys2-runtime#296. Signed-off-by: Johannes Schindelin <[email protected]>
lazka
pushed a commit
to msys2/MSYS2-packages
that referenced
this pull request
Jul 1, 2025
This corresponds to msys2/msys2-runtime#296. Signed-off-by: Johannes Schindelin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In Git for Windows v2.50.0, two rather big bugs were identified: First, cloning large repositories via SSH would frequently hang due to a bug in the non-Cygwin <-> Cygwin pipe handling, and second, Ctrl+C would not interrupt that operation.
This PR contains backports of the fixes for both issues, corresponding to git-for-windows/msys2-runtime#103 and to git-for-windows/msys2-runtime#104.