[PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Takashi Yano]

If pipe reader is a non-cygwin app first, and cygwin process reads
the same pipe after that, the pipe has been set to bclocking mode
for the cygwin app. Hoever, the commit 9e4d308cd592 assumes the pipe
for cygwin process always is non-blocking mode. With this patch,
the pipe mode is restored to non-blocking when the non-cygwin app
exits. As the side effect of this fix, query_hdl for non-cygwin app
can be retrieved from its stub process. Therefore, 46bb999a894 has
been modified a bit as well.

Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
Reported-by: wh <wh9692@protonmail.com>
Reviewed-by:
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc | 12 +++++++-----
 winsup/cygwin/spawn.cc         | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 29d3b41d9..11c833297 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -1205,15 +1205,17 @@ fhandler_pipe::get_query_hdl_per_process (OBJECT_NAME_INFORMATION *ntfn)
       ULONG len;
 
       /* Non-cygwin app may call ReadFile() for empty pipe, which makes
-	NtQueryObject() for ObjectNameInformation block. Therefore, do
-	not try to get query_hdl for non-cygwin apps. */
+	NtQueryObject() for ObjectNameInformation block. However, the stub
+	process for the non-cygwin app should have dup'ed pipe handle.
+	Therefore, use exec_dwProcessId instead. */
       _pinfo *p = pids[i];
-      if (!p || ISSTATE (p, PID_NOTCYGWIN))
+      if (!p)
 	continue;
 
+      DWORD winpid =
+	ISSTATE (p, PID_NOTCYGWIN) ? p->exec_dwProcessId : p->dwProcessId;
       HANDLE proc = OpenProcess (PROCESS_DUP_HANDLE
-				 | PROCESS_QUERY_INFORMATION,
-				 0, p->dwProcessId);
+				 | PROCESS_QUERY_INFORMATION, 0, winpid);
       if (!proc)
 	continue;
 
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 71d75bbf4..92723e5a1 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -579,6 +579,7 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       int fileno_stdout = in__stdout < 0 ? 1 : in__stdout;
       int fileno_stderr = 2;
 
+      fhandler_pipe *pipew_duped = NULL, *piper_duped = NULL;
       if (!iscygwin ())
 	{
 	  bool need_send_sig = false;
@@ -590,6 +591,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	      {
 		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
 		pipe->set_pipe_non_blocking (false);
+		pipew_duped = (fhandler_pipe *)
+			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
+		pipew_duped = new (pipew_duped) fhandler_pipe;
+		pipe->dup (pipew_duped, 0);
 		if (pipe->request_close_query_hdl ())
 		  need_send_sig = true;
 	      }
@@ -597,6 +602,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	      {
 		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
 		pipe->set_pipe_non_blocking (false);
+		piper_duped = (fhandler_pipe *)
+			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
+		piper_duped = new (piper_duped) fhandler_pipe;
+		pipe->dup (piper_duped, 0);
 	      }
 
 	  if (need_send_sig)
@@ -905,6 +914,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	      term_spawn_worker.cleanup ();
 	      term_spawn_worker.close_handle_set ();
 	    }
