Received-SPF: pass (google.com: domain of linux-kernel+bounces-25495-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 14 Jan 2024 16:08:21 +0100
From: Martin Hostettler <textshell@uchuujin.de>
To: Jiri Slaby <jirislaby@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	Nicolas Pitre <nicolas.pitre@linaro.org>,
	Adam Borowski <kilobyte@angband.pl>,
	Egmont Koblinger <egmont@gmail.com>
Subject: Re: [PATCH 3/4] vt: ignore csi sequences with intermediate
 characters.
Message-ID: <ZaP45QY2WEsDqoxg@neutronstar.dyndns.org>
References: <20181215143423.4556-1-textshell@uchuujin.de>
 <20181215143423.4556-4-textshell@uchuujin.de>
 <f6190d39-6afe-4106-911e-00e93a7e0640@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f6190d39-6afe-4106-911e-00e93a7e0640@kernel.org>

On Thu, Dec 14, 2023 at 01:10:07PM +0100, Jiri Slaby wrote:
> On 15. 12. 18, 15:34, Martin Hostettler wrote:
> > Various csi sequences contain intermediate characters between the
> > parameters and the final character. Introduce a additional state that
> > cleanly ignores these sequences.
> > 
> > This allows the vt to ignore these sequences used by more capable
> > terminal implementations such as "request mode", etc.
> > 
> > Signed-off-by: Martin Hostettler <textshell@uchuujin.de>
> > ---
> >   drivers/tty/vt/vt.c | 11 ++++++++++-
> >   1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 448b4f6be7d1..24cd0e9c037b 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -2023,7 +2023,7 @@ static void restore_cur(struct vc_data *vc)
> >   }
> >   enum { ESnormal, ESesc, ESsquare, ESgetpars, ESfunckey,
> > -	EShash, ESsetG0, ESsetG1, ESpercent, ESignore, ESnonstd,
> > +	EShash, ESsetG0, ESsetG1, ESpercent, EScsiignore, ESnonstd,
> >   	ESpalette, ESosc };
> >   /* console_lock is held (except via vc_init()) */
> > @@ -2259,6 +2259,10 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
> >   			vc->vc_par[vc->vc_npar] += c - '0';
> >   			return;
> >   		}
> > +		if (c >= 0x20 && c <= 0x2f) {
> > +			vc->vc_state = EScsiignore;
> > +			return;
> > +		}
> >   		vc->vc_state = ESnormal;
> >   		switch(c) {
> >   		case 'h':
> > @@ -2421,6 +2425,11 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
> >   			return;
> >   		}
> >   		return;
> > +	case EScsiignore:
> > +		if (c >= 20 && c <= 0x3f)
> 
> Staring at the current code, I am confused as I cannot find out why "20".
> Was this supposed to be 0x20 (the same as above -- 0x20 is SPACE and that
> _is_ sensible)? Or why was this arbitrary 20 chosen?

I'm not sure what happend here. But i agree this should be 0x20 or it
should be removed (see below) but not decimal 20.

This is supposed to match what ECMA-48, xterm and the usual terminal
parsers do.

The most important behavior here is that common usages work, which
fortunatly it seems this did not break.

CAN, SUB and ESC are in the range 20 to 0x20, but they are already handled
before the switch. And 0x20 to 0x3f are properly ignored.

I think that is all that really matters for terminal interoperablity.

When comparing with how xterm handles this state [1], xterm ignores more
characters. If we want to match that, i think we would want to remove the
whole `c >= 20 &&` part.

xterm ignores 0-4, 6, 16-23, 25, 28-0x3f and 127.
Or in other words, it does not ignore
5 (ENQ), 7 (BEL), 8 (BS), 9 (TAB), 10-13, 14 (Shift out), 15 (shift in),
24 (CAN), 26 (SUB), 27 (ESC) and 0x40-126

This code already handles 7, 8-15, 24, 26, 27, 127 before the switch.
The code also does not implement ENQ so effectivly ignoring it is the same
as handling it.

But if we match xterm here, we should also match it in other cases like
ESsquare and ESgetpars.

So in summary i think we should fix this, but the fix does not need any
backporting to stable kernels.

Do you want to send a patch to fix this, or should i send a fix.

Do you have thoughts in which of the two plausible directions we should
change the code?

[1] https://github.com/ThomasDickey/xterm-snapshots/blob/3a151f2f31358d135204c8f90759f3cfd0697b9e/VTPrsTbl.c#L5290

> 
> thanks,
> -- 
> js
> suse labs
>