[PATCH 2/4] Cygwin: pty: Avoid screen distortion on slave read.

[PATCH 2/4] Cygwin: pty: Avoid screen distortion on slave read.

[Takashi Yano]

- Echo back print is distorted when the cygwin program which calls
  Win32 console API directly calls slave read(). This patch fixes
  the issue.
---
 winsup/cygwin/fhandler.h      |  3 ++-
 winsup/cygwin/fhandler_tty.cc | 51 ++++++++++++++++++++---------------
 2 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 53b6c2c45..a22f3a136 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2206,7 +2206,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   void set_switch_to_pcon (int fd);
   void reset_switch_to_pcon (void);
-  void push_to_pcon_screenbuffer (const char *ptr, size_t len);
+  void push_to_pcon_screenbuffer (const char *ptr, size_t len, bool is_echo);
   void mask_switch_to_pcon_in (bool mask);
   void fixup_after_attach (bool native_maybe, int fd);
   bool is_line_input (void)
@@ -2215,6 +2215,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   }
   void setup_locale (void);
   void set_freeconsole_on_close (bool val);
+  void trigger_redraw_screen (void);
   void wait_pcon_fwd (void);
 };
 
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index a92bcfc40..f88382752 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -80,7 +80,13 @@ set_switch_to_pcon (void)
 	fhandler_base *fh = cfd;
 	fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
 	if (ptys->get_pseudo_console ())
-	  ptys->set_switch_to_pcon (fd);
+	  {
+	    ptys->set_switch_to_pcon (fd);
+	    ptys->trigger_redraw_screen ();
+	    DWORD mode =
+	      ENABLE_PROCESSED_INPUT | ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT;
+	    SetConsoleMode (ptys->get_handle (), mode);
+	  }
       }
 }
 
@@ -1097,9 +1103,6 @@ fhandler_pty_slave::set_switch_to_pcon (int fd_set)
 	if (!try_reattach_pcon ())
 	  goto skip_console_setting;
       FlushConsoleInputBuffer (get_handle ());
-      DWORD mode;
-      mode = ENABLE_PROCESSED_INPUT | ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT;
-      SetConsoleMode (get_handle (), mode);
 skip_console_setting:
       restore_reattach_pcon ();
       if (get_ttyp ()->pcon_pid == 0 ||
@@ -1160,7 +1163,8 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
 }
 
 void
-fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len)
+fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len,
+					       bool is_echo)
 {
   bool attached =
     !!fhandler_console::get_console_process_id (get_helper_process_id (), true);
@@ -1231,7 +1235,7 @@ fhandler_pty_slave::push_to_pcon_screenbuffer (const char *ptr, size_t len)
     }
   if (!nlen) /* Nothing to be synchronized */
     goto cleanup;