+	  if (pipew_duped)
+	    {
+	      bool is_nonblocking = pipew_duped->is_nonblocking ();
+	      pipew_duped->set_pipe_non_blocking (is_nonblocking);
+	      pipew_duped->close ();
+	      cfree (pipew_duped);
+	    }
+	  if (piper_duped)
+	    {
+	      piper_duped->set_pipe_non_blocking (true);
+	      piper_duped->close ();
+	      cfree (piper_duped);
+	    }
 	  /* Make sure that ctrl_c_handler() is not on going. Calling
 	     init_console_handler(false) locks until returning from
 	     ctrl_c_handler(). This insures that setting sigExeced
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Corinna Vinschen]

Hi Takashi,

On Mar 10 19:31, Takashi Yano wrote:
> @@ -590,6 +591,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>  	      {
>  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
>  		pipe->set_pipe_non_blocking (false);
> +		pipew_duped = (fhandler_pipe *)
> +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> +		pipew_duped = new (pipew_duped) fhandler_pipe;
> +		pipe->dup (pipew_duped, 0);
>  		if (pipe->request_close_query_hdl ())
>  		  need_send_sig = true;
>  	      }

The code setting up pipes and the dummy_tty is sufficiently complex,
so that I wonder if it shouldn't have

- its own methods and
- comments to describe why this stuff is necessary.

What about adding two methods, kind of like (the names are only
suggestion, albeit bad ones):

  child_info_spawn::noncygwin_child_pre_fork()

to keep the above stuff together (plus comments) and

  child_info_spawn::noncygwin_child_post_fork()

for the below code?

> @@ -597,6 +602,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>  	      {
>  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
>  		pipe->set_pipe_non_blocking (false);
> +		piper_duped = (fhandler_pipe *)
> +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> +		piper_duped = new (piper_duped) fhandler_pipe;
> +		pipe->dup (piper_duped, 0);
>  	      }
>  
>  	  if (need_send_sig)
> @@ -905,6 +914,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>  	      term_spawn_worker.cleanup ();
>  	      term_spawn_worker.close_handle_set ();
>  	    }
> +	  if (pipew_duped)
> +	    {
> +	      bool is_nonblocking = pipew_duped->is_nonblocking ();
> +	      pipew_duped->set_pipe_non_blocking (is_nonblocking);

Is that really right?  You're asking pipew_duped for its
nonblocking flag and then set pipew_duped to the same value...?

> +	      pipew_duped->close ();
> +	      cfree (pipew_duped);
> +	    }


Thanks,
Corinna

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Takashi Yano]

On Mon, 11 Mar 2024 11:47:32 +0100
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Mar 10 19:31, Takashi Yano wrote:
> > @@ -590,6 +591,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >  	      {
> >  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> >  		pipe->set_pipe_non_blocking (false);
> > +		pipew_duped = (fhandler_pipe *)
> > +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> > +		pipew_duped = new (pipew_duped) fhandler_pipe;
> > +		pipe->dup (pipew_duped, 0);
> >  		if (pipe->request_close_query_hdl ())
> >  		  need_send_sig = true;
> >  	      }
> 
> The code setting up pipes and the dummy_tty is sufficiently complex,
> so that I wonder if it shouldn't have
> 
> - its own methods and
> - comments to describe why this stuff is necessary.
> 
> What about adding two methods, kind of like (the names are only
> suggestion, albeit bad ones):
> 
>   child_info_spawn::noncygwin_child_pre_fork()
> 
> to keep the above stuff together (plus comments) and
> 
>   child_info_spawn::noncygwin_child_post_fork()
> 
> for the below code?
> 
> > @@ -597,6 +602,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >  	      {
> >  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> >  		pipe->set_pipe_non_blocking (false);
> > +		piper_duped = (fhandler_pipe *)
> > +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> > +		piper_duped = new (piper_duped) fhandler_pipe;
> > +		pipe->dup (piper_duped, 0);
> >  	      }
> >  
> >  	  if (need_send_sig)
> > @@ -905,6 +914,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >  	      term_spawn_worker.cleanup ();
> >  	      term_spawn_worker.close_handle_set ();
> >  	    }
> > +	  if (pipew_duped)
> > +	    {
> > +	      bool is_nonblocking = pipew_duped->is_nonblocking ();
> > +	      pipew_duped->set_pipe_non_blocking (is_nonblocking);
> 
> Is that really right?  You're asking pipew_duped for its
> nonblocking flag and then set pipew_duped to the same value...?
> 
> > +	      pipew_duped->close ();
> > +	      cfree (pipew_duped);
> > +	    }

Thanks for the reviewing and advice. I'll work for v2 patch. Please wait a while.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Takashi Yano]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

On Mon, 11 Mar 2024 20:42:37 +0900
Takashi Yano wrote:
> On Mon, 11 Mar 2024 11:47:32 +0100
> Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On Mar 10 19:31, Takashi Yano wrote:
> > > @@ -590,6 +591,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> > >  	      {
> > >  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> > >  		pipe->set_pipe_non_blocking (false);
> > > +		pipew_duped = (fhandler_pipe *)
> > > +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> > > +		pipew_duped = new (pipew_duped) fhandler_pipe;
> > > +		pipe->dup (pipew_duped, 0);
> > >  		if (pipe->request_close_query_hdl ())
> > >  		  need_send_sig = true;
> > >  	      }
> > 
> > The code setting up pipes and the dummy_tty is sufficiently complex,
> > so that I wonder if it shouldn't have
> > 
> > - its own methods and
> > - comments to describe why this stuff is necessary.
> > 
> > What about adding two methods, kind of like (the names are only
> > suggestion, albeit bad ones):
> > 
> >   child_info_spawn::noncygwin_child_pre_fork()
> > 
> > to keep the above stuff together (plus comments) and
> > 
> >   child_info_spawn::noncygwin_child_post_fork()
> > 
> > for the below code?
> > 
> > > @@ -597,6 +602,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> > >  	      {
> > >  		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> > >  		pipe->set_pipe_non_blocking (false);
> > > +		piper_duped = (fhandler_pipe *)
> > > +			ccalloc (HEAP_FHANDLER, 1, sizeof (fhandler_pipe));
> > > +		piper_duped = new (piper_duped) fhandler_pipe;
> > > +		pipe->dup (piper_duped, 0);
> > >  	      }
> > >  
> > >  	  if (need_send_sig)
> > > @@ -905,6 +914,19 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> > >  	      term_spawn_worker.cleanup ();
> > >  	      term_spawn_worker.close_handle_set ();
> > >  	    }
> > > +	  if (pipew_duped)
> > > +	    {
> > > +	      bool is_nonblocking = pipew_duped->is_nonblocking ();
> > > +	      pipew_duped->set_pipe_non_blocking (is_nonblocking);
> > 
> > Is that really right?  You're asking pipew_duped for its
> > nonblocking flag and then set pipew_duped to the same value...?
> > 
> > > +	      pipew_duped->close ();
> > > +	      cfree (pipew_duped);
> > > +	    }
> 
> Thanks for the reviewing and advice. I'll work for v2 patch. Please wait a while.

I have reconsider what is essential. Then, just setting read pipe to non-blocking
mode for cygwin apps is enough when it starts.

Please review v2 patch attached.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: v2-0001-Cygwin-pipe-Make-sure-to-set-read-pipe-non-blocki.patch --]
[-- Type: text/plain, Size: 5892 bytes --]

From 43ab2d456f3ef91a1cf8718ee178ac91e4b41679 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Mon, 11 Mar 2024 22:08:00 +0900
Subject: [PATCH v2] Cygwin: pipe: Make sure to set read pipe non-blocking for
 cygwin apps.

If pipe reader is a non-cygwin app first, and cygwin process reads
the same pipe after that, the pipe has been set to bclocking mode
for the cygwin app. However, the commit 9e4d308cd592 assumes the
pipe for cygwin process always is non-blocking mode. With this patch,
the pipe mode is reset to non-blocking when cygwin app is started.

Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
Reported-by: wh <wh9692@protonmail.com>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc          | 54 +++++++++++++++++++++++++
 winsup/cygwin/local_includes/fhandler.h |  2 +
 winsup/cygwin/spawn.cc                  | 35 +---------------
 3 files changed, 58 insertions(+), 33 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 29d3b41d9..b29726dcb 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -18,6 +18,7 @@ details. */
 #include "pinfo.h"
 #include "shared_info.h"
 #include "tls_pbuf.h"
