[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

Pseudo console generates escape sequences on execution of non-cygwin
apps.  If the terminal does not support escape sequence, output will
be garbled. This patch prevents garbled output in dumb terminal by
disabling pseudo console.
---
 winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 8308bccf3..b6d58e97a 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  /* If TERM is "dumb" or not set, disable pseudo console */
+	  if (envblock)
+	    {
+	      bool term_is_set = false;
+	      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
+		{
+		  if (wcscmp (p, L"TERM=dumb") == 0)
+		    nopcon = true;
+		  if (wcsncmp (p, L"TERM=", 5) == 0)
+		    term_is_set = true;
+		}
+	      if (!term_is_set)
+		nopcon = true;
+	    }
+	  else
+	    {
+	      const char *term = getenv ("TERM");
+	      if (!term || strcmp (term, "dumb") == 0)
+		nopcon = true;
+	    }
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

Hi Takashi,

On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> Pseudo console generates escape sequences on execution of non-cygwin
> apps.  If the terminal does not support escape sequence, output will
> be garbled. This patch prevents garbled output in dumb terminal by
> disabling pseudo console.

I'm a bit puzzled by this patch.  We had code handling emacs and dumb
terminals explicitely in the early forms of the first incarnation of
the pseudo tty code, but fortunately you found a way to handle this
without hardcoding terminal types into Cygwin.  Why do you think we
have to do this now?


Thanks,
Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

On Wed, 26 Aug 2020 19:36:06 +0200
Corinna Vinschen wrote:
> On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > Pseudo console generates escape sequences on execution of non-cygwin
> > apps.  If the terminal does not support escape sequence, output will
> > be garbled. This patch prevents garbled output in dumb terminal by
> > disabling pseudo console.
> 
> I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> terminals explicitely in the early forms of the first incarnation of
> the pseudo tty code, but fortunately you found a way to handle this
> without hardcoding terminal types into Cygwin.  Why do you think we
> have to do this now?

What previously disccussed was the problem that the clearing
screen at pty startup displays garbage (^[[H^[[2J) in emacs.
Finally, this was settled by eliminating clear-screen and
triggering redraw-screen instead at the first execution of
non-cygwin app.

However, the problem reported in
https://cygwin.com/pipermail/cygwin/2020-August/245983.html
still remains. 

What's worse in the new implementation, pseudo console sends
ESC[6n (querying cursor position) internally on startup and
waits for a response. This causes hang if pseudo console is
started in dumb terminal.

This patch is for fixing this issue.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:
> On Wed, 26 Aug 2020 19:36:06 +0200
> Corinna Vinschen wrote:
> > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > Pseudo console generates escape sequences on execution of non-cygwin
> > > apps.  If the terminal does not support escape sequence, output will
> > > be garbled. This patch prevents garbled output in dumb terminal by
> > > disabling pseudo console.
> > 
> > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > terminals explicitely in the early forms of the first incarnation of
> > the pseudo tty code, but fortunately you found a way to handle this
> > without hardcoding terminal types into Cygwin.  Why do you think we
> > have to do this now?
> 
> What previously disccussed was the problem that the clearing
> screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> Finally, this was settled by eliminating clear-screen and
> triggering redraw-screen instead at the first execution of
> non-cygwin app.
> 
> However, the problem reported in
> https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> still remains. 
> 
> What's worse in the new implementation, pseudo console sends
> ESC[6n (querying cursor position) internally on startup and
> waits for a response. This causes hang if pseudo console is
> started in dumb terminal.
> 
> This patch is for fixing this issue.

Would it be feasible to implement this using a timeout instead?
If the response isn't sent within, say, 100ms, just skip it?


Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

On Thu, 27 Aug 2020 10:47:56 +0200
Corinna Vinschen wrote:
> On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:
> > On Wed, 26 Aug 2020 19:36:06 +0200
> > Corinna Vinschen wrote:
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> > > 
> > > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > > terminals explicitely in the early forms of the first incarnation of
> > > the pseudo tty code, but fortunately you found a way to handle this
> > > without hardcoding terminal types into Cygwin.  Why do you think we
> > > have to do this now?
> > 
> > What previously disccussed was the problem that the clearing
> > screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> > Finally, this was settled by eliminating clear-screen and
> > triggering redraw-screen instead at the first execution of
> > non-cygwin app.
> > 
> > However, the problem reported in
> > https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> > still remains. 
> > 
> > What's worse in the new implementation, pseudo console sends
> > ESC[6n (querying cursor position) internally on startup and
> > waits for a response. This causes hang if pseudo console is
> > started in dumb terminal.
> > 
> > This patch is for fixing this issue.
> 
> Would it be feasible to implement this using a timeout instead?
> If the response isn't sent within, say, 100ms, just skip it?

Hang is caused at CreateProcessW() call, so there is no way to
use time out. It is possible to send ESC[6n before creating
pseudo console for a test and wait for responce with timeout,
however, if the terminal is dumb, garbage ^[[6n will be displayed.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

On Aug 27 17:59, Takashi Yano via Cygwin-patches wrote:
> On Thu, 27 Aug 2020 10:47:56 +0200
> Corinna Vinschen wrote:
> > On Aug 27 13:07, Takashi Yano via Cygwin-patches wrote:
> > > On Wed, 26 Aug 2020 19:36:06 +0200
> > > Corinna Vinschen wrote:
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > > > 
> > > > I'm a bit puzzled by this patch.  We had code handling emacs and dumb
> > > > terminals explicitely in the early forms of the first incarnation of
> > > > the pseudo tty code, but fortunately you found a way to handle this
> > > > without hardcoding terminal types into Cygwin.  Why do you think we
> > > > have to do this now?
> > > 
> > > What previously disccussed was the problem that the clearing
> > > screen at pty startup displays garbage (^[[H^[[2J) in emacs.
> > > Finally, this was settled by eliminating clear-screen and
> > > triggering redraw-screen instead at the first execution of
> > > non-cygwin app.
> > > 
> > > However, the problem reported in
> > > https://cygwin.com/pipermail/cygwin/2020-August/245983.html
> > > still remains. 
> > > 
> > > What's worse in the new implementation, pseudo console sends
> > > ESC[6n (querying cursor position) internally on startup and
> > > waits for a response. This causes hang if pseudo console is
> > > started in dumb terminal.
> > > 
> > > This patch is for fixing this issue.
> > 
> > Would it be feasible to implement this using a timeout instead?
> > If the response isn't sent within, say, 100ms, just skip it?
> 
> Hang is caused at CreateProcessW() call, so there is no way to
> use time out. It is possible to send ESC[6n before creating
> pseudo console for a test and wait for responce with timeout,
> however, if the terminal is dumb, garbage ^[[6n will be displayed.

Doesn't sound so great either.  That would slow down process
startup on dumb terminal a lot, right?

Ok, if you don't see any other way to fix that, I'll push that patch
in a while.


Thanks,
Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

Hi Takashi,

On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> Pseudo console generates escape sequences on execution of non-cygwin
> apps.  If the terminal does not support escape sequence, output will
> be garbled. This patch prevents garbled output in dumb terminal by
> disabling pseudo console.
> ---
>  winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> index 8308bccf3..b6d58e97a 100644
> --- a/winsup/cygwin/spawn.cc
> +++ b/winsup/cygwin/spawn.cc
> @@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
>        ZeroMemory (&si_pcon, sizeof (si_pcon));
>        STARTUPINFOW *si_tmp = &si;
>        if (!iscygwin () && ptys_primary && is_console_app (runpath))
> -	if (ptys_primary->setup_pseudoconsole (&si_pcon,
> -			     mode != _P_OVERLAY && mode != _P_WAIT))
> -	  {
> -	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> -	    si_tmp = &si_pcon.StartupInfo;
> -	    enable_pcon = true;
> -	  }
> +	{
> +	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
> +	  /* If TERM is "dumb" or not set, disable pseudo console */
> +	  if (envblock)
> +	    {
> +	      bool term_is_set = false;
> +	      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
> +		{
> +		  if (wcscmp (p, L"TERM=dumb") == 0)
> +		    nopcon = true;
> +		  if (wcsncmp (p, L"TERM=", 5) == 0)
> +		    term_is_set = true;
> +		}
> +	      if (!term_is_set)
> +		nopcon = true;
> +	    }
> +	  else
> +	    {
> +	      const char *term = getenv ("TERM");
> +	      if (!term || strcmp (term, "dumb") == 0)
> +		nopcon = true;
> +	    }
> +	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
> +	    {
> +	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> +	      si_tmp = &si_pcon.StartupInfo;
> +	      enable_pcon = true;
> +	    }
> +	}
>  
>      loop:
>        /* When ruid != euid we create the new process under the current original
> -- 
> 2.28.0

Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
method so the TERM specific stuff is done in the fhandler code, not
in spawn.cc?


Thanks,
Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

Hi Corinna,

On Fri, 28 Aug 2020 15:45:03 +0200
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > Pseudo console generates escape sequences on execution of non-cygwin
> > apps.  If the terminal does not support escape sequence, output will
> > be garbled. This patch prevents garbled output in dumb terminal by
> > disabling pseudo console.
> > ---
> >  winsup/cygwin/spawn.cc | 36 +++++++++++++++++++++++++++++-------
> >  1 file changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
> > index 8308bccf3..b6d58e97a 100644
> > --- a/winsup/cygwin/spawn.cc
> > +++ b/winsup/cygwin/spawn.cc
> > @@ -647,13 +647,35 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
> >        ZeroMemory (&si_pcon, sizeof (si_pcon));
> >        STARTUPINFOW *si_tmp = &si;
> >        if (!iscygwin () && ptys_primary && is_console_app (runpath))
> > -	if (ptys_primary->setup_pseudoconsole (&si_pcon,
> > -			     mode != _P_OVERLAY && mode != _P_WAIT))
> > -	  {
> > -	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> > -	    si_tmp = &si_pcon.StartupInfo;
> > -	    enable_pcon = true;
> > -	  }
> > +	{
> > +	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
> > +	  /* If TERM is "dumb" or not set, disable pseudo console */
> > +	  if (envblock)
> > +	    {
> > +	      bool term_is_set = false;
> > +	      for (PWCHAR p = envblock; *p != L'\0'; p += wcslen (p) + 1)
> > +		{
> > +		  if (wcscmp (p, L"TERM=dumb") == 0)
> > +		    nopcon = true;
> > +		  if (wcsncmp (p, L"TERM=", 5) == 0)
> > +		    term_is_set = true;
> > +		}
> > +	      if (!term_is_set)
> > +		nopcon = true;
> > +	    }
> > +	  else
> > +	    {
> > +	      const char *term = getenv ("TERM");
> > +	      if (!term || strcmp (term, "dumb") == 0)
> > +		nopcon = true;
> > +	    }
> > +	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
> > +	    {
> > +	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
> > +	      si_tmp = &si_pcon.StartupInfo;
> > +	      enable_pcon = true;
> > +	    }
> > +	}
> >  
> >      loop:
> >        /* When ruid != euid we create the new process under the current original
> > -- 
> > 2.28.0
> 
> Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> method so the TERM specific stuff is done in the fhandler code, not
> in spawn.cc?

Thansk for the suggestion. I will submit v2 patch.

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

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

Hi Corinna,

On Sat, 29 Aug 2020 04:25:54 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> Hi Corinna,
>
> On Fri, 28 Aug 2020 15:45:03 +0200
> Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > Pseudo console generates escape sequences on execution of non-cygwin
> > > apps.  If the terminal does not support escape sequence, output will
> > > be garbled. This patch prevents garbled output in dumb terminal by
> > > disabling pseudo console.
[...]
> > 
> > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > method so the TERM specific stuff is done in the fhandler code, not
> > in spawn.cc?
> 
> Thansk for the suggestion. I will submit v2 patch.

What do you think of v3 patch attached? With this patch,
terminal capability is checked by looking into terminfo
database rather than just checking terminal name. This
solution is more essential for the issue to be solved,
I think.

One downside of this solution, I noticed, is that tmux
sets TERM to "screen", which does not have CSI6n, by
default. As a result, pseudo console is disbled in tmux
by default. Setting TERM, such as screen.xterm-256color,
will solve the issue.

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

[-- Attachment #2: v3-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7494 bytes --]

From d6480b5993cbca1e7cf5a4376f44811283abbe9c Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sat, 29 Aug 2020 19:09:16 +0900
Subject: [PATCH v3] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 123 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 ++++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 138 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..acd18b63f 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,110 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 > (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") == 0)
+    {
+      /* If the process is background, or another process is already
+	 started under pseudo console, responce for CSI6n may be eaten
+	 by the other process. Therefore, checking set-title capability
+	 should be skipped. */
+      if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+	  && !!pinfo (get_ttyp ()->pcon_pid))
+	; /* Do nothing */
+      else if (!background)
+	{
+	  /* Check if terminal has set-title capability */
+	  DWORD n;
+	  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+	  get_ttyp ()->ti.c_lflag &= ~ICANON;
+	  WriteFile (get_output_handle_cyg (),
+		     "\033[6n\033]0;\007\033[6n", 13, &n, NULL);
+	  char buf[] = "\033[32768;32768R\033[32768;32768R";
+	  char *p = buf;
+	  int len = sizeof (buf) - 1;
+	  int x1, y1, x2, y2;
+	  do
+	    {
+	      ReadFile (get_handle_cyg (), p, len, &n, NULL);
+	      p += n;
+	      len -= n;
+	      *p = '\0';
+	    }
+	  while (sscanf (buf, "\033[%d;%dR\033[%d;%dR",
+			 &y1, &x1, &y2, &x2) != 4);
+	  get_ttyp ()->ti.c_lflag = c_lflag;
+	  if (x2 == x1)
+	    get_ttyp ()->has_set_title = true;
+	  else
+	    {
+	      for (int i=0; i<x2-x1; i++)
+		WriteFile (get_output_handle_cyg (), "\b \b", 3, &n, NULL);
+	      get_ttyp ()->has_set_title = false;
+	    }
+	}
+      return true;
+    }
+  return false;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

On Sat, 29 Aug 2020 20:12:28 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> Hi Corinna,
> 
> On Sat, 29 Aug 2020 04:25:54 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > Hi Corinna,
> >
> > On Fri, 28 Aug 2020 15:45:03 +0200
> > Corinna Vinschen wrote:
> > > Hi Takashi,
> > > 
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> [...]
> > > 
> > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > method so the TERM specific stuff is done in the fhandler code, not
> > > in spawn.cc?
> > 
> > Thansk for the suggestion. I will submit v2 patch.
> 
> What do you think of v3 patch attached? With this patch,
> terminal capability is checked by looking into terminfo
> database rather than just checking terminal name. This
> solution is more essential for the issue to be solved,
> I think.
> 
> One downside of this solution, I noticed, is that tmux
> sets TERM to "screen", which does not have CSI6n, by
> default. As a result, pseudo console is disbled in tmux
> by default. Setting TERM, such as screen.xterm-256color,
> will solve the issue.

Attached is the v4 patch. Small bug was fixed.

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

[-- Attachment #2: v4-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7615 bytes --]

From 378c9337f08aa2a4a70c9bbb4bab7a00d9c7fcff Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sat, 29 Aug 2020 21:59:32 +0900
Subject: [PATCH v4] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 125 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 ++++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 140 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..800f8f0cf 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,112 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") == 0)
+    {
+      /* If the process is background, or another process is already
+	 started under pseudo console, responce for CSI6n may be eaten
+	 by the other process. Therefore, checking set-title capability
+	 should be skipped. */
+      if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+	  && !!pinfo (get_ttyp ()->pcon_pid))
+	; /* Do nothing */
+      else if (!background)
+	{
+	  /* Check if terminal has set-title capability */
+	  DWORD n;
+	  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+	  get_ttyp ()->ti.c_lflag &= ~ICANON;
+	  WriteFile (get_output_handle_cyg (),
+		     "\033[6n\033]0;\007\033[6n", 13, &n, NULL);
+	  char buf[] = "\033[32768;32768R\033[32768;32768R";
+	  char *p = buf;
+	  int len = sizeof (buf) - 1;
+	  int x1, y1, x2, y2;
+	  do
+	    {
+	      ReadFile (get_handle_cyg (), p, len, &n, NULL);
+	      p += n;
+	      len -= n;
+	      *p = '\0';
+	    }
+	  while (sscanf (buf, "\033[%d;%dR\033[%d;%dR",
+			 &y1, &x1, &y2, &x2) != 4);
+	  get_ttyp ()->ti.c_lflag = c_lflag;
+	  if (x2 == x1 && y2 == y1)
+	    /* If "\033]0;\007" does not move cursor position,
+	       set-title is supposed to be supported. */
+	    get_ttyp ()->has_set_title = true;
+	  else
+	    {
+	      for (int i=0; i<x2-x1; i++)
+		WriteFile (get_output_handle_cyg (), "\b \b", 3, &n, NULL);
+	      get_ttyp ()->has_set_title = false;
+	    }
+	}
+      return true;
+    }
+  return false;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

On Sat, 29 Aug 2020 22:14:20 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> On Sat, 29 Aug 2020 20:12:28 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > Hi Corinna,
> > 
> > On Sat, 29 Aug 2020 04:25:54 +0900
> > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > Hi Corinna,
> > >
> > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > Corinna Vinschen wrote:
> > > > Hi Takashi,
> > > > 
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > [...]
> > > > 
> > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > in spawn.cc?
> > > 
> > > Thansk for the suggestion. I will submit v2 patch.
> > 
> > What do you think of v3 patch attached? With this patch,
> > terminal capability is checked by looking into terminfo
> > database rather than just checking terminal name. This
> > solution is more essential for the issue to be solved,
> > I think.
> > 
> > One downside of this solution, I noticed, is that tmux
> > sets TERM to "screen", which does not have CSI6n, by
> > default. As a result, pseudo console is disbled in tmux
> > by default. Setting TERM, such as screen.xterm-256color,
> > will solve the issue.
> 
> Attached is the v4 patch. Small bug was fixed.

Bug fixed again. v5 patch attached.

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

[-- Attachment #2: v5-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7503 bytes --]

From 3f1e8eb9ed563b510dae766a9091507541d16b4a Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 30 Aug 2020 05:07:52 +0900
Subject: [PATCH v5] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 124 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 ++++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 139 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..0a6d211cd 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,111 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") == 0)
+    {
+      /* If the process is background, or another process is already
+	 started under pseudo console, responce for CSI6n may be eaten
+	 by the other process. Therefore, checking set-title capability
+	 should be skipped. */
+      if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+	  && !!pinfo (get_ttyp ()->pcon_pid))
+	; /* Do nothing */
+      else if (!background)
+	{
+	  /* Check if terminal has set-title capability */
+	  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+	  get_ttyp ()->ti.c_lflag &= ~ICANON;
+	  write ("\033[6n\033]0;\007\033[6n", 13);
+	  char buf[] = "\033[32768;32768R\033[32768;32768R";
+	  char *p = buf;
+	  int len = sizeof (buf) - 1;
+	  int x1, y1, x2, y2;
+	  do
+	    {
+	      size_t n = len;
+	      read (p, n);
+	      p += n;
+	      len -= n;
+	      *p = '\0';
+	    }
+	  while (sscanf (buf, "\033[%d;%dR\033[%d;%dR",
+			 &y1, &x1, &y2, &x2) != 4);
+	  get_ttyp ()->ti.c_lflag = c_lflag;
+	  if (x2 == x1 && y2 == y1)
+	    /* If "\033]0;\007" does not move cursor position,
+	       set-title is supposed to be supported. */
+	    get_ttyp ()->has_set_title = true;
+	  else
+	    {
+	      for (int i=0; i<x2-x1; i++)
+		write ("\b \b", 3);
+	      get_ttyp ()->has_set_title = false;
+	    }
+	}
+      return true;
+    }
+  return false;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

On Sun, 30 Aug 2020 05:25:06 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> On Sat, 29 Aug 2020 22:14:20 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > On Sat, 29 Aug 2020 20:12:28 +0900
> > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > Hi Corinna,
> > > 
> > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > Hi Corinna,
> > > >
> > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > Corinna Vinschen wrote:
> > > > > Hi Takashi,
> > > > > 
> > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > disabling pseudo console.
> > > [...]
> > > > > 
> > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > in spawn.cc?
> > > > 
> > > > Thansk for the suggestion. I will submit v2 patch.
> > > 
> > > What do you think of v3 patch attached? With this patch,
> > > terminal capability is checked by looking into terminfo
> > > database rather than just checking terminal name. This
> > > solution is more essential for the issue to be solved,
> > > I think.
> > > 
> > > One downside of this solution, I noticed, is that tmux
> > > sets TERM to "screen", which does not have CSI6n, by
> > > default. As a result, pseudo console is disbled in tmux
> > > by default. Setting TERM, such as screen.xterm-256color,
> > > will solve the issue.
> > 
> > Attached is the v4 patch. Small bug was fixed.
> 
> Bug fixed again. v5 patch attached.

v6: Refactor the code a little.

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

[-- Attachment #2: v6-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7458 bytes --]

From 1f40ef8545867d2d564478bd31fb602d051b1102 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 30 Aug 2020 05:50:20 +0900
Subject: [PATCH v6] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 122 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 ++++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..bb32af0dc 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,109 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") != 0)
+    return false;
+
+  /* If the process is background, or another process is already
+     started under pseudo console, responce to CSI6n may be eaten
+     by the other process. Therefore, checking set-title capability
+     should be skipped. */
+  if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+      && !!pinfo (get_ttyp ()->pcon_pid))
+    return true;
+  if (background)
+    return true;
+
+  /* Check if terminal has set-title capability */
+  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+  get_ttyp ()->ti.c_lflag &= ~ICANON;
+  write ("\033[6n\033]0;\007\033[6n", 13);
+  char buf[] = "\033[32768;32768R\033[32768;32768R";
+  char *p = buf;
+  int len = sizeof (buf) - 1;
+  int x1, y1, x2, y2;
+  do
+    {
+      size_t n = len;
+      read (p, n);
+      p += n;
+      len -= n;
+      *p = '\0';
+    }
+  while (sscanf (buf, "\033[%d;%dR\033[%d;%dR", &y1, &x1, &y2, &x2) != 4);
+  get_ttyp ()->ti.c_lflag = c_lflag;
+  if (x2 == x1 && y2 == y1)
+    /* If "\033]0;\007" does not move cursor position,
+       set-title is supposed to be supported. */
+    get_ttyp ()->has_set_title = true;
+  else
+    {
+      for (int i=0; i<x2-x1; i++)
+	write ("\b \b", 3);
+      get_ttyp ()->has_set_title = false;
+    }
+  return true;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

On Sun, 30 Aug 2020 06:13:17 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> On Sun, 30 Aug 2020 05:25:06 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > On Sat, 29 Aug 2020 22:14:20 +0900
> > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > On Sat, 29 Aug 2020 20:12:28 +0900
> > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > Hi Corinna,
> > > > 
> > > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > > Hi Corinna,
> > > > >
> > > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > > Corinna Vinschen wrote:
> > > > > > Hi Takashi,
> > > > > > 
> > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > > disabling pseudo console.
> > > > [...]
> > > > > > 
> > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > > in spawn.cc?
> > > > > 
> > > > > Thansk for the suggestion. I will submit v2 patch.
> > > > 
> > > > What do you think of v3 patch attached? With this patch,
> > > > terminal capability is checked by looking into terminfo
> > > > database rather than just checking terminal name. This
> > > > solution is more essential for the issue to be solved,
> > > > I think.
> > > > 
> > > > One downside of this solution, I noticed, is that tmux
> > > > sets TERM to "screen", which does not have CSI6n, by
> > > > default. As a result, pseudo console is disbled in tmux
> > > > by default. Setting TERM, such as screen.xterm-256color,
> > > > will solve the issue.
> > > 
> > > Attached is the v4 patch. Small bug was fixed.
> > 
> > Bug fixed again. v5 patch attached.
> 
> v6: Refactor the code a little.

v7: Fix another bug again.

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

[-- Attachment #2: v7-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7698 bytes --]

From 18f2238712fa30918374233486b89e18f205e719 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 30 Aug 2020 16:37:29 +0900
Subject: [PATCH v7] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 131 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 +++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 146 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..596bc6bc7 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,118 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") != 0)
+    return false;
+
+  /* If the process is background, or another process is already
+     started under pseudo console, responce to CSI6n may be eaten
+     by the other process. Therefore, checking set-title capability
+     should be skipped. */
+  if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+      && !!pinfo (get_ttyp ()->pcon_pid))
+    return true;
+  if (background)
+    return true;
+
+  /* Check if terminal has set-title capability */
+  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+  get_ttyp ()->ti.c_lflag &= ~ICANON;
+  write ("\033[6n\033]0;\033\\\033[6n", 14);
+  char buf[] = "\033[32768;32768R\033[32768;32768R";
+  char *p = buf;
+  int len = sizeof (buf) - 1;
+  int x1, y1, x2, y2;
+  do
+    {
+      size_t n = len;
+      read (p, n);
+      p += n;
+      len -= n;
+      *p = '\0';
+      char *p2 = strrchr (buf, '\033');
+      if (p2 == NULL || sscanf (p2, "\033[%d;%dR", &y2, &x2) != 2)
+	continue;
+      *p2 = '\0';
+      char *p1 = strrchr (buf, '\033');
+      *p2 = '\033';
+      if (p1 == NULL || sscanf (p1, "\033[%d;%dR", &y1, &x1) != 2)
+	continue;
+      break;
+    }
+  while (true);
+  get_ttyp ()->ti.c_lflag = c_lflag;
+  if (x2 == x1 && y2 == y1)
+    /* If "\033]0;\033\\" does not move cursor position,
+       set-title is supposed to be supported. */
+    get_ttyp ()->has_set_title = true;
+  else
+    {
+      for (int i=0; i<x2-x1; i++)
+	write ("\b \b", 3);
+      get_ttyp ()->has_set_title = false;
+    }
+  return true;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

On Sun, 30 Aug 2020 16:42:17 +0900
Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> On Sun, 30 Aug 2020 06:13:17 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > On Sun, 30 Aug 2020 05:25:06 +0900
> > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > On Sat, 29 Aug 2020 22:14:20 +0900
> > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > On Sat, 29 Aug 2020 20:12:28 +0900
> > > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > > Hi Corinna,
> > > > > 
> > > > > On Sat, 29 Aug 2020 04:25:54 +0900
> > > > > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > > > > Hi Corinna,
> > > > > >
> > > > > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > > > > Corinna Vinschen wrote:
> > > > > > > Hi Takashi,
> > > > > > > 
> > > > > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > > > > disabling pseudo console.
> > > > > [...]
> > > > > > > 
> > > > > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > > > > in spawn.cc?
> > > > > > 
> > > > > > Thansk for the suggestion. I will submit v2 patch.
> > > > > 
> > > > > What do you think of v3 patch attached? With this patch,
> > > > > terminal capability is checked by looking into terminfo
> > > > > database rather than just checking terminal name. This
> > > > > solution is more essential for the issue to be solved,
> > > > > I think.
> > > > > 
> > > > > One downside of this solution, I noticed, is that tmux
> > > > > sets TERM to "screen", which does not have CSI6n, by
> > > > > default. As a result, pseudo console is disbled in tmux
> > > > > by default. Setting TERM, such as screen.xterm-256color,
> > > > > will solve the issue.
> > > > 
> > > > Attached is the v4 patch. Small bug was fixed.
> > > 
> > > Bug fixed again. v5 patch attached.
> > 
> > v6: Refactor the code a little.
> 
> v7: Fix another bug again.

v8: Yet another bug fix.

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

[-- Attachment #2: v8-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 7697 bytes --]

From f7178cc0b5709a134ca5e4e1d85dfb53dbe82f81 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Sun, 30 Aug 2020 16:55:10 +0900
Subject: [PATCH v8] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n in terminfo database. Also, removes escape sequence for
  setting window title if the terminal does not have the set-title
  capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 133 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  19 +++--
 winsup/cygwin/tty.cc          |   1 +
 winsup/cygwin/tty.h           |   1 +
 5 files changed, 148 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..f55bcf9d1 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env, bool bg);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..2a7e66afd 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,120 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env, bool background)
+{
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* Check if terminal has capability which pusedo console needs */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int user7 = 294; /* u7 (query cursor position) entry index */
+  if (user7 >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + user7 >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[user7] == -1)
+    return false;
+  const char *user7_str = str_table + str_idx[user7];
+  if (user7_str >= str_table + str_size)
+    return false;
+  if (user7_str >= terminfo + n)
+    return false;
+  if (strcmp (user7_str, "\033[6n") != 0)
+    return false;
+
+  /* If the process is background, or another process is already
+     started under pseudo console, responce to CSI6n may be eaten
+     by the other process. Therefore, checking set-title capability
+     should be skipped. */
+  if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
+      && !!pinfo (get_ttyp ()->pcon_pid))
+    return true;
+  if (background)
+    return true;
+
+  /* Check if terminal has set-title capability */
+  tcflag_t c_lflag = get_ttyp ()->ti.c_lflag;
+  get_ttyp ()->ti.c_lflag &= ~ICANON;
+  write ("\033[6n\033]0;\033\\\033[6n", 14);
+  char buf[1024];
+  char *p = buf;
+  int len = sizeof (buf) - 1;
+  int x1, y1, x2, y2;
+  do
+    {
+      size_t n = len;
+      read (p, n);
+      p += n;
+      len -= n;
+      *p = '\0';
+      char *p2 = strrchr (buf, '\033');
+      if (p2 == NULL || sscanf (p2, "\033[%d;%dR", &y2, &x2) != 2)
+	continue;
+      *p2 = '\0';
+      char *p1 = strrchr (buf, '\033');
+      *p2 = '\033';
+      if (p1 == NULL || sscanf (p1, "\033[%d;%dR", &y1, &x1) != 2)
+	continue;
+      break;
+    }
+  while (len);
+  get_ttyp ()->ti.c_lflag = c_lflag;
+  if (len == 0)
+    return true;
+  if (x2 == x1 && y2 == y1)
+    /* If "\033]0;\033\\" does not move cursor position,
+       set-title is supposed to be supported. */
+    get_ttyp ()->has_set_title = true;
+  else
+    {
+      for (int i=0; i<x2-x1; i++)
+	write ("\b \b", 3);
+      get_ttyp ()->has_set_title = false;
+    }
+  return true;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..9a5e3f6ef 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,18 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  bool background = ctty_pgid && ctty_pgid != myself->pgid;
+	  if (!ptys_primary->term_has_pcon_cap (envblock, background))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..e6d57ff6e 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,7 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..13af95687 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,7 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

Hi Takashi,

On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote:
> Hi Corinna,
> 
> On Sat, 29 Aug 2020 04:25:54 +0900
> Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > Hi Corinna,
> >
> > On Fri, 28 Aug 2020 15:45:03 +0200
> > Corinna Vinschen wrote:
> > > Hi Takashi,
> > > 
> > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > apps.  If the terminal does not support escape sequence, output will
> > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > disabling pseudo console.
> [...]
> > > 
> > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > method so the TERM specific stuff is done in the fhandler code, not
> > > in spawn.cc?
> > 
> > Thansk for the suggestion. I will submit v2 patch.
> 
> What do you think of v3 patch attached? With this patch,
> terminal capability is checked by looking into terminfo
> database rather than just checking terminal name. This
> solution is more essential for the issue to be solved,
> I think.
> 
> One downside of this solution, I noticed, is that tmux
> sets TERM to "screen", which does not have CSI6n, by
> default. As a result, pseudo console is disbled in tmux
> by default. Setting TERM, such as screen.xterm-256color,
> will solve the issue.

I like the idea in general, but isn't there a noticable perfomance hit?


Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

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

Hi Corinna,

On Sun, 30 Aug 2020 14:49:25 +0200
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Aug 29 20:12, Takashi Yano via Cygwin-patches wrote:
> > Hi Corinna,
> > 
> > On Sat, 29 Aug 2020 04:25:54 +0900
> > Takashi Yano via Cygwin-patches <cygwin-patches@cygwin.com> wrote:
> > > Hi Corinna,
> > >
> > > On Fri, 28 Aug 2020 15:45:03 +0200
> > > Corinna Vinschen wrote:
> > > > Hi Takashi,
> > > > 
> > > > On Aug 26 21:00, Takashi Yano via Cygwin-patches wrote:
> > > > > Pseudo console generates escape sequences on execution of non-cygwin
> > > > > apps.  If the terminal does not support escape sequence, output will
> > > > > be garbled. This patch prevents garbled output in dumb terminal by
> > > > > disabling pseudo console.
> > [...]
> > > > 
> > > > Would you mind to encapsulate the TERM checks into a fhandler_pty_slave
> > > > method so the TERM specific stuff is done in the fhandler code, not
> > > > in spawn.cc?
> > > 
> > > Thansk for the suggestion. I will submit v2 patch.
> > 
> > What do you think of v3 patch attached? With this patch,
> > terminal capability is checked by looking into terminfo
> > database rather than just checking terminal name. This
> > solution is more essential for the issue to be solved,
> > I think.
> > 
> > One downside of this solution, I noticed, is that tmux
> > sets TERM to "screen", which does not have CSI6n, by
> > default. As a result, pseudo console is disbled in tmux
> > by default. Setting TERM, such as screen.xterm-256color,
> > will solve the issue.
> 
> I like the idea in general, but isn't there a noticable perfomance hit?

I have measured the startup time of non-cygwin process
with v2 and v8 patch.

mintty with v2    : 92.7ms
emacs-dumb with v2: 22.8ms

mintty with v8    : 94.6ms
emacs-dumb with v8: 22.7ms

There is not noticeable difference more than measurement
error.

By the way, I have implemented timeout strategy for CSI6n, you
mentioned in the previous comment, for a test. This check is
done only on the first execution of non-cygwin apps in the pty.
With this patch, first checks if the terminfo has cursor_home
(ESC [H). If terminfo has cursor_home, ANSI escape sequence is
supposed to be supported. In this case, I expect not to display
garbage "^[[6n" even if CSI6n is sent because the parser ignores
unsupported CSI sequences.

With this implementation, pseudo console works in tmux as well
even if TERM=screen.

Please have a look v9 patch attached.

The performance of v9 is also checked.

mintty with v9    : 93.9ms
emacs-dumb with v9: 22.8ms
ANSI without CSI6n: 22.8ms

[the first time in v9]
mintty            : 94.8ms
emacs-dumb        : 22.5ms
ANSI without CSI6n: 63.5ms

Most of the results are the same as v2 and v8 except for the
first execution of non-cygwin apps in ansi terminal without
CSI6n. It takes about 40ms (timeout) longer than dumb terminal
in ANSI terminal without CSI6n support.

However, this causes only on the first execution of non-cygwin
apps in pty.

I think this is the most reasonable one I have ever proposed.

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

[-- Attachment #2: v9-0001-Cygwin-pty-Disable-pseudo-console-if-TERM-does-no.patch --]
[-- Type: application/octet-stream, Size: 9419 bytes --]

From a59325a9abfdbcf2c7faca898519ffc1e693a626 Mon Sep 17 00:00:00 2001
From: Takashi Yano <takashi.yano@nifty.ne.jp>
Date: Mon, 31 Aug 2020 11:23:58 +0900
Subject: [PATCH v9] Cygwin: pty: Disable pseudo console if TERM does not have
 CSI6n.

- Pseudo console internally sends escape sequence CSI6n (query cursor
  position) on startup of non-cygwin apps. If the terminal does not
  support CSI6n, CreateProcess() hangs waiting for response. To prevent
  hang, this patch disables pseudo console if the terminal does not
  have CSI6n. This is checked on the first execution of non-cygwin
  app using the following steps.
    1) Check if the terminal support ANSI escape sequences by looking
       into terminfo database. If terminfo has cursor_home (ESC [H),
       the terminal is supposed to support ANSI escape sequences.
    2) If the terminal supports ANSI escape sequneces, send CSI6n for
       a test and wait for a responce for 40ms.
    3) If there is a responce within 40ms, CSI6n is supposed to be
       supported.
  Also set-title capability is checked, and removes escape sequence
  for setting window title if the terminal does not have the set-
  title capability.
---
 winsup/cygwin/fhandler.h      |   1 +
 winsup/cygwin/fhandler_tty.cc | 185 ++++++++++++++++++++++++++++++++++
 winsup/cygwin/spawn.cc        |  18 ++--
 winsup/cygwin/tty.cc          |   3 +
 winsup/cygwin/tty.h           |   3 +
 5 files changed, 203 insertions(+), 7 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 9fd95c098..b4ba9428a 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2332,6 +2332,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   bool setup_pseudoconsole (STARTUPINFOEXW *si, bool nopcon);
   void close_pseudoconsole (void);
+  bool term_has_pcon_cap (const WCHAR *env);
   void set_switch_to_pcon (void);
   void reset_switch_to_pcon (void);
   void mask_switch_to_pcon_in (bool mask);
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 0865c1fac..12c965ea9 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2169,6 +2169,22 @@ fhandler_pty_master::pty_master_fwd_thread ()
       char *ptr = outbuf;
       if (get_ttyp ()->h_pseudo_console)
 	{
+	  if (!get_ttyp ()->has_set_title)
+	    {
+	      /* Remove Set title sequence */
+	      char *p0, *p1;
+	      p0 = outbuf;
+	      while ((p0 = (char *) memmem (p0, rlen, "\033]0;", 4)))
+		{
+		  p1 = (char *) memchr (p0, '\007', rlen - (p0 - outbuf));
+		  if (p1)
+		    {
+		      memmove (p0, p1 + 1, rlen - (p1 + 1 - outbuf));
+		      rlen -= p1 + 1 - p0;
+		      wlen = rlen;
+		    }
+		}
+	    }
 	  /* Remove CSI > Pm m */
 	  int state = 0;
 	  int start_at = 0;
@@ -2659,3 +2675,172 @@ fhandler_pty_slave::close_pseudoconsole (void)
       get_ttyp ()->pcon_start = false;
     }
 }
+
+static bool
+has_ansi_escape_sequences (const WCHAR *env)
+{
+  /* Retrieve TERM name */
+  const char *term = NULL;
+  char term_str[260];
+  if (env)
+    {
+    for (const WCHAR *p = env; *p != L'\0'; p += wcslen (p) + 1)
+      if (swscanf (p, L"TERM=%236s", term_str) == 1)
+	{
+	  term = term_str;
+	  break;
+	}
+    }
+  else
+    term = getenv ("TERM");
+
+  if (!term)
+    return false;
+
+  /* If cursor_home is not "\033[H", terminal is not supposed to
+     support ANSI escape sequences. */
+  char tinfo[260];
+  __small_sprintf (tinfo, "/usr/share/terminfo/%02x/%s", term[0], term);
+  path_conv path (tinfo);
+  WCHAR wtinfo[260];
+  path.get_wide_win32_path (wtinfo);
+  HANDLE h;
+  h = CreateFileW (wtinfo, GENERIC_READ, FILE_SHARE_READ,
+		   NULL, OPEN_EXISTING, 0, NULL);
+  if (h == NULL)
+    return false;
+  char terminfo[4096];
+  DWORD n;
+  ReadFile (h, terminfo, sizeof (terminfo), &n, 0);
+  CloseHandle (h);
+
+  int num_size = 2;
+  if (*(int16_t *)terminfo == 01036 /* MAGIC2 */)
+    num_size = 4;
+  const int name_pos = 12; /* Position of terminal name */
+  const int name_size = *(int16_t *) (terminfo + 2);
+  const int bool_count = *(int16_t *) (terminfo + 4);
+  const int num_count = *(int16_t *) (terminfo + 6);
+  const int str_count = *(int16_t *) (terminfo + 8);
+  const int str_size = *(int16_t *) (terminfo + 10);
+  const int cursor_home = 12; /* cursor_home entry index */
+  if (cursor_home >= str_count)
+    return false;
+  int str_idx_pos = name_pos + name_size + bool_count + num_size * num_count;
+  if (str_idx_pos & 1)
+    str_idx_pos ++;
+  const int16_t *str_idx = (int16_t *) (terminfo + str_idx_pos);
+  const char *str_table = (const char *) (str_idx + str_count);
+  if (str_idx + cursor_home >= (int16_t *) (terminfo + n))
+    return false;
+  if (str_idx[cursor_home] == -1)
+    return false;
+  const char *cursor_home_str = str_table + str_idx[cursor_home];
+  if (cursor_home_str >= str_table + str_size)
+    return false;
+  if (cursor_home_str >= terminfo + n)
+    return false;
+  if (strcmp (cursor_home_str, "\033[H") != 0)
+    return false;
+  return true;
+}
+
+bool
+fhandler_pty_slave::term_has_pcon_cap (const WCHAR *env)
+{
+  if (get_ttyp ()->pcon_cap_checked)
+    return get_ttyp ()->has_csi6n;
+
+  DWORD n;
+  char buf[1024];
+  char *p;
+  int len;
+  int x1, y1, x2, y2;
+  tcflag_t c_lflag;
+  DWORD t0;
+
+  /* Check if terminal has ANSI escape sequence. */
+  if (!has_ansi_escape_sequences (env))
+    goto maybe_dumb;
+
+  /* Check if terminal has CSI6n */
+  c_lflag = get_ttyp ()->ti.c_lflag;
+  get_ttyp ()->ti.c_lflag &= ~ICANON;
+  get_ttyp ()->h_pseudo_console = (HPCON *)-1; /* dummy */
+  get_ttyp ()->pcon_start = true;
+  WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
+  p = buf;
+  len = sizeof (buf) - 1;
+  t0 = GetTickCount ();
+  do
+    {
+      if (::bytes_available (n, get_handle ()) && n)
+	{
+	  ReadFile (get_handle (), p, len, &n, NULL);
+	  p += n;
+	  len -= n;
+	  *p = '\0';
+	  char *p1 = strrchr (buf, '\033');
+	  if (p1 == NULL || sscanf (p1, "\033[%d;%dR", &y1, &x1) != 2)
+	    continue;
+	  break;
+	}
+      else if (GetTickCount () - t0 > 40) /* Timeout */
+	goto not_has_csi6n;
+      else
+	Sleep (1);
+    }
+  while (len);
+  if (len == 0)
+    goto not_has_csi6n;
+
+  get_ttyp ()->pcon_cap_checked = true;
+  get_ttyp ()->has_csi6n = true;
+
+  /* Check if terminal has set-title capability */
+  get_ttyp ()->pcon_start = true;
+  WriteFile (get_output_handle_cyg (), "\033]0;\033\\\033[6n", 10, &n, NULL);
+  p = buf;
+  len = sizeof (buf) - 1;
+  do
+    {
+      ReadFile (get_handle (), p, len, &n, NULL);
+      p += n;
+      len -= n;
+      *p = '\0';
+      char *p2 = strrchr (buf, '\033');
+      if (p2 == NULL || sscanf (p2, "\033[%d;%dR", &y2, &x2) != 2)
+	continue;
+      break;
+    }
+  while (len);
+  get_ttyp ()->pcon_start = false;
+  get_ttyp ()->h_pseudo_console = NULL;
+  get_ttyp ()->ti.c_lflag = c_lflag;
+
+  if (len == 0)
+    return true;
+
+  if (x2 == x1 && y2 == y1)
+    /* If "\033]0;\033\\" does not move cursor position,
+       set-title is supposed to be supported. */
+    get_ttyp ()->has_set_title = true;
+  else
+    {
+      /* Try to erase garbage string caused by "\033]0;\033\\" */
+      for (int i=0; i<x2-x1; i++)
+	WriteFile (get_output_handle_cyg (), "\b \b", 3, &n, NULL);
+      get_ttyp ()->has_set_title = false;
+    }
+  return true;
+
+not_has_csi6n:
+  get_ttyp ()->pcon_start = false;
+  get_ttyp ()->h_pseudo_console = NULL;
+  get_ttyp ()->ti.c_lflag = c_lflag;
+maybe_dumb:
+  get_ttyp ()->has_csi6n = false;
+  get_ttyp ()->has_set_title = false;
+  get_ttyp ()->pcon_cap_checked = true;
+  return false;
+}
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index a2f7697d7..92d190d1a 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -647,13 +647,17 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
       ZeroMemory (&si_pcon, sizeof (si_pcon));
       STARTUPINFOW *si_tmp = &si;
       if (!iscygwin () && ptys_primary && is_console_app (runpath))
-	if (ptys_primary->setup_pseudoconsole (&si_pcon,
-			     mode != _P_OVERLAY && mode != _P_WAIT))
-	  {
-	    c_flags |= EXTENDED_STARTUPINFO_PRESENT;
-	    si_tmp = &si_pcon.StartupInfo;
-	    enable_pcon = true;
-	  }
+	{
+	  bool nopcon = mode != _P_OVERLAY && mode != _P_WAIT;
+	  if (!ptys_primary->term_has_pcon_cap (envblock))
+	    nopcon = true;
+	  if (ptys_primary->setup_pseudoconsole (&si_pcon, nopcon))
+	    {
+	      c_flags |= EXTENDED_STARTUPINFO_PRESENT;
+	      si_tmp = &si_pcon.StartupInfo;
+	      enable_pcon = true;
+	    }
+	}
 
     loop:
       /* When ruid != euid we create the new process under the current original
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index d60f27545..7e3b88b0b 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -242,6 +242,9 @@ tty::init ()
   term_code_page = 0;
   pcon_last_time = 0;
   pcon_start = false;
+  pcon_cap_checked = false;
+  has_csi6n = false;
+  has_set_title = false;
 }
 
 HANDLE
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index c491d3891..4e9199dba 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -101,6 +101,9 @@ private:
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
+  bool pcon_cap_checked;
+  bool has_csi6n;
+  bool has_set_title;
 
 public:
   HANDLE from_master () const { return _from_master; }
-- 
2.28.0

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

Hi Takashi,

On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:
> Hi Corinna,
> 
> On Sun, 30 Aug 2020 14:49:25 +0200
> Corinna Vinschen wrote:
> > I like the idea in general, but isn't there a noticable perfomance hit?
> 
> I have measured the startup time of non-cygwin process
> with v2 and v8 patch.
> 
> mintty with v2    : 92.7ms
> emacs-dumb with v2: 22.8ms
> 
> mintty with v8    : 94.6ms
> emacs-dumb with v8: 22.7ms
> 
> There is not noticeable difference more than measurement
> error.

Great.

> By the way, I have implemented timeout strategy for CSI6n, you
> mentioned in the previous comment, for a test. This check is
> done only on the first execution of non-cygwin apps in the pty.
> With this patch, first checks if the terminfo has cursor_home
> (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> supposed to be supported. In this case, I expect not to display
> garbage "^[[6n" even if CSI6n is sent because the parser ignores
> unsupported CSI sequences.
> 
> With this implementation, pseudo console works in tmux as well
> even if TERM=screen.
> 
> Please have a look v9 patch attached.
> 
> The performance of v9 is also checked.
> 
> mintty with v9    : 93.9ms
> emacs-dumb with v9: 22.8ms
> ANSI without CSI6n: 22.8ms
> 
> [the first time in v9]
> mintty            : 94.8ms
> emacs-dumb        : 22.5ms
> ANSI without CSI6n: 63.5ms
> 
> Most of the results are the same as v2 and v8 except for the
> first execution of non-cygwin apps in ansi terminal without
> CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> in ANSI terminal without CSI6n support.
> 
> However, this causes only on the first execution of non-cygwin
> apps in pty.
> 
> I think this is the most reasonable one I have ever proposed.

This looks good, but I have a few nits:

- Don't use GetTickCount().  Use GetTickCount64().  Otherwise the code
  is prone to the 49 days tick counter overflow problem(*).

- get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().

- Following the maybe_dumb label, you're setting has_csi6n and
  has_set_title to false, but these are the default values anyway.
  Also, setting has_set_title to false in the preceeding else branch
  seems unnecessary.  Do you want that for clarity?  If not I'd drop
  them.

With these minor problems fixed, I'm happy to push the change.

(*) I just noticed belatedly that GetTickCount() is used in the tty code
    already.  Can you please change pcon_last_time to ULONGLONG and use
    GetTickCount64() instead?


Thanks,
Corinna

Re: [PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Takashi Yano]

Hi Corinna,

Thank you for checking the patch.

On Mon, 31 Aug 2020 10:16:51 +0200
Corinna Vinschen wrote:
> Hi Takashi,
> 
> On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:
> > Hi Corinna,
> > 
> > On Sun, 30 Aug 2020 14:49:25 +0200
> > Corinna Vinschen wrote:
> > > I like the idea in general, but isn't there a noticable perfomance hit?
> > 
> > I have measured the startup time of non-cygwin process
> > with v2 and v8 patch.
> > 
> > mintty with v2    : 92.7ms
> > emacs-dumb with v2: 22.8ms
> > 
> > mintty with v8    : 94.6ms
> > emacs-dumb with v8: 22.7ms
> > 
> > There is not noticeable difference more than measurement
> > error.
> 
> Great.
> 
> > By the way, I have implemented timeout strategy for CSI6n, you
> > mentioned in the previous comment, for a test. This check is
> > done only on the first execution of non-cygwin apps in the pty.
> > With this patch, first checks if the terminfo has cursor_home
> > (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> > supposed to be supported. In this case, I expect not to display
> > garbage "^[[6n" even if CSI6n is sent because the parser ignores
> > unsupported CSI sequences.
> > 
> > With this implementation, pseudo console works in tmux as well
> > even if TERM=screen.
> > 
> > Please have a look v9 patch attached.
> > 
> > The performance of v9 is also checked.
> > 
> > mintty with v9    : 93.9ms
> > emacs-dumb with v9: 22.8ms
> > ANSI without CSI6n: 22.8ms
> > 
> > [the first time in v9]
> > mintty            : 94.8ms
> > emacs-dumb        : 22.5ms
> > ANSI without CSI6n: 63.5ms
> > 
> > Most of the results are the same as v2 and v8 except for the
> > first execution of non-cygwin apps in ansi terminal without
> > CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> > in ANSI terminal without CSI6n support.
> > 
> > However, this causes only on the first execution of non-cygwin
> > apps in pty.
> > 
> > I think this is the most reasonable one I have ever proposed.
> 
> This looks good, but I have a few nits:
> 
> - Don't use GetTickCount().  Use GetTickCount64().  Otherwise the code
>   is prone to the 49 days tick counter overflow problem(*).

This does not matter because the code is
  if (GetTickCount() - t0 > 40)
rather than
  if (GetTickCount() > t0 + 40)
and because DWORD is 32bit unsigned integer.

If the overflow occurs within the timeout period, for example,
t0 = 0xfffffff0 and GetTickCount() becomes 0x00000002,
GetTickCount() - t0 equals to 18 (0x12) as expected.

If the code is
  if (GetTickCount() > t0 + 40)
and t0 = 0xfffffff0 and GetTickCount() is 0xfffffff1,
t0 + 40 equals to 24 (0x18)
so GetTickCount() > t0 + 40 is true against expectation.

> - get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().

pcon_start is cleared to false in master write() if responce to CSI6n
is sent. Therefore, it is necessary to set again after the responce.
I will added the comment in the source.

> - Following the maybe_dumb label, you're setting has_csi6n and
>   has_set_title to false, but these are the default values anyway.
>   Also, setting has_set_title to false in the preceeding else branch
>   seems unnecessary.  Do you want that for clarity?  If not I'd drop
>   them.

You are right. I will submit the v10 patch.

> With these minor problems fixed, I'm happy to push the change.
> 
> (*) I just noticed belatedly that GetTickCount() is used in the tty code
>     already.  Can you please change pcon_last_time to ULONGLONG and use
>     GetTickCount64() instead?

Same here. That does not matter.

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