On 2/6/23 18:12, Simon Glass wrote:

Hi Heinrich,

On Sun, 5 Feb 2023 at 14:29, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

Am 5. Februar 2023 03:08:54 MEZ schrieb Simon Glass sjg@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@chromium.org Reported-by: Heinrich Schuchardt xypron.glpk@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);