From 17b45e684af98c1cf37648ad05a98d500b367c5a Mon Sep 17 00:00:00 2001 From: Simon Glass Date: Tue, 28 Mar 2023 08:34:13 +1300 Subject: [PATCH] cli: Correct several bugs in cli_getch() 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 - 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 Reported-by: Heinrich Schuchardt --- 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 87c23ed..61d4cb2 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 cc918f6..a5ab10f 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 0000000..3dce4bd --- /dev/null +++ b/test/common/cread.c @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright 2023 Google LLC + */ + +#include +#include +#include +#include +#include + +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); -- 2.7.4