-  if (get_ttyp ()->switch_to_pcon_out)
+  if (get_ttyp ()->switch_to_pcon_out && !is_echo)
     goto cleanup;
   /* Remove ESC sequence which returns results to console
      input buffer. Without this, cursor position report
@@ -1388,7 +1392,7 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
   if (get_pseudo_console ())
     {
       acquire_output_mutex (INFINITE);
-      push_to_pcon_screenbuffer ((char *)ptr, len);
+      push_to_pcon_screenbuffer ((char *)ptr, len, false);
       release_output_mutex ();
     }
 
@@ -1716,7 +1720,9 @@ out:
   if (get_pseudo_console () && ptr0 && (get_ttyp ()->ti.c_lflag & ECHO))
     {
       acquire_output_mutex (INFINITE);
-      push_to_pcon_screenbuffer (ptr0, len);
+      push_to_pcon_screenbuffer (ptr0, len, true);
+      if (get_ttyp ()->switch_to_pcon_out)
+	trigger_redraw_screen ();
       release_output_mutex ();
     }
   mask_switch_to_pcon_in (false);
@@ -2700,6 +2706,21 @@ fhandler_pty_slave::wait_pcon_fwd (void)
   cygwait (get_ttyp ()->fwd_done, INFINITE);
 }
 
+void
+fhandler_pty_slave::trigger_redraw_screen (void)
+{
+  /* Forcibly redraw screen based on console screen buffer. */
+  /* The following code triggers redrawing the screen. */
+  CONSOLE_SCREEN_BUFFER_INFO sbi;
+  GetConsoleScreenBufferInfo (get_output_handle (), &sbi);
+  SMALL_RECT rect = {0, 0,
+    (SHORT) (sbi.dwSize.X -1), (SHORT) (sbi.dwSize.Y - 1)};
+  COORD dest = {0, 0};
+  CHAR_INFO fill = {' ', 0};
+  ScrollConsoleScreenBuffer (get_output_handle (), &rect, NULL, dest, &fill);
+  get_ttyp ()->need_redraw_screen = false;
+}
+
 void
 fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
 {
@@ -2754,19 +2775,7 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
 	      get_ttyp ()->switch_to_pcon_out = true;
 
 	      if (get_ttyp ()->need_redraw_screen)
-		{
-		  /* Forcibly redraw screen based on console screen buffer. */
-		  /* The following code triggers redrawing the screen. */
-		  CONSOLE_SCREEN_BUFFER_INFO sbi;
-		  GetConsoleScreenBufferInfo (get_output_handle (), &sbi);
-		  SMALL_RECT rect = {0, 0,
-		    (SHORT) (sbi.dwSize.X -1), (SHORT) (sbi.dwSize.Y - 1)};
-		  COORD dest = {0, 0};
-		  CHAR_INFO fill = {' ', 0};
-		  ScrollConsoleScreenBuffer (get_output_handle (),
-					     &rect, NULL, dest, &fill);
-		  get_ttyp ()->need_redraw_screen = false;
-		}
+		trigger_redraw_screen ();
 	    }
 	  init_console_handler (false);
 	}
-- 
2.21.0

[PATCH 1/4] Cygwin: pty: Define mask_switch_to_pcon_in() in fhandler_tty.cc.

[Takashi Yano]

- This patch moves the definition of mask_switch_to_pcon() from
  fhandler.h to fhandler_tty.cc.
---
 winsup/cygwin/fhandler.h      |  9 +--------
 winsup/cygwin/fhandler_tty.cc | 10 ++++++++++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 82527eca3..53b6c2c45 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -2207,14 +2207,7 @@ class fhandler_pty_slave: public fhandler_pty_common
   void set_switch_to_pcon (int fd);
   void reset_switch_to_pcon (void);
   void push_to_pcon_screenbuffer (const char *ptr, size_t len);
-  void mask_switch_to_pcon_in (bool mask)
-  {
-    if (!mask && get_ttyp ()->pcon_pid &&
-	get_ttyp ()->pcon_pid != myself->pid &&
-	!!pinfo (get_ttyp ()->pcon_pid))
-      return;
-    get_ttyp ()->mask_switch_to_pcon_in = mask;
-  }
+  void mask_switch_to_pcon_in (bool mask);
   void fixup_after_attach (bool native_maybe, int fd);
   bool is_line_input (void)
   {
diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 181bed5a9..a92bcfc40 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1395,6 +1395,16 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
   return towrite;
 }
 
