[PATCH v2 1/2] cli: Correct several bugs in cli_getch()

[Heinrich Schuchardt]

On 2/6/23 18:12, Simon Glass wrote:
> Hi Heinrich,
>
> On Sun, 5 Feb 2023 at 14:29, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> Am 5. Februar 2023 03:08:54 MEZ schrieb Simon Glass <sjg at chromium.org>:
>>> This function does not behave as expected when unknown escape sequences
>>> are sent to it:
>>>
>>> - it fails to store (and thus echo) the last character of the invalid
>>>   sequence
>>
>> What would be the benefit of echoing it? Is there any reason to assume that we want an output for an escape sequence that we fail to parse?
>
> Yes. In fact we don't even know if it is invalid or not. It is just
> that U-Boot doesn't support it. The mimics the behaviour we used to
> have, I believe.

We should assume that escape sequences are valid as they are either
coming from our drivers or passed from a terminal emulation to our
serial console.

When I open the eficonfig command and in the main menu I press <PG-UP>
the program returns to the command line and a ~ is printed. The expected
behavior is that <PG-UP> is ignored.

This is the escape sequence:
1b 5b 35 7e

cli_ch_process(0x1b) returns 0.
cli_ch_process(0x5b) returns 0.
cli_ch_process(0x35) returns 0x1b.

This 0x1b is converted to BKEY_QUIT in bootmenu_conv_key(). For unknown
escape sequences cli_ch_process() should return 0 and set cch->esc_len =
0 to minimize interference with the caller.

Mapping control sequences to characters 0x01-0x1b in cli_ch_esc() is not
viable: F1-F12 with combined with SHIFT, ALT, CTRL, META already require
192 different values for representation. But that is stuff for future
patches.

Best regards

Heinrich

>
> Regards,
> Simon
>
>
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> - it fails to set esc_len to 0 when it finishes emitting the invalid
>>>   sequence, meaning that the following character will appear to be part
>>>   of a new escape sequence
>>> - it processes the first character of the rejected sequence as a valid
>>>   character, just starting the sequence all over again
>>>
>>> The last two bugs conspire to produce an "impossible condition #876"
>>> message which is the main symptom of this behaviour.
>>>
>>> Fix these bugs and add a test to verify the behaviour.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> Reported-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>> common/cli_getch.c   |  5 +++--
>>> test/common/Makefile |  1 +
>>> test/common/cread.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 52 insertions(+), 2 deletions(-)
>>> create mode 100644 test/common/cread.c
>>>
>>> diff --git a/common/cli_getch.c b/common/cli_getch.c
>>> index 87c23edcf4b..61d4cb261b8 100644
>>> --- a/common/cli_getch.c
>>> +++ b/common/cli_getch.c
>>> @@ -129,7 +129,7 @@ static int cli_ch_esc(struct cli_ch_state *cch, int ichar,
>>>
>>>        *actp = act;
>>>
>>> -      return act == ESC_CONVERTED ? ichar : 0;
>>> +      return ichar;
>>> }
>>>
>>> int cli_ch_process(struct cli_ch_state *cch, int ichar)
>>> @@ -145,6 +145,7 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
>>>                                return cch->esc_save[cch->emit_upto++];
>>>                        cch->emit_upto = 0;
>>>                        cch->emitting = false;
>>> +                      cch->esc_len = 0;
>>>                }
>>>                return 0;
>>>        } else if (ichar == -ETIMEDOUT) {
>>> @@ -185,7 +186,7 @@ int cli_ch_process(struct cli_ch_state *cch, int ichar)
>>>                        cch->esc_save[cch->esc_len++] = ichar;
>>>                        ichar = cch->esc_save[cch->emit_upto++];
>>>                        cch->emitting = true;
>>> -                      break;
>>> +                      return ichar;
>>>                case ESC_CONVERTED:
>>>                        /* valid escape sequence, return the resulting char */
>>>                        cch->esc_len = 0;
>>> diff --git a/test/common/Makefile b/test/common/Makefile
>>> index cc918f64e54..a5ab10f6f69 100644
>>> --- a/test/common/Makefile
>>> +++ b/test/common/Makefile
>>> @@ -3,3 +3,4 @@ obj-y += cmd_ut_common.o
>>> obj-$(CONFIG_AUTOBOOT) += test_autoboot.o
>>> obj-$(CONFIG_CYCLIC) += cyclic.o
>>> obj-$(CONFIG_EVENT) += event.o
>>> +obj-y += cread.o
>>> diff --git a/test/common/cread.c b/test/common/cread.c
>>> new file mode 100644
>>> index 00000000000..3dce4bdb0ef
>>> --- /dev/null
>>> +++ b/test/common/cread.c
>>> @@ -0,0 +1,48 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright 2023 Google LLC
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <cli.h>
>>> +#include <test/common.h>
>>> +#include <test/test.h>
>>> +#include <test/ut.h>
>>> +
>>> +static int cli_ch_test(struct unit_test_state *uts)
>>> +{
>>> +      struct cli_ch_state s_cch, *cch = &s_cch;
>>> +
>>> +      cli_ch_init(cch);
>>> +
>>> +      /* should be nothing to return at first */
>>> +      ut_asserteq(0, cli_ch_process(cch, 0));
>>> +
>>> +      /* check normal entry */
>>> +      ut_asserteq('a', cli_ch_process(cch, 'a'));
>>> +      ut_asserteq('b', cli_ch_process(cch, 'b'));
>>> +      ut_asserteq('c', cli_ch_process(cch, 'c'));
>>> +      ut_asserteq(0, cli_ch_process(cch, 0));
>>> +
>>> +      /* send an invalid escape sequence */
>>> +      ut_asserteq(0, cli_ch_process(cch, '\e'));
>>> +      ut_asserteq(0, cli_ch_process(cch, '['));
>>> +
>>> +      /*
>>> +       * with the next char it sees that the sequence is invalid, so starts
>>> +       * emitting it
>>> +       */
>>> +      ut_asserteq('\e', cli_ch_process(cch, 'X'));
>>> +
>>> +      /* now we set 0 bytes to empty the buffer */
>>> +      ut_asserteq('[', cli_ch_process(cch, 0));
>>> +      ut_asserteq('X', cli_ch_process(cch, 0));
>>> +      ut_asserteq(0, cli_ch_process(cch, 0));
>>> +
>>> +      /* things are normal again */
>>> +      ut_asserteq('a', cli_ch_process(cch, 'a'));
>>> +      ut_asserteq(0, cli_ch_process(cch, 0));
>>> +
>>> +      return 0;
>>> +}
>>> +COMMON_TEST(cli_ch_test, 0);