[PATCH v2] Cygwin: pipe: Restore blocking mode of read pipe on close()
If a cygwin app is executed from a non-cygwin app and the cygwin
app exits, the read pipe remains in the 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 remains in the blocking mode. With this
patch, the blocking mode of the read pipe is stored into a variable
was_blocking_read_pipe on set_pipe_non_blocking() when the cygwin
app starts and restored on close(). In addition, the pipe mode is
set to non-blocking mode in raw_read() if the mode is blocking
mode as well.
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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/fhandler/pipe.cc | 41 +
winsup/cygwin/local_includes/fhandler.h | 3 ++
winsup/cygwin/sigproc.cc| 9 +-
3 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 997346877..3b78cc183 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -54,6 +54,16 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
NTSTATUS status;
IO_STATUS_BLOCK io;
FILE_PIPE_INFORMATION fpi;
+ bool was_blocking_read_pipe_new = was_blocking_read_pipe;
+
+ if (get_device () == FH_PIPER && nonblocking && !was_blocking_read_pipe)
+{
+ status = NtQueryInformationFile (get_handle (), &io, &fpi, sizeof fpi,
+ FilePipeInformation);
+ if (NT_SUCCESS (status))
+ was_blocking_read_pipe_new =
+ (fpi.CompletionMode == FILE_PIPE_QUEUE_OPERATION);
+}
fpi.ReadMode = FILE_PIPE_BYTE_STREAM_MODE;
fpi.CompletionMode = nonblocking ? FILE_PIPE_COMPLETE_OPERATION
@@ -62,6 +72,11 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ else
+{
+ was_blocking_read_pipe = was_blocking_read_pipe_new;
+ is_blocking_read_pipe = !nonblocking;
+}
}
int
@@ -95,6 +110,8 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t
uniq_id)
even with FILE_SYNCHRONOUS_IO_NONALERT. */
set_pipe_non_blocking (get_device () == FH_PIPER ?
true : is_nonblocking ());
+ was_blocking_read_pipe = false;
+
return 1;
}
@@ -289,6 +306,9 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
if (!len)
return;
+ if (is_blocking_read_pipe)
+set_pipe_non_blocking (true);
+
DWORD timeout = is_nonblocking () ? 0 : INFINITE;
DWORD waitret = cygwait (read_mtx, timeout);
switch (waitret)
@@ -721,6 +741,8 @@ fhandler_pipe::close ()
CloseHandle (query_hdl);
if (query_hdl_close_req_evt)
CloseHandle (query_hdl_close_req_evt);
+ if (was_blocking_read_pipe)
+set_pipe_non_blocking (false);
int ret = fhandler_base::close ();
ReleaseMutex (hdl_cnt_mtx);
CloseHandle (hdl_cnt_mtx);
@@ -1373,6 +1395,7 @@ fhandler_pipe::spawn_worker (int fileno_stdin, int
fileno_stdout,
{
fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
pipe->set_pipe_non_blocking (false);
+ need_send_noncygchld_sig = true;
}
/* If multiple writers including non-cygwin app exist, the non-cygwin
@@ -1398,3 +1421,21 @@ fhandler_pipe::spawn_worker (int fileno_stdin, int
fileno_stdout,
t->kill_pgrp (__SIGNONCYGCHLD);
}
}
+
+void
+fhandler_pipe::sigproc_worker (void)
+{
+ cygheap_fdenum cfd (false);
+ while (cfd.next () >= 0)
+if (cfd->get_dev () == FH_PIPEW)
+ {
+ fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+ if (pipe->need_close_query_hdl ())
+ pipe->close_query_handle ();
+ }
+else if (cfd->get_dev () == FH_PIPER)
+ {
+ fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+ pipe->is_blocking_read_pipe = true;
+ }
+}
diff --git a/winsup/cygwin/local_includes/fhandler.h
b/winsup/cygwin/local_includes/fhandler.h
index 10bc9c7ec..04479 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1197,6 +1197,8 @@ class fhandler_pipe: public fhandler_pipe_fifo
private:
HANDLE read_mtx;
pid_t popen_pid;
+ bool was_blocking_read_pipe;
+ bool is_blocking_read_pipe;
HANDLE query_hdl;
HANDLE hdl_cnt_mtx;
HANDLE query_hdl_proc;
@@ -1287,6 +1289,7 @@ public:
}
static void spawn_worker (int fileno_stdin, int fileno_stdout,
int fileno_stderr);
+ static void sigproc_worker (void);
};
#define CYGWIN_FIFO_PIPE_NAME_LEN 47
diff --g
Re: [PATCH] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
On Thu, 5 Sep 2024 17:16:23 +0200
Corinna Vinschen wrote:
> thanks for this patch. Two points:
Thanks for reviewing so quickly!
> On Sep 5 22:18, Takashi Yano wrote:
> > @@ -446,12 +441,20 @@ fhandler_pipe_fifo::raw_write (const void *ptr,
> > size_t len)
> >return -1;
> > }
> >
> > - if (len <= pipe_buf_size || pipe_buf_size == 0)
> > + bool query_hdl_available = true;
> > + ssize_t avail = pipe_buf_size;
> > + if (is_nonblocking ())
> > +avail = pipe_data_available (-1, this, get_handle (), PDA_WRITE);
> > + if (avail == 0)
> > +{
> > + set_errno (EAGAIN);
> > + return -1;
> > +}
>
> avail can only be 0 in nonblocking mode, so this should be checked only
> for nonblocking pipes. However, isn't pipe_data_available() a bit
> costly to be called all the time in nonblocking mode? The current
> strategy is to write first and only if that fails, call
> pipe_data_available(). Also, doesn't this circumvent the mechanism
> chosing the chunk size to act POSIX-like as per the lengthy comment
> preceeding the write loop?
Please reffer the comment later.
> > @@ -459,6 +462,28 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t
> > len)
> >return -1;
> > }
> >
> > + if (is_nonblocking ())
> > +{
> > + fhandler_pipe *fh = (fhandler_pipe *) this;
> > + query_hdl_available =
> > + fh->get_query_handle () || fh->temporary_query_hdl ();
> > + if (avail > 1 || query_hdl_available)
> > + len = chunk;
> > + else if (avail == 0) /* The pipe is really full. */
> > + {
> > + set_errno (EAGAIN);
> > + return -1;
> > + }
> > + else if (!fh->set_pipe_non_blocking (true))
> > + /* NtSetInformationFile() in set_pipe_non_blocking(true)
> > + fails for unknown reasons if the pipe is not empty.
>
> Which NTSTATUS code does NtSetInformationFile() return in this case?
>
> This sounds pretty ominous. If we can't set the pipe to non-blocking
> under all circumstances, isn't this bound to fail again, just differently?
NTSTATUS is STATUS_PIPE_BUSY. As far as I tested, this causes if
the pipe is not empty. In current implementaion, pipe mode is
set at the initializing, so the pipe is supposed to be empty.
Based on this behaviour of fail and success, I come up with new idea
that does not need query handle at all. When WriteQuotaAvailable is 0,
two cases are possible.
1) The pipe is really full.
2) The pipe is being read for larger size than pipe size and it is blocked.
In the case 1), pipe is not empty so NtSetInformationFile() will fail.
In the case 2), reading is blocked, so the pipe is empty. Therefore,
NtSetInformationFile() will succeed.
So, we can distinguish these cases by calling NtSetInformationFile().
Therefore, query handle is not necessary in above cases.
Currently, query handle is used only when WriteQuotaAvailable is 0,
so we can drop query handle entirely.
In fact, query handle has meaning when reader requests smaller
data size than pipe size, however, we do not use query handle
in such way.
Please review v2 patch. This is much simpler than current implementation.
> > + In this case, no pipe reader should be reading the pipe,
> > + so pipe_data_available() has returned correct value. */
>
> I don't understand the conclusion here. The pipe could have 12K data but
> the pipe reader only reads in 4K chunks. In that case `avail' would be
> incorrect.
Yeah, you are right. Important thing here is avail is not zero and not
lager than real pipe space, isn't it?
> > @@ -916,6 +964,11 @@ is_running_as_service (void)
> > simplicity, nt_create will omit the 'open_mode' and 'name'
> > parameters, which aren't needed for our purposes. */
> >
> > +/* Regardless of above comment, the current nt_create() is reverted to just
> > + call fhandler_pipe::create() to allow adding FILE_FLAG_OVERLAPPED flag.
> > + This is needed for query_hdl so that PeekNamedPipe() and NtQueryObject()
> > + are not blocked even while reader is reading the pipe. */
>
> Specifying FILE_FLAG_OVERLAPPED on the Win32 API level is the same thing
> as dropping the FILE_SYNCHRONOUS_IO_NONALERT flag from
> NtCreateNamedPipeFile. Perhaps in conjunction with dropping
> SYNCHRONIZE, but that's theoretically just a way to make the
> file handle synchronizable, so, may or may not be necessary.
That's too bad. :(
> The other advantage of NtCreateNamedPipeFile is
> that you can specify exact permission, like FILE_WRITE_ATTRIBUTES,
> without having to specify the pipe as PIPE_ACCESS_OUTBOUND pipe.
> This may even explain why
> NtSetInformationFile() in set_pipe_non_blocking() failed for you.
>
> Either way, *iff* you just call fhandler_pipe::create() from
> nt_create(), we should drop nt_create entirely. But actually I'd prefer
> the NT method, if possible and rather remove fhandler_pipe::create() in
> the long run.
I'll revert regarding FILE_FLAG_OVERLAPPED changes and revive original
nt_create().
> > --- a/winsup/
Re: [PATCH] Cygwin: pipe: Restore blocking mode of read pipe on close()
On Thu, 5 Sep 2024 17:38:11 +0200 Corinna Vinschen wrote: > I think you should push your original patch to the 3.5 branch for now, > and we test the big change in the main branch first. Sounds reasonable. I'll submit a v2 patch for 3.5 branch: [PATCH v2] Cygwin: pipe: Restore blocking mode of read pipe on close() and v2 patch for 3.6 branch: [PATCH v2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut -- Takashi Yano
[PATCH v2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 493
winsup/cygwin/local_includes/fhandler.h | 42 +-
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 26 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 99 insertions(+), 482 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..2d91a0132 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMute
Re: [PATCH v2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
On Fri, 6 Sep 2024 18:01:08 +0900
Takashi Yano wrote:
> @@ -655,19 +655,17 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE
> h, int flags)
>handling this fact. */
>if (fh->get_device () == FH_PIPEW && fpli.WriteQuotaAvailable == 0)
> {
> - HANDLE query_hdl = ((fhandler_pipe *) fh)->get_query_handle ();
> - if (!query_hdl)
> - query_hdl = ((fhandler_pipe *) fh)->temporary_query_hdl ();
> - if (!query_hdl) /* We cannot know actual write pipe space. */
> - return (flags & PDA_SELECT) ? PIPE_BUF : 1;
> - DWORD nbytes_in_pipe;
> - BOOL res =
> - PeekNamedPipe (query_hdl, NULL, 0, NULL, &nbytes_in_pipe, NULL);
> - if (!((fhandler_pipe *) fh)->get_query_handle ())
> - CloseHandle (query_hdl); /* Close temporary query_hdl */
> - if (!res) /* We cannot know actual write pipe space. */
> - return (flags & PDA_SELECT) ? PIPE_BUF : 1;
> - fpli.WriteQuotaAvailable = fpli.InboundQuota - nbytes_in_pipe;
> + /* NtSetInformationFile() in set_pipe_non_blocking(true)
> + seems to fail for unknown reasons with STATUS_PIPE_BUSY
> + if no reader is reading the pipe. In this case, the pipe
> + is really full if WriteQuotaAvailable is zero. Otherwise,
> + the pipe is empty. */
> + if (!((fhandler_pipe *) fh)->set_pipe_non_blocking (true))
> + return 0; /* Full */
> + /* Restore pipe mode to blocking mode */
> + ((fhandler_pipe *) fh)->set_pipe_non_blocking (false);
> + /* Empty */
> + fpli.WriteQuotaAvailable = fpli.IutboundQuota;
^^^ In
of cource...
> }
>if (fpli.WriteQuotaAvailable > 0)
> {
--
Takashi Yano
Re: [PATCH v2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
On Fri, 6 Sep 2024 18:01:08 +0900
Takashi Yano wrote:
> diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
> index bc02c3f9d..9ddb9898c 100644
> --- a/winsup/cygwin/select.cc
> +++ b/winsup/cygwin/select.cc
> @@ -642,7 +642,7 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE h,
> int flags)
> Consequentially, the only reliable information is available on the
> read side, so fetch info from the read side via the pipe-specific
> query handle. Use fpli.WriteQuotaAvailable as storage for the actual
> -interesting value, which is the InboundQuote on the write side,
> +interesting value, which is the IutboundQuota on the write side,
> decremented by the number of bytes of data in that buffer. */
>/* Note: Do not use NtQueryInformationFile() for query_hdl because
>NtQueryInformationFile() seems to interfere with reading pipes
> @@ -655,19 +655,17 @@ pipe_data_available (int fd, fhandler_base *fh, HANDLE
> h, int flags)
>handling this fact. */
>if (fh->get_device () == FH_PIPEW && fpli.WriteQuotaAvailable == 0)
> {
> - HANDLE query_hdl = ((fhandler_pipe *) fh)->get_query_handle ();
> - if (!query_hdl)
> - query_hdl = ((fhandler_pipe *) fh)->temporary_query_hdl ();
> - if (!query_hdl) /* We cannot know actual write pipe space. */
> - return (flags & PDA_SELECT) ? PIPE_BUF : 1;
> - DWORD nbytes_in_pipe;
> - BOOL res =
> - PeekNamedPipe (query_hdl, NULL, 0, NULL, &nbytes_in_pipe, NULL);
> - if (!((fhandler_pipe *) fh)->get_query_handle ())
> - CloseHandle (query_hdl); /* Close temporary query_hdl */
> - if (!res) /* We cannot know actual write pipe space. */
> - return (flags & PDA_SELECT) ? PIPE_BUF : 1;
> - fpli.WriteQuotaAvailable = fpli.InboundQuota - nbytes_in_pipe;
> + /* NtSetInformationFile() in set_pipe_non_blocking(true)
> + seems to fail for unknown reasons with STATUS_PIPE_BUSY
> + if no reader is reading the pipe. In this case, the pipe
> + is really full if WriteQuotaAvailable is zero. Otherwise,
> + the pipe is empty. */
> + if (!((fhandler_pipe *) fh)->set_pipe_non_blocking (true))
> + return 0; /* Full */
> + /* Restore pipe mode to blocking mode */
> + ((fhandler_pipe *) fh)->set_pipe_non_blocking (false);
> + /* Empty */
> + fpli.WriteQuotaAvailable = fpli.IutboundQuota;
^^^ In
Of cource...
> }
>if (fpli.WriteQuotaAvailable > 0)
> {
--
Takashi Yano
[PATCH v3] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 483
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 26 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 98 insertions(+), 473 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..21447cdc4 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
Re: [PATCH] Cygwin: pipe: Restore blocking mode of read pipe on close()
On Fri, 6 Sep 2024 18:01:27 +0900 Takashi Yano wrote: > On Thu, 5 Sep 2024 17:38:11 +0200 > Corinna Vinschen wrote: > > I think you should push your original patch to the 3.5 branch for now, > > and we test the big change in the main branch first. > > Sounds reasonable. I'll submit a v2 patch for 3.5 branch: > [PATCH v2] Cygwin: pipe: Restore blocking mode of read pipe on close() > and v2 patch for 3.6 branch: > [PATCH v2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut [PATCH v3] Cygwin: pipe: Switch pipe mode to blocking mode by defaut - Use NtQueryInformationFile(FilePipeLocalInformation) instead of PeekNamedPipe(). This make raw_read() faster a bit. -- Takashi Yano
[PATCH v3.1] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 483
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 26 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 98 insertions(+), 473 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..21447cdc4 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
[PATCH v3.2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 482
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 25 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 96 insertions(+), 473 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..acc53502e 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
[PATCH v4] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 474
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 25 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 92 insertions(+), 469 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..63bab3f4d 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
[PATCH v4.1] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 480
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 25 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 95 insertions(+), 472 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..f167e0c9c 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
[PATCH v4.2] Cygwin: pipe: Switch pipe mode to blocking mode by defaut
Previously, cygwin read pipe used non-blocking mode althogh non-
cygwin app uses blocking-mode by default. Despite this requirement,
if a cygwin app is executed from a non-cygwwin 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 wrie 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_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 founhd, 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
Reviewed-by: Corinna Vinschen
Signed-off-by: Takashi Yano
---
winsup/cygwin/dtable.cc | 5 +-
winsup/cygwin/fhandler/pipe.cc | 482
winsup/cygwin/local_includes/fhandler.h | 42 +--
winsup/cygwin/local_includes/sigproc.h | 1 -
winsup/cygwin/select.cc | 25 +-
winsup/cygwin/sigproc.cc| 10 -
winsup/cygwin/spawn.cc | 4 -
7 files changed, 95 insertions(+), 474 deletions(-)
diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc
index 9508f3e0b..7303f7eac 100644
--- a/winsup/cygwin/dtable.cc
+++ b/winsup/cygwin/dtable.cc
@@ -410,9 +410,8 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle)
{
fhandler_pipe *fhp = (fhandler_pipe *) fh;
fhp->set_pipe_buf_size ();
- /* Set read pipe always to nonblocking */
- fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ?
- true : fhp->is_nonblocking ());
+ /* Set pipe always blocking */
+ fhp->set_pipe_non_blocking (false);
}
if (!fh->open_setup (openflags))
diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index c686df650..f99cbbc56 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -48,7 +48,7 @@ fhandler_pipe::fhandler_pipe ()
In addition to setting the blocking mode of the pipe handle, it
also sets the pipe's read mode to byte_stream unconditionally. */
-void
+bool
fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
{
NTSTATUS status;
@@ -62,6 +62,7 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking)
FilePipeInformation);
if (!NT_SUCCESS (status))
debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status);
+ return NT_SUCCESS (status);
}
int
@@ -91,24 +92,10 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode,
int64_t uniq_id)
set_ino (uniq_id);
set_unique_id (uniq_id | !!(mode & GENERIC_WRITE));
if (opened_properly)
-/* Set read pipe always nonblocking to allow signal handling
- even with FILE_SYNCHRONOUS_IO_NONALERT. */
-set_pipe_non_blocking (get_device () == FH_PIPER ?
- true : is_nonblocking ());
-
- /* Store pipe name to path_conv pc for query_hdl check */
- if (get_dev () == FH_PIPEW)
-{
- UNICODE_STRING name;
- WCHAR pipename_buf[MAX_PATH];
- __small_swprintf (pipename_buf, L"%S%S-%u-pipe-nt-%p",
- &ro_u_npfs, &cygheap->installation_key,
- GetCurrentProcessId (), unique_id >> 32);
- name.Length = wcslen (pipename_buf) * sizeof (WCHAR);
- name.MaximumLength = sizeof (pipename_buf);
- name.Buffer = pipename_buf;
- pc.set_nt_native_path (&name);
-}
+/* Set pipe always blocking and simulate non-blocking mode in
+ raw_read()/raw_write() to allow signal handling even with
+ FILE_SYNCHRONOUS_IO_NONALERT. */
+set_pipe_non_blocking (false);
return 1;
}
@@ -214,39 +201,19 @@ out:
bool
fhandler_pipe::open_setup (int flags)
{
- bool read_mtx_created = false;
-
if (!fhandler_base::open_setup (flags))
-goto err;
+return false;
if (get_dev () == FH_PIPER && !read_mtx)
{
SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
read_mtx = CreateMut