+void
+fhandler_pty_slave::mask_switch_to_pcon_in (bool mask)
+{
+  if (!mask && get_ttyp ()->pcon_pid &&
+      get_ttyp ()->pcon_pid != myself->pid &&
+      !!pinfo (get_ttyp ()->pcon_pid))
+    return;
+  get_ttyp ()->mask_switch_to_pcon_in = mask;
+}
+
 bool
 fhandler_pty_common::to_be_read_from_pcon (void)
 {
-- 
2.21.0

[PATCH 3/4] Cygwin: pty: Remove debug codes and organize related codes.

[Takashi Yano]

- Debug codes used in the early stage of pseudo console support are
  removed. (Regarding ALWAYS_USE_PCON and USE_API_HOOK) Along with
  this, the codes related to this change are organized.
---
 winsup/cygwin/fhandler_tty.cc | 81 ++++++++++-------------------------
 winsup/cygwin/select.cc       | 23 ----------
 2 files changed, 23 insertions(+), 81 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index f88382752..36b3341b1 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -28,9 +28,6 @@ details. */
 #include "tls_pbuf.h"
 #include "registry.h"
 
-#define ALWAYS_USE_PCON false
-#define USE_API_HOOK true
-
 #ifndef PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE
 #define PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE 0x00020016
 #endif /* PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE */
@@ -68,7 +65,6 @@ static bool isHybrid;
 static bool do_not_reset_switch_to_pcon;
 static bool freeconsole_on_close = true;
 
-#if USE_API_HOOK
 static void
 set_switch_to_pcon (void)
 {
@@ -364,12 +360,6 @@ CreateProcessW_Hooked
   set_ishybrid_and_switch_to_pcon (h);
   return CreateProcessW_Orig (n, c, pa, ta, inh, f, e, d, si, pi);
 }
-#else /* USE_API_HOOK */
-#define WriteFile_Orig 0
-#define ReadFile_Orig 0
-#define PeekConsoleInputA_Orig 0
-void set_ishybrid_and_switch_to_pcon (HANDLE) {}
-#endif /* USE_API_HOOK */
 
 static char *
 convert_mb_str (UINT cp_to, size_t *len_to,
@@ -1091,11 +1081,6 @@ fhandler_pty_slave::set_switch_to_pcon (int fd_set)
 {
   if (fd < 0)
     fd = fd_set;
-  if (!isHybrid)
-    {
-      reset_switch_to_pcon ();
-      return;
-    }
   if (fd == 0 && !get_ttyp ()->switch_to_pcon_in)
     {
       pid_restore = 0;
@@ -1109,6 +1094,11 @@ skip_console_setting:
 	  !pinfo (get_ttyp ()->pcon_pid))
 	get_ttyp ()->pcon_pid = myself->pid;
       get_ttyp ()->switch_to_pcon_in = true;
+      if (isHybrid && !get_ttyp ()->switch_to_pcon_out)
+	{
+	  wait_pcon_fwd ();
+	  get_ttyp ()->switch_to_pcon_out = true;
+	}
     }
   else if ((fd == 1 || fd == 2) && !get_ttyp ()->switch_to_pcon_out)
     {
@@ -1117,14 +1107,14 @@ skip_console_setting:
 	  !pinfo (get_ttyp ()->pcon_pid))
 	get_ttyp ()->pcon_pid = myself->pid;
       get_ttyp ()->switch_to_pcon_out = true;
+      if (isHybrid)
+	get_ttyp ()->switch_to_pcon_in = true;
     }
 }
 
 void
 fhandler_pty_slave::reset_switch_to_pcon (void)
 {
-  if (isHybrid)
-    this->set_switch_to_pcon (fd);
   if (get_ttyp ()->pcon_pid &&
       get_ttyp ()->pcon_pid != myself->pid &&
       !!pinfo (get_ttyp ()->pcon_pid))
@@ -1132,27 +1122,17 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
     return;
   if (isHybrid)
     {
-      if (ALWAYS_USE_PCON)
-	{
-	  DWORD mode;
-	  GetConsoleMode (get_handle (), &mode);
-	  mode |= ENABLE_ECHO_INPUT;
-	  mode |= ENABLE_LINE_INPUT;
-	  mode &= ~ENABLE_PROCESSED_INPUT;
-	  SetConsoleMode (get_handle (), mode);
-	}
-      get_ttyp ()->pcon_pid = 0;
+      DWORD bytes_in_pipe;
+      WaitForSingleObject (input_mutex, INFINITE);
+      if (bytes_available (bytes_in_pipe) && !bytes_in_pipe)
+	ResetEvent (input_available_event);
+      FlushConsoleInputBuffer (get_handle ());
+      ReleaseMutex (input_mutex);
       init_console_handler (true);
       return;
     }
   if (do_not_reset_switch_to_pcon)
     return;
-  if (get_ttyp ()->switch_to_pcon_in)
-    {
-      DWORD mode;
-      GetConsoleMode (get_handle (), &mode);
-      SetConsoleMode (get_handle (), mode & ~ENABLE_ECHO_INPUT);
-    }
   if (get_ttyp ()->switch_to_pcon_out)
     /* Wait for pty_master_fwd_thread() */
     wait_pcon_fwd ();
@@ -1413,7 +1393,7 @@ bool
 fhandler_pty_common::to_be_read_from_pcon (void)
 {
   return get_ttyp ()->switch_to_pcon_in &&
-    (!get_ttyp ()->mask_switch_to_pcon_in || ALWAYS_USE_PCON);
+    !get_ttyp ()->mask_switch_to_pcon_in;
 }
 
 void __reg3
@@ -1441,8 +1421,8 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 
   if (ptr) /* Indicating not tcflush(). */
     {
-      reset_switch_to_pcon ();
       mask_switch_to_pcon_in (true);
+      reset_switch_to_pcon ();
     }
 
   if (is_nonblocking () || !ptr) /* Indicating tcflush(). */
@@ -1562,7 +1542,7 @@ fhandler_pty_slave::read (void *ptr, size_t& len)
 	    flags &= ~ENABLE_ECHO_INPUT;
 	  if ((get_ttyp ()->ti.c_lflag & ISIG) &&
 	      !(get_ttyp ()->ti.c_iflag & IGNBRK))
-	    flags |= ALWAYS_USE_PCON ? 0 : ENABLE_PROCESSED_INPUT;
+	    flags |= ENABLE_PROCESSED_INPUT;
 	  if (dwMode != flags)
 	    SetConsoleMode (get_handle (), flags);
 	  /* Read get_handle() instad of get_handle_cyg() */
@@ -2325,13 +2305,11 @@ fhandler_pty_master::write (const void *ptr, size_t len)
       char *buf = convert_mb_str
 	(CP_UTF8, &nlen, get_ttyp ()->term_code_page, (const char *) ptr, len);
 
+      WaitForSingleObject (input_mutex, INFINITE);
+
       DWORD wLen;
       WriteFile (to_slave, buf, nlen, &wLen, NULL);
 
-      if (ALWAYS_USE_PCON &&
-	  (ti.c_lflag & ISIG) && memchr (p, ti.c_cc[VINTR], len))
-	get_ttyp ()->kill_pgrp (SIGINT);
-
       if (ti.c_lflag & ICANON)
 	{
 	  if (memchr (buf, '\r', nlen))
@@ -2340,20 +2318,12 @@ fhandler_pty_master::write (const void *ptr, size_t len)
       else
 	SetEvent (input_available_event);
 
+      ReleaseMutex (input_mutex);
+
       mb_str_free (buf);
       return len;
     }
 
-  if (get_ttyp ()->switch_to_pcon_in &&
-      (ti.c_lflag & ISIG) &&
-      memchr (p, ti.c_cc[VINTR], len) &&
-      get_ttyp ()->getpgid () == get_ttyp ()->pcon_pid)
-    {
-      DWORD n;
-      /* Send ^C to pseudo console as well */
-      WriteFile (to_slave, "\003", 1, &n, 0);
-    }
-
   line_edit_status status = line_edit (p, len, ti, &ret);
   if (status > line_edit_signalled && status != line_edit_pipe_full)
     ret = -1;
@@ -2739,17 +2709,13 @@ fhandler_pty_slave::fixup_after_attach (bool native_maybe, int fd_set)
 	  /* Clear screen to synchronize pseudo console screen buffer
 	     with real terminal. This is necessary because pseudo
 	     console screen buffer is empty at start. */
-	  if (get_ttyp ()->num_pcon_attached_slaves == 0
-	      && !ALWAYS_USE_PCON)
+	  if (get_ttyp ()->num_pcon_attached_slaves == 0)
 	    /* Assume this is the first process using this pty slave. */
 	    get_ttyp ()->need_redraw_screen = true;
 
 	  get_ttyp ()->num_pcon_attached_slaves ++;
 	}
 