+#include "sigproc.h"
 #include <assert.h>
 
 /* This is only to be used for writing.  When reading,
@@ -1288,3 +1289,56 @@ close_proc:
     }
   return NULL;
 }
+
+void
+fhandler_pipe::spawn_worker (bool iscygwin, int fileno_stdin,
+			     int fileno_stdout, int fileno_stderr)
+{
+  bool need_send_noncygchld_sig = false;
+
+  /* Set read pipe itself always non-blocking for cygwin process. blocking/
+     non-blocking is simulated in the raw_read(). As for write pipe, follow
+     the is_nonblocking(). */
+  int fd;
+  cygheap_fdenum cfd (false);
+  while ((fd = cfd.next ()) >= 0)
+    if (cfd->get_dev () == FH_PIPEW
+	&& (fd == fileno_stdout || fd == fileno_stderr))
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	bool mode = iscygwin ? pipe->is_nonblocking () : false;
+	pipe->set_pipe_non_blocking (mode);
+
+	/* Setup for query_ndl stuff. Read the comment below. */
+	if (!iscygwin && pipe->request_close_query_hdl ())
+	  need_send_noncygchld_sig = true;
+      }
+    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	pipe->set_pipe_non_blocking (iscygwin);
+      }
+
+  /* If multiple writers including non-cygwin app exist, the non-cygwin
+     app cannot detect pipe closure on the read side when the pipe is
+     created by system account or the the pipe creator is running as
+     service.  This is because query_hdl which is held in write side
+     also is a read end of the pipe, so the pipe is still alive for
+     the non-cygwin app even after the reader is closed.
+
+     To avoid this problem, let all processes in the same process
+     group close query_hdl using internal signal __SIGNONCYGCHLD when
+     non-cygwin app is started.  */
+  if (need_send_noncygchld_sig)
+    {
+      tty_min dummy_tty;
+      dummy_tty.ntty = (fh_devices) myself->ctty;
+      dummy_tty.pgid = myself->pgid;
+      tty_min *t = cygwin_shared->tty.get_cttyp ();
+      if (!t) /* If tty is not allocated, use dummy_tty instead. */
+	t = &dummy_tty;
+      /* Emit __SIGNONCYGCHLD to let all processes in the
+	 process group close query_hdl. */
+      t->kill_pgrp (__SIGNONCYGCHLD);
+    }
+}
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 8729eb276..dca473efb 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1295,6 +1295,8 @@ public:
 	}
       return false;
     }
