[PATCH] cli: Consume invalid escape sequences early

[PATCH] cli: Consume invalid escape sequences early

[Yurii Monakov]

This commit fixes some issues with extra 'Esc' keys entered by user:

1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
=>
nknown command 'ry 'help'
=>
2. Sequence <Esc><p><r><i><Enter> gives:
=> ri
Unknown command 'ri' - try 'help'
=>
3. Extra 'Esc' key presses break backspace functionality.

Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
---
 common/cli_getch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/cli_getch.c b/common/cli_getch.c
index 61d4cb261b..0ee7908777 100644
--- a/common/cli_getch.c
+++ b/common/cli_getch.c
@@ -46,6 +46,8 @@ static int cli_ch_esc(struct cli_ch_state *cch, int ichar,
 	case 1:
 		if (ichar == '[' || ichar == 'O')
 			act = ESC_SAVE;
+		else
+			act = ESC_CONVERTED;
 		break;
 	case 2:
 		switch (ichar) {
-- 
2.34.1

Re: [PATCH] cli: Consume invalid escape sequences early

[Simon Glass]

Hi Yurii,

On Fri, 6 Oct 2023 at 15:32, Yurii Monakov <monakov.y@gmail.com> wrote:
>
> This commit fixes some issues with extra 'Esc' keys entered by user:
>
> 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> =>
> nknown command 'ry 'help'
> =>
> 2. Sequence <Esc><p><r><i><Enter> gives:
> => ri
> Unknown command 'ri' - try 'help'
> =>
> 3. Extra 'Esc' key presses break backspace functionality.
>
> Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> ---
>  common/cli_getch.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/common/cli_getch.c b/common/cli_getch.c
> index 61d4cb261b..0ee7908777 100644
> --- a/common/cli_getch.c
> +++ b/common/cli_getch.c
> @@ -46,6 +46,8 @@ static int cli_ch_esc(struct cli_ch_state *cch, int ichar,
>         case 1:
>                 if (ichar == '[' || ichar == 'O')
>                         act = ESC_SAVE;
> +               else
> +                       act = ESC_CONVERTED;
>                 break;
>         case 2:
>                 switch (ichar) {
> --
> 2.34.1
>

This is a bit unfortunate. Basically the problem (as I understand it)
is that characters are built up for processing so the expected ichar
== 0 is never passed in.

I think the fix is reasonable. Does it work as well if use:

               else if (ichar == '\e')
                       act = ESC_CONVERTED;

?

Also please can you add a test case o test/common/cread.c

https://u-boot.readthedocs.io/en/latest/develop/tests_sandbox.html

Regards,
Simon

[PATCH] cli: Consume invalid escape sequences early

[Yurii Monakov]

Hi Simon,

On Sun, Oct 8, 2023 at 2:10=E2=80=AFAM Simon Glass <sjg@google.com> wrote:
>
> Hi Yurii,
>
> On Fri, 6 Oct 2023 at 15:32, Yurii Monakov <monakov.y@gmail.com> wrote:
> >
> > This commit fixes some issues with extra 'Esc' keys entered by user:
> >
> > 1. Sequence <Esc><Esc><Enter> right after autoboot stop gives:
> > =3D>
> > nknown command 'ry 'help'
> > =3D>
> > 2. Sequence <Esc><p><r><i><Enter> gives:
> > =3D> ri
> > Unknown command 'ri' - try 'help'
> > =3D>
> > 3. Extra 'Esc' key presses break backspace functionality.
> >
> > Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> > ---
> >  common/cli_getch.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/common/cli_getch.c b/common/cli_getch.c
> > index 61d4cb261b..0ee7908777 100644
> > --- a/common/cli_getch.c
> > +++ b/common/cli_getch.c
> > @@ -46,6 +46,8 @@ static int cli_ch_esc(struct cli_ch_state *cch, int i=
char,
> >         case 1:
> >                 if (ichar =3D=3D '[' || ichar =3D=3D 'O')
> >                         act =3D ESC_SAVE;
> > +               else
> > +                       act =3D ESC_CONVERTED;
> >                 break;
> >         case 2:
> >                 switch (ichar) {
> > --
> > 2.34.1
> >
>
> This is a bit unfortunate. Basically the problem (as I understand it)
> is that characters are built up for processing so the expected ichar
> =3D=3D 0 is never passed in.
>
> I think the fix is reasonable. Does it work as well if use:
>
>                else if (ichar =3D=3D '\e')
>                        act =3D ESC_CONVERTED;
>
> ?
>
> Also please can you add a test case o test/common/cread.c
>
> https://u-boot.readthedocs.io/en/latest/develop/tests_sandbox.html
>
> Regards,
> Simon

> This is a bit unfortunate. Basically the problem (as I understand it)
The main problem is that extra Esc key presses are accumulated internally,
even if it is already clear that the current escape sequence is invalid.
This results in weird behaviour. For example, the next character after 'Esc=
' key
simply disappears from input and 'Unknown command' printed after 'Enter'.
Googling "u-boot 'nknown' ry help" gives plenty of such examples. This ofte=
n
happens when user stops autoboot pressing 'Esc' key.

> expected ichar =3D=3D 0 is never passed in
IIUYC, this is already checked in the calling function (cli_ch_process).

> Does it work as well if use:
It was my first fix attempt. But unfortunately it does not work, because if
the next character is not valid for the escape sequence it gets swallowed.

> Also please can you add a test case o test/common/cread.c
It will be quite easy. I've tested this in the sandbox (and with qemu) and
was not able to break something with outstanding 'Esc' key presses.

Should I resend this patch?

Best Regards,
Yurii