-      if (ALWAYS_USE_PCON && !isHybrid && pcon_attached_to == get_minor ())
-	set_ishybrid_and_switch_to_pcon (get_output_handle ());
-
       if (pcon_attached_to == get_minor () && native_maybe)
 	{
 	  if (fd == 0)
@@ -2801,7 +2767,8 @@ fhandler_pty_slave::fixup_after_exec ()
   /* Native windows program does not reset event on read.
      Therefore, reset here if no input is available. */
   DWORD bytes_in_pipe;
-  if (bytes_available (bytes_in_pipe) && !bytes_in_pipe)
+  if (!to_be_read_from_pcon () &&
+      bytes_available (bytes_in_pipe) && !bytes_in_pipe)
     ResetEvent (input_available_event);
 
   reset_switch_to_pcon ();
@@ -2841,7 +2808,6 @@ fhandler_pty_slave::fixup_after_exec ()
   if (get_ttyp ()->term_code_page == 0)
     setup_locale ();
 
-#if USE_API_HOOK
   /* Hook Console API */
   if (get_pseudo_console ())
     {
@@ -2875,7 +2841,6 @@ fhandler_pty_slave::fixup_after_exec ()
       DO_HOOK (NULL, CreateProcessA);
       DO_HOOK (NULL, CreateProcessW);
     }
-#endif /* USE_API_HOOK */
 }
 
 /* This thread function handles the master control pipe.  It waits for a
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index b3aedf20f..5048e549f 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -1211,29 +1211,6 @@ peek_pty_slave (select_record *s, bool from_select)
 	  goto out;
 	}
 
-      if (ptys->to_be_read_from_pcon ())
-	{
-	  if (ptys->is_line_input ())
-	    {
-	      INPUT_RECORD inp[INREC_SIZE];
-	      DWORD n;
-	      PeekConsoleInput (ptys->get_handle (), inp, INREC_SIZE, &n);
-	      bool end_of_line = false;
-	      while (n-- > 0)
-		if (inp[n].EventType == KEY_EVENT &&
-		    inp[n].Event.KeyEvent.bKeyDown &&
-		    inp[n].Event.KeyEvent.uChar.AsciiChar == '\r')
-		  end_of_line = true;
-	      if (end_of_line)
-		{
-		  gotone = s->read_ready = true;
-		  goto out;
-		}
-	      else
-		goto out;
-	    }
-	}
-
       if (IsEventSignalled (ptys->input_available_event))
 	{
 	  gotone = s->read_ready = true;
-- 
2.21.0

[PATCH 4/4] Cygwin: pty: Add missing member initialization for struct pipe_reply.

[Takashi Yano]

- For pseudo console support, struct pipe_reply was changed in the
  past, however, the initialization was not fixed.
---
 winsup/cygwin/fhandler_tty.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 36b3341b1..1c23c93e3 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2878,7 +2878,7 @@ fhandler_pty_master::pty_master_thread ()
   while (!exit && (ConnectNamedPipe (master_ctl, NULL)
 		   || GetLastError () == ERROR_PIPE_CONNECTED))
     {
-      pipe_reply repl = { NULL, NULL, NULL, 0 };
+      pipe_reply repl = { NULL, NULL, NULL, NULL, 0 };
       bool deimp = false;
       NTSTATUS allow = STATUS_ACCESS_DENIED;
       ACCESS_MASK access = EVENT_MODIFY_STATE;
-- 
2.21.0

[PATCH 0/4] Remove debug codes and organize with some fixes.

[Takashi Yano]

Takashi Yano (4):
  Cygwin: pty: Define mask_switch_to_pcon_in() in fhandler_tty.cc.
  Cygwin: pty: Avoid screen distortion on slave read.
  Cygwin: pty: Remove debug codes and organize related codes.
  Cygwin: pty: Add missing member initialization for struct pipe_reply.

 winsup/cygwin/fhandler.h      |  12 +--
 winsup/cygwin/fhandler_tty.cc | 144 +++++++++++++++-------------------
 winsup/cygwin/select.cc       |  23 ------
 3 files changed, 67 insertions(+), 112 deletions(-)

-- 
2.21.0

Re: [PATCH 0/4] Remove debug codes and organize with some fixes.

[Corinna Vinschen]

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

On Feb  9 23:45, Takashi Yano wrote:
> Takashi Yano (4):
>   Cygwin: pty: Define mask_switch_to_pcon_in() in fhandler_tty.cc.
>   Cygwin: pty: Avoid screen distortion on slave read.
>   Cygwin: pty: Remove debug codes and organize related codes.
>   Cygwin: pty: Add missing member initialization for struct pipe_reply.
> 
>  winsup/cygwin/fhandler.h      |  12 +--
>  winsup/cygwin/fhandler_tty.cc | 144 +++++++++++++++-------------------
>  winsup/cygwin/select.cc       |  23 ------
>  3 files changed, 67 insertions(+), 112 deletions(-)
> 
> -- 
> 2.21.0

Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]