diff mbox series

[RESEND,v3,1/2] cli: Correct several bugs in cli_getch()

Message ID 20230327193414.1803604-1-sjg@chromium.org
State Accepted
Commit 17b45e684af98c1cf37648ad05a98d500b367c5a
Delegated to: Tom Rini
Headers show
Series [RESEND,v3,1/2] cli: Correct several bugs in cli_getch() | expand

Commit Message

Simon Glass March 27, 2023, 7:34 p.m. UTC 
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 <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

Comments

Tom Rini March 28, 2023, 3:13 p.m. UTC | #1 
On Tue, 28 Mar 2023 08:34:13 +1300, Simon Glass wrote:

> 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
> 
> [...]

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/common/cli_getch.c b/common/cli_getch.c
index 87c23edcf4b4..61d4cb261b81 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 cc918f64e544..a5ab10f6f69b 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 000000000000..3dce4bdb0ef3
--- /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);