Re: [PATCH] Cygwin: pty: Disable clear screen for ssh sessions with -t option.

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Takashi,

On Oct 23 12:27, Takashi Yano wrote:
> On Tue, 22 Oct 2019 15:40:48 +0200
> Corinna Vinschen wrote:
> > Am I doing something wrong?  This code crashes mintty on my
> > installation.  At start, a string of "6n6n6n6n..." appears and then
> > mintty exits.
> 
> I cannot reproduce that.... How about this one?

In my limited testing it seems to work nicely.

> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index da6119dfb2cf..26f99669f4fc 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -1296,6 +1296,30 @@ detach:
>    restore_reattach_pcon ();
>  }
>  
> +/* If master process is running as service, attaching to
> +   pseudo console should be done in fork. If attaching
> +   is done in spawn for inetd or sshd, it fails because
> +   the helper process is running as privileged user while
> +   slave process is not. This function is used to determine
> +   if the process is running as a srvice or not. */
> +static bool
> +is_running_as_service (void)

This function should probably use check_token_membership(PSID).
I'm also not quite sure if checking for mandatory_system_integrity_sid
makes sense.  Are there examples where the service SID is missing
but the integrity is set to system integrity level?

>  ssize_t __stdcall
>  fhandler_pty_slave::write (const void *ptr, size_t len)
>  {
> @@ -1305,6 +1329,30 @@ fhandler_pty_slave::write (const void *ptr, size_t len)
>    if (bg <= bg_eof)
>      return (ssize_t) bg;
>  
> +  if (get_ttyp ()->need_clear_screen_on_write)
> +    {
> +      if (is_running_as_service ())

This check is redundant.  The only way to set need_clear_screen_on_write
to true is if is_running_as_service() was already checked for below.

> +	{
> +	  struct termios ti, ti_new;
> +	  tcgetattr (&ti);
> +	  ti_new = ti;
> +	  ti_new.c_lflag &= (~ICANON | ECHO);
> +	  tcsetattr (TCSANOW, &ti_new);
> +	  char buf[32];
> +	  DWORD n;
> +	  WriteFile (get_output_handle_cyg (), "\033[6n", 4, &n, NULL);
> +	  ReadFile (get_handle_cyg (), buf, sizeof(buf)-1, &n, NULL);
> +	  ResetEvent (input_available_event);
> +	  tcsetattr (TCSANOW, &ti);
> +	  buf[n] = '\0';
> +	  int rows, cols;
> +	  sscanf (buf, "\033[%d;%dR", &rows, &cols);

Wouldn't it be safer to initialize rows and cols to 0 and to check the
result of sscanf?

>  HANDLE
> diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
> index 927d7afd9384..c7aeef85b482 100644
> --- a/winsup/cygwin/tty.h
> +++ b/winsup/cygwin/tty.h
> @@ -106,6 +106,7 @@ private:
>    int num_pcon_attached_slaves;
>    UINT term_code_page;
>    bool need_clear_screen;
> +  bool need_clear_screen_on_write;

Maybe the name should be aligned to the fact that it doesn't clear the
screen anymore?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: signature.asc
Description: PGP signature