[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default

Ken Brown kbrown@cornell.edu
Wed Sep 25 20:11:12 GMT 2024


On 9/21/2024 5:15 PM, Takashi Yano wrote:
> Previously, cygwin read pipe used non-blocking mode although non-
> cygwin app uses blocking-mode by default. Despite this requirement,
> if a cygwin app is executed from a non-cygwin app and the cygwin
> app exits, read pipe remains on non-blocking mode because of the
> commit fc691d0246b9. Due to this behaviour, the non-cygwin app
> cannot read the pipe correctly after that. Similarly, if a non-
> cygwin app is executed from a cygwin app and the non-cygwin app
> exits, the read pipe mode remains on blocking mode although cygwin
> read pipe should be non-blocking mode.
> 
> These bugs were provoked by pipe mode toggling between cygwin and
> non-cygwin apps. To make management of pipe mode simpler, this
> patch has re-designed the pipe implementation. In this new
> implementation, both read and write pipe basically use only blocking
> mode and the behaviour corresponding to the pipe mode is simulated
> in raw_read() and raw_write(). Only when NtQueryInformationFile
> (FilePipeLocalInformation) fails for some reasons, the raw_read()/
> raw_write() cannot simulate non-blocking access. Therefore, the pipe
> mode is temporarily changed to non-blocking mode.
> 
> Moreover, because the fact that NtSetInformationFile() in
> set_pipe_non_blocking(true) fails with STATUS_PIPE_BUSY if the pipe
> is not empty has been found, query handle is not necessary anymore.
> This allows the implementation much simpler than before.
> 
> Addresses: https://github.com/git-for-windows/git/issues/5115
> Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps.");
> Reported-by: isaacag, Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Ken Brown <kbrown@cornell.edu>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>   winsup/cygwin/dtable.cc                 |   5 +-
>   winsup/cygwin/fhandler/pipe.cc          | 657 ++++++++----------------
>   winsup/cygwin/local_includes/fhandler.h |  44 +-
>   winsup/cygwin/local_includes/sigproc.h  |   1 -
>   winsup/cygwin/select.cc                 |  46 +-
>   winsup/cygwin/sigproc.cc                |  10 -
>   winsup/cygwin/spawn.cc                  |   4 -
>   7 files changed, 252 insertions(+), 515 deletions(-)
LGTM, but it's complicated enough that I could have missed something. 
It will clearly need lots of testing.

One trivial suggestion: For clarity, you should probably add the 
initialization of pipe_mtx to the fhandler_pipe_fifo constructor, 
although I think it's initialized to NULL by default.  Also, it wouldn't 
hurt to add a comment in fhandler.h that pipe_mtx is only used in the 
pipe case, i.e., it remains NULL for fifos.

Ken


More information about the Cygwin-patches mailing list