+  static void spawn_worker (bool iscygwin, int fileno_stdin,
+			    int fileno_stdout, int fileno_stderr);
 };
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN     47
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 71d75bbf4..64749b572 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -579,39 +579,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       int fileno_stdout = in__stdout < 0 ? 1 : in__stdout;
       int fileno_stderr = 2;
 
-      if (!iscygwin ())
-	{
-	  bool need_send_sig = false;
-	  int fd;
-	  cygheap_fdenum cfd (false);
-	  while ((fd = cfd.next ()) >= 0)
-	    if (cfd->get_dev () == FH_PIPEW
-		     && (fd == fileno_stdout || fd == fileno_stderr))
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-		if (pipe->request_close_query_hdl ())
-		  need_send_sig = true;
-	      }
-	    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-	      }
-
-	  if (need_send_sig)
-	    {
-	      tty_min dummy_tty;
-	      dummy_tty.ntty = (fh_devices) myself->ctty;
-	      dummy_tty.pgid = myself->pgid;
-	      tty_min *t = cygwin_shared->tty.get_cttyp ();
-	      if (!t) /* If tty is not allocated, use dummy_tty instead. */
-		t = &dummy_tty;
-	      /* Emit __SIGNONCYGCHLD to let all processes in the
-		 process group close query_hdl. */
-	      t->kill_pgrp (__SIGNONCYGCHLD);
-	    }
-	}
+      fhandler_pipe::spawn_worker (iscygwin (), fileno_stdin,
+				   fileno_stdout, fileno_stderr);
 
       bool no_pcon = mode != _P_OVERLAY && mode != _P_WAIT;
       term_spawn_worker.setup (iscygwin (), handle (fileno_stdin, false),
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Corinna Vinschen]

Hi Takashi,


this looks much better.  Just one question and a few comment
changes...

On Mar 11 22:18, Takashi Yano wrote:
> Subject: [PATCH v2] Cygwin: pipe: Make sure to set read pipe non-blocking for
>  cygwin apps.
> 
> If pipe reader is a non-cygwin app first, and cygwin process reads
> the same pipe after that, the pipe has been set to bclocking mode
> for the cygwin app. However, the commit 9e4d308cd592 assumes the
> pipe for cygwin process always is non-blocking mode. With this patch,
> the pipe mode is reset to non-blocking when cygwin app is started.
> 
> Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
> Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
> Reported-by: wh <wh9692@protonmail.com>
> Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pipe.cc          | 54 +++++++++++++++++++++++++
>  winsup/cygwin/local_includes/fhandler.h |  2 +
>  winsup/cygwin/spawn.cc                  | 35 +---------------
>  3 files changed, 58 insertions(+), 33 deletions(-)
> 
> diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> index 29d3b41d9..b29726dcb 100644
> --- a/winsup/cygwin/fhandler/pipe.cc
> +++ b/winsup/cygwin/fhandler/pipe.cc
> @@ -18,6 +18,7 @@ details. */
>  #include "pinfo.h"
>  #include "shared_info.h"
>  #include "tls_pbuf.h"
> +#include "sigproc.h"
>  #include <assert.h>
>  
>  /* This is only to be used for writing.  When reading,
> @@ -1288,3 +1289,56 @@ close_proc:
>      }
>    return NULL;
>  }
> +
> +void
> +fhandler_pipe::spawn_worker (bool iscygwin, int fileno_stdin,
> +			     int fileno_stdout, int fileno_stderr)
> +{
> +  bool need_send_noncygchld_sig = false;
> +
> +  /* Set read pipe itself always non-blocking for cygwin process. blocking/
> +     non-blocking is simulated in the raw_read(). As for write pipe, follow
> +     the is_nonblocking(). */

You can drop the articles here, i.e.

        ...non-blocking is simulated in raw_read().  For write pipe follow
	is_nonblocking().

> +  int fd;
> +  cygheap_fdenum cfd (false);
> +  while ((fd = cfd.next ()) >= 0)
> +    if (cfd->get_dev () == FH_PIPEW
> +	&& (fd == fileno_stdout || fd == fileno_stderr))
> +      {
> +	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> +	bool mode = iscygwin ? pipe->is_nonblocking () : false;
> +	pipe->set_pipe_non_blocking (mode);

What bugs me here is that we now run the loop for cygwin children
as well.  The old code only did that for non-cygwin children.
This looks like quite a performance hit, potentially. Especially
if the process has many open descriptors.  Let's say, a recursive
make or so.  Did you find this is necessary?  No way to avoid that?

> +
> +	/* Setup for query_ndl stuff. Read the comment below. */
> +	if (!iscygwin && pipe->request_close_query_hdl ())
> +	  need_send_noncygchld_sig = true;
> +      }
> +    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
> +      {
> +	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> +	pipe->set_pipe_non_blocking (iscygwin);
> +      }
> +
> +  /* If multiple writers including non-cygwin app exist, the non-cygwin
> +     app cannot detect pipe closure on the read side when the pipe is
> +     created by system account or the the pipe creator is running as
                                     ^^^^^^^


Thanks,
Corinna

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Takashi Yano]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]

On Mon, 11 Mar 2024 21:33:04 +0100
Corinna Vinschen wrote:
> this looks much better.  Just one question and a few comment
> changes...
> 
> On Mar 11 22:18, Takashi Yano wrote:
> > Subject: [PATCH v2] Cygwin: pipe: Make sure to set read pipe non-blocking for
> >  cygwin apps.
> > 
> > If pipe reader is a non-cygwin app first, and cygwin process reads
> > the same pipe after that, the pipe has been set to bclocking mode
> > for the cygwin app. However, the commit 9e4d308cd592 assumes the
> > pipe for cygwin process always is non-blocking mode. With this patch,
> > the pipe mode is reset to non-blocking when cygwin app is started.
> > 
> > Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
> > Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
> > Reported-by: wh <wh9692@protonmail.com>
> > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> >  winsup/cygwin/fhandler/pipe.cc          | 54 +++++++++++++++++++++++++
> >  winsup/cygwin/local_includes/fhandler.h |  2 +
> >  winsup/cygwin/spawn.cc                  | 35 +---------------
> >  3 files changed, 58 insertions(+), 33 deletions(-)
> > 
> > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
> > index 29d3b41d9..b29726dcb 100644
> > --- a/winsup/cygwin/fhandler/pipe.cc
> > +++ b/winsup/cygwin/fhandler/pipe.cc
> > @@ -18,6 +18,7 @@ details. */
> >  #include "pinfo.h"
> >  #include "shared_info.h"
> >  #include "tls_pbuf.h"
> > +#include "sigproc.h"
> >  #include <assert.h>
> >  
> >  /* This is only to be used for writing.  When reading,
> > @@ -1288,3 +1289,56 @@ close_proc:
> >      }
> >    return NULL;
> >  }
> > +
> > +void
> > +fhandler_pipe::spawn_worker (bool iscygwin, int fileno_stdin,
> > +			     int fileno_stdout, int fileno_stderr)
> > +{
> > +  bool need_send_noncygchld_sig = false;
> > +
> > +  /* Set read pipe itself always non-blocking for cygwin process. blocking/
> > +     non-blocking is simulated in the raw_read(). As for write pipe, follow
> > +     the is_nonblocking(). */
> 
> You can drop the articles here, i.e.
> 
>         ...non-blocking is simulated in raw_read().  For write pipe follow
> 	is_nonblocking().

Fixed.

> > +  int fd;
> > +  cygheap_fdenum cfd (false);
> > +  while ((fd = cfd.next ()) >= 0)
> > +    if (cfd->get_dev () == FH_PIPEW
> > +	&& (fd == fileno_stdout || fd == fileno_stderr))
> > +      {
> > +	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> > +	bool mode = iscygwin ? pipe->is_nonblocking () : false;
> > +	pipe->set_pipe_non_blocking (mode);
> 
> What bugs me here is that we now run the loop for cygwin children
> as well.  The old code only did that for non-cygwin children.
> This looks like quite a performance hit, potentially. Especially
> if the process has many open descriptors.  Let's say, a recursive
> make or so.  Did you find this is necessary?  No way to avoid that?

Yeah, you are right. It is not too late to set non-blocking mode
in fixup_after_exec(). Thanks.

> > +
> > +	/* Setup for query_ndl stuff. Read the comment below. */
> > +	if (!iscygwin && pipe->request_close_query_hdl ())
> > +	  need_send_noncygchld_sig = true;
> > +      }
> > +    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
> > +      {
> > +	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> > +	pipe->set_pipe_non_blocking (iscygwin);
> > +      }
> > +
> > +  /* If multiple writers including non-cygwin app exist, the non-cygwin
> > +     app cannot detect pipe closure on the read side when the pipe is
> > +     created by system account or the the pipe creator is running as
>                                      ^^^^^^^

Fixed.

Please check v3 patch attached.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: v3-0001-Cygwin-pipe-Make-sure-to-set-read-pipe-non-blocki.patch --]
[-- Type: text/plain, Size: 6337 bytes --]

From 2778f511f7f3b37c6bedcaff620d5086a28ff7d8 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Mon, 11 Mar 2024 22:08:00 +0900
Subject: [PATCH v3] Cygwin: pipe: Make sure to set read pipe non-blocking for
 cygwin apps.

If pipe reader is a non-cygwin app first, and cygwin process reads
the same pipe after that, the pipe has been set to bclocking mode
for the cygwin app. However, the commit 9e4d308cd592 assumes the
pipe for cygwin process always is non-blocking mode. With this patch,
the pipe mode is reset to non-blocking when cygwin app is started.

Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
Reported-by: wh <wh9692@protonmail.com>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc          | 61 +++++++++++++++++++++++++
 winsup/cygwin/local_includes/fhandler.h |  3 ++
 winsup/cygwin/spawn.cc                  | 34 +-------------
 3 files changed, 66 insertions(+), 32 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 29d3b41d9..553bae8c3 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -18,6 +18,7 @@ details. */
 #include "pinfo.h"
 #include "shared_info.h"
 #include "tls_pbuf.h"
+#include "sigproc.h"
 #include <assert.h>
 
 /* This is only to be used for writing.  When reading,
@@ -602,6 +603,14 @@ fhandler_pipe::fixup_after_fork (HANDLE parent)
   ReleaseMutex (hdl_cnt_mtx);
 }
 
+void
+fhandler_pipe::fixup_after_exec ()
+{
+  bool mode = get_device () == FH_PIPEW ? is_nonblocking () : true;
+  set_pipe_non_blocking (mode);
+  fhandler_base::fixup_after_exec ();
+}
+
 int
 fhandler_pipe::dup (fhandler_base *child, int flags)
 {
@@ -1288,3 +1297,55 @@ close_proc:
     }
   return NULL;
 }
+
+void
+fhandler_pipe::spawn_worker (int fileno_stdin, int fileno_stdout,
+			     int fileno_stderr)
+{
+  bool need_send_noncygchld_sig = false;
+
+  /* Set read pipe itself always non-blocking for cygwin process.
+     Blocking/non-blocking is simulated in raw_read(). For write
+     pipe, follow is_nonblocking(). */
+  int fd;
+  cygheap_fdenum cfd (false);
+  while ((fd = cfd.next ()) >= 0)
+    if (cfd->get_dev () == FH_PIPEW
+	&& (fd == fileno_stdout || fd == fileno_stderr))
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	pipe->set_pipe_non_blocking (false);
+
+	/* Setup for query_ndl stuff. Read the comment below. */
+	if (pipe->request_close_query_hdl ())
+	  need_send_noncygchld_sig = true;
+      }
+    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	pipe->set_pipe_non_blocking (false);
+      }
+
+  /* If multiple writers including non-cygwin app exist, the non-cygwin
+     app cannot detect pipe closure on the read side when the pipe is
+     created by system account or the pipe creator is running as service.
+     This is because query_hdl which is held in write side also is a read
+     end of the pipe, so the pipe is still alive for the non-cygwin app
+     even after the reader is closed.
+
+     To avoid this problem, let all processes in the same process
+     group close query_hdl using internal signal __SIGNONCYGCHLD when
+     non-cygwin app is started.  */
+  if (need_send_noncygchld_sig)
+    {
+      tty_min dummy_tty;
+      dummy_tty.ntty = (fh_devices) myself->ctty;
+      dummy_tty.pgid = myself->pgid;
+      tty_min *t = cygwin_shared->tty.get_cttyp ();
+      if (!t) /* If tty is not allocated, use dummy_tty instead. */
+	t = &dummy_tty;
+      /* Emit __SIGNONCYGCHLD to let all processes in the
+	 process group close query_hdl. */
+      t->kill_pgrp (__SIGNONCYGCHLD);
+    }
+}
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 8729eb276..d9e0a011b 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1234,6 +1234,7 @@ public:
   int open (int flags, mode_t mode = 0);
   bool open_setup (int flags);
   void fixup_after_fork (HANDLE);
+  void fixup_after_exec ();
   int dup (fhandler_base *child, int);
   void set_close_on_exec (bool val);
   int close ();
@@ -1295,6 +1296,8 @@ public:
 	}
       return false;
     }
+  static void spawn_worker (int fileno_stdin, int fileno_stdout,
+			    int fileno_stderr);
 };
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN     47
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 71d75bbf4..3da77088d 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -580,38 +580,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       int fileno_stderr = 2;
 
       if (!iscygwin ())
-	{
-	  bool need_send_sig = false;
-	  int fd;
-	  cygheap_fdenum cfd (false);
-	  while ((fd = cfd.next ()) >= 0)
-	    if (cfd->get_dev () == FH_PIPEW
-		     && (fd == fileno_stdout || fd == fileno_stderr))
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-		if (pipe->request_close_query_hdl ())
-		  need_send_sig = true;
-	      }
-	    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-	      }
-
-	  if (need_send_sig)
-	    {
-	      tty_min dummy_tty;
-	      dummy_tty.ntty = (fh_devices) myself->ctty;
-	      dummy_tty.pgid = myself->pgid;
-	      tty_min *t = cygwin_shared->tty.get_cttyp ();
-	      if (!t) /* If tty is not allocated, use dummy_tty instead. */
-		t = &dummy_tty;
-	      /* Emit __SIGNONCYGCHLD to let all processes in the
-		 process group close query_hdl. */
-	      t->kill_pgrp (__SIGNONCYGCHLD);
-	    }
-	}
+	fhandler_pipe::spawn_worker (fileno_stdin, fileno_stdout,
+				     fileno_stderr);
 
       bool no_pcon = mode != _P_OVERLAY && mode != _P_WAIT;
       term_spawn_worker.setup (iscygwin (), handle (fileno_stdin, false),
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Takashi Yano]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Tue, 12 Mar 2024 08:03:16 +0900
Takashi Yano wrote:
> +  /* Set read pipe itself always non-blocking for cygwin process.
> +     Blocking/non-blocking is simulated in raw_read(). For write
> +     pipe, follow is_nonblocking(). */
> +  int fd;
> +  cygheap_fdenum cfd (false);
> +  while ((fd = cfd.next ()) >= 0)
> +    if (cfd->get_dev () == FH_PIPEW
> +	&& (fd == fileno_stdout || fd == fileno_stderr))
> +      {
> +	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
> +	pipe->set_pipe_non_blocking (false);

Sorry. Commenting here is not right. v4 patch attached.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

[-- Attachment #2: v4-0001-Cygwin-pipe-Make-sure-to-set-read-pipe-non-blocki.patch --]
[-- Type: text/plain, Size: 6480 bytes --]

From 4b536f7dda6c6000e8eccb6af6dbf6abd80f1020 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Mon, 11 Mar 2024 22:08:00 +0900
Subject: [PATCH v4] Cygwin: pipe: Make sure to set read pipe non-blocking for
 cygwin apps.

If pipe reader is a non-cygwin app first, and cygwin process reads
the same pipe after that, the pipe has been set to bclocking mode
for the cygwin app. However, the commit 9e4d308cd592 assumes the
pipe for cygwin process always is non-blocking mode. With this patch,
the pipe mode is reset to non-blocking when cygwin app is started.

Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
Reported-by: wh <wh9692@protonmail.com>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/pipe.cc          | 63 +++++++++++++++++++++++++
 winsup/cygwin/local_includes/fhandler.h |  3 ++
 winsup/cygwin/spawn.cc                  | 34 +------------
 3 files changed, 68 insertions(+), 32 deletions(-)

diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc
index 29d3b41d9..ae43cbc00 100644
--- a/winsup/cygwin/fhandler/pipe.cc
+++ b/winsup/cygwin/fhandler/pipe.cc
@@ -18,6 +18,7 @@ details. */
 #include "pinfo.h"
 #include "shared_info.h"
 #include "tls_pbuf.h"
+#include "sigproc.h"
 #include <assert.h>
 
 /* This is only to be used for writing.  When reading,
@@ -602,6 +603,17 @@ fhandler_pipe::fixup_after_fork (HANDLE parent)
   ReleaseMutex (hdl_cnt_mtx);
 }
 
+void
+fhandler_pipe::fixup_after_exec ()
+{
+  /* Set read pipe itself always non-blocking for cygwin process.
+     Blocking/non-blocking is simulated in raw_read(). For write
+     pipe, follow is_nonblocking(). */
+  bool mode = get_device () == FH_PIPEW ? is_nonblocking () : true;
+  set_pipe_non_blocking (mode);
+  fhandler_base::fixup_after_exec ();
+}
+
 int
 fhandler_pipe::dup (fhandler_base *child, int flags)
 {
@@ -1288,3 +1300,54 @@ close_proc:
     }
   return NULL;
 }
+
+void
+fhandler_pipe::spawn_worker (int fileno_stdin, int fileno_stdout,
+			     int fileno_stderr)
+{
+  bool need_send_noncygchld_sig = false;
+
+  /* spawn_worker() is called from spawn.cc only when non-cygwin app
+     is started. Set pipe mode blocking for the non-cygwin process. */
+  int fd;
+  cygheap_fdenum cfd (false);
+  while ((fd = cfd.next ()) >= 0)
+    if (cfd->get_dev () == FH_PIPEW
+	&& (fd == fileno_stdout || fd == fileno_stderr))
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	pipe->set_pipe_non_blocking (false);
+
+	/* Setup for query_ndl stuff. Read the comment below. */
+	if (pipe->request_close_query_hdl ())
+	  need_send_noncygchld_sig = true;
+      }
+    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
+      {
+	fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
+	pipe->set_pipe_non_blocking (false);
+      }
+
+  /* If multiple writers including non-cygwin app exist, the non-cygwin
+     app cannot detect pipe closure on the read side when the pipe is
+     created by system account or the pipe creator is running as service.
+     This is because query_hdl which is held in write side also is a read
+     end of the pipe, so the pipe is still alive for the non-cygwin app
+     even after the reader is closed.
+
+     To avoid this problem, let all processes in the same process
+     group close query_hdl using internal signal __SIGNONCYGCHLD when
+     non-cygwin app is started.  */
+  if (need_send_noncygchld_sig)
+    {
+      tty_min dummy_tty;
+      dummy_tty.ntty = (fh_devices) myself->ctty;
+      dummy_tty.pgid = myself->pgid;
+      tty_min *t = cygwin_shared->tty.get_cttyp ();
+      if (!t) /* If tty is not allocated, use dummy_tty instead. */
+	t = &dummy_tty;
+      /* Emit __SIGNONCYGCHLD to let all processes in the
+	 process group close query_hdl. */
+      t->kill_pgrp (__SIGNONCYGCHLD);
+    }
+}
diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
index 8729eb276..d9e0a011b 100644
--- a/winsup/cygwin/local_includes/fhandler.h
+++ b/winsup/cygwin/local_includes/fhandler.h
@@ -1234,6 +1234,7 @@ public:
   int open (int flags, mode_t mode = 0);
   bool open_setup (int flags);
   void fixup_after_fork (HANDLE);
+  void fixup_after_exec ();
   int dup (fhandler_base *child, int);
   void set_close_on_exec (bool val);
   int close ();
@@ -1295,6 +1296,8 @@ public:
 	}
       return false;
     }
+  static void spawn_worker (int fileno_stdin, int fileno_stdout,
+			    int fileno_stderr);
 };
 
 #define CYGWIN_FIFO_PIPE_NAME_LEN     47
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 71d75bbf4..3da77088d 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -580,38 +580,8 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       int fileno_stderr = 2;
 
       if (!iscygwin ())
-	{
-	  bool need_send_sig = false;
-	  int fd;
-	  cygheap_fdenum cfd (false);
-	  while ((fd = cfd.next ()) >= 0)
-	    if (cfd->get_dev () == FH_PIPEW
-		     && (fd == fileno_stdout || fd == fileno_stderr))
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-		if (pipe->request_close_query_hdl ())
-		  need_send_sig = true;
-	      }
-	    else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin)
-	      {
-		fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd;
-		pipe->set_pipe_non_blocking (false);
-	      }
-
-	  if (need_send_sig)
-	    {
-	      tty_min dummy_tty;
-	      dummy_tty.ntty = (fh_devices) myself->ctty;
-	      dummy_tty.pgid = myself->pgid;
-	      tty_min *t = cygwin_shared->tty.get_cttyp ();
-	      if (!t) /* If tty is not allocated, use dummy_tty instead. */
-		t = &dummy_tty;
-	      /* Emit __SIGNONCYGCHLD to let all processes in the
-		 process group close query_hdl. */
-	      t->kill_pgrp (__SIGNONCYGCHLD);
-	    }
-	}
+	fhandler_pipe::spawn_worker (fileno_stdin, fileno_stdout,
+				     fileno_stderr);
 
       bool no_pcon = mode != _P_OVERLAY && mode != _P_WAIT;
       term_spawn_worker.setup (iscygwin (), handle (fileno_stdin, false),
-- 
2.43.0

Re: [PATCH] Cygwin: pipe: Restore non-blocking mode which was reset for non-cygwin app.

[Corinna Vinschen]

On Mar 12 08:17, Takashi Yano wrote:
> Subject: [PATCH v4] Cygwin: pipe: Make sure to set read pipe non-blocking for
>  cygwin apps.
> 
> If pipe reader is a non-cygwin app first, and cygwin process reads
> the same pipe after that, the pipe has been set to bclocking mode
> for the cygwin app. However, the commit 9e4d308cd592 assumes the
> pipe for cygwin process always is non-blocking mode. With this patch,
> the pipe mode is reset to non-blocking when cygwin app is started.
> 
> Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255644.html
> Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.")
> Reported-by: wh <wh9692@protonmail.com>
> Reviewed-by: Corinna Vinschen <corinna@vinschen.de>
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> ---
>  winsup/cygwin/fhandler/pipe.cc          | 63 +++++++++++++++++++++++++
>  winsup/cygwin/local_includes/fhandler.h |  3 ++
>  winsup/cygwin/spawn.cc                  | 34 +------------
>  3 files changed, 68 insertions(+), 32 deletions(-)

Looks good, please push.


Thanks,
Corinna