[PATCH] ui/ncurses: Safely handle lost terminal control commands
Cyril Bur
cyrilbur at gmail.com
Fri Nov 10 15:48:21 AEDT 2017
On Thu, 2017-11-09 at 17:40 +1100, Samuel Mendoza-Jonas wrote:
> Normally terminal control commands are caught and handled before ncurses
> or Petitboot could see them. However several combinations of broken
> terminal emulators or console connections can cause these command
> sequences to be seen by Petitboot, usually resulting in Petitboot
> exiting due to the ESC character and then the rest printed to the
> console.
>
> Aside from confusing the user this can also cancel autoboot, so it's
> important we don't let these sequences go unnoticed if possible.
> In ui/ncurses/console-codes we add a state machine that recognises the
> syntax of these control/escape sequences and handles the lost
> characters. We don't try to emulate the functionality of these commands,
> instead just logging them for reference.
The need for this to happen is upsetting.
Looks like it works, depending on how clean you want it to be I have a
few ideas, feel free to ignore up to 100% of them.
Reviewed-by: Cyril Bur <cyrilbur at gmail.com>
>
> Signed-off-by: Samuel Mendoza-Jonas <sam at mendozajonas.com>
> ---
> Makefile.am | 1 +
> test/ui/Makefile.am | 35 ++++++++++
> test/ui/console-sequence.c | 99 ++++++++++++++++++++++++++
> ui/ncurses/Makefile.am | 2 +
> ui/ncurses/console-codes.c | 170 +++++++++++++++++++++++++++++++++++++++++++++
> ui/ncurses/console-codes.h | 20 ++++++
> ui/ncurses/nc-cui.c | 25 +++++++
> 7 files changed, 352 insertions(+)
> create mode 100644 test/ui/Makefile.am
> create mode 100644 test/ui/console-sequence.c
> create mode 100644 ui/ncurses/console-codes.c
> create mode 100644 ui/ncurses/console-codes.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 15d561f..c0ad839 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -55,6 +55,7 @@ include test/Makefile.am
> include test/lib/Makefile.am
> include test/parser/Makefile.am
> include test/urls/Makefile.am
> +include test/ui/Makefile.am
> include ui/common/Makefile.am
>
> if WITH_NCURSES
> diff --git a/test/ui/Makefile.am b/test/ui/Makefile.am
> new file mode 100644
> index 0000000..19c3637
> --- /dev/null
> +++ b/test/ui/Makefile.am
> @@ -0,0 +1,35 @@
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; version 2 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
> +ui_TESTS = \
> + test/ui/console-sequence
> +
> +TESTS += $(ui_TESTS)
> +check_PROGRAMS += $(ui_TESTS)
> +
> +test_ui_console_sequence_SOURCES = \
> + test/ui/console-sequence.c
> +
> +test_ui_console_sequence_CPPFLAGS = \
> + -DPETITBOOT_TEST \
> + -I$(top_srcdir)/lib
> +
> +test_ui_console_sequence_LDFLAGS = \
> + $(core_lib)
> +
> +
> +
> +#$(ui_TESTS): LIBS += \
> +# ui/ncurses/libpbnc.la \
> +# $(core_lib) \
> +# @MENU_LIB@ @FORM_LIB@ @CURSES_LIB@
> diff --git a/test/ui/console-sequence.c b/test/ui/console-sequence.c
> new file mode 100644
> index 0000000..0818ebc
> --- /dev/null
> +++ b/test/ui/console-sequence.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (C) 2017 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +
> +#include "talloc/talloc.h"
> +
> +#define ERR (-1)
> +#define pb_log(...) printf(__VA_ARGS__)
> +#define pb_debug(...) printf(__VA_ARGS__)
> +
> +/*
> + * Several example terminal commands, see:
> + * https://vt100.net/docs/vt100-ug/chapter3.html
> + */
> +static char identify_rsp[] = {033, 0133, 077, 061, 073, 060, 0143, '\0'};
> +static char decdwl[] = {033, 043, 066, '\0'};
> +static char attrs[] = {033, 0133, 060, 073, 064, 073, 065, 0155, '\0'};
> +static char cursor[] = {033, 070, '\0'};
> +static char conf_test[] = {033, 0133, 062, 073, 061, 0171, '\0'};
> +static char status[] = {033, 0133, 060, 0156, '\0'};
> +static char erase_screen[] = {033, 0133, 062, 0112, '\0'};
> +static char status_ok_rsp[] = {033, 0133, 064, 0156, '\0'};
> +static char *ptr;
> +
> +static char getch(void)
> +{
> + if (!ptr || *ptr == '\0')
> + return -ERR;
> + return *ptr++;
> +}
> +
> +#include "ui/ncurses/console-codes.c"
> +
> +int main(void)
> +{
> + void *ctx;
> + char *seq;
> +
> + ctx = talloc_new(NULL);
> +
> + ptr = &identify_rsp[1];
> + printf("Identity response\n");
> + seq = handle_control_sequence(ctx, identify_rsp[0]);
> + assert(strncmp(seq, identify_rsp, strlen(identify_rsp)) == 0);
> +
> + printf("DECDWL\n");
> + ptr = &decdwl[1];
> + seq = handle_control_sequence(ctx, decdwl[0]);
> + assert(strncmp(seq, decdwl, strlen(decdwl)) == 0);
> +
> + printf("Attributes\n");
> + ptr = &attrs[1];
> + seq = handle_control_sequence(ctx, attrs[0]);
> + assert(strncmp(seq, attrs, strlen(attrs)) == 0);
> +
> + printf("Reset Cursor\n");
> + ptr = &cursor[1];
> + seq = handle_control_sequence(ctx, cursor[0]);
> + assert(strncmp(seq, cursor, strlen(cursor)) == 0);
> +
> + printf("Confidence Test\n");
> + ptr = &conf_test[1];
> + seq = handle_control_sequence(ctx, conf_test[0]);
> + assert(strncmp(seq, conf_test, strlen(conf_test)) == 0);
> +
> + printf("Status\n");
> + ptr = &status[1];
> + seq = handle_control_sequence(ctx, status[0]);
> + assert(strncmp(seq, status, strlen(status)) == 0);
> +
> + printf("Erase Screen\n");
> + ptr = &erase_screen[1];
> + seq = handle_control_sequence(ctx, erase_screen[0]);
> + assert(strncmp(seq, erase_screen, strlen(erase_screen)) == 0);
> +
> + printf("Status Response\n");
> + ptr = &status_ok_rsp[1];
> + seq = handle_control_sequence(ctx, status_ok_rsp[0]);
> + assert(strncmp(seq, status_ok_rsp, strlen(status_ok_rsp)) == 0);
> +
> + talloc_free(ctx);
> + return EXIT_SUCCESS;
> +}
> diff --git a/ui/ncurses/Makefile.am b/ui/ncurses/Makefile.am
> index 40e11b8..3bc5254 100644
> --- a/ui/ncurses/Makefile.am
> +++ b/ui/ncurses/Makefile.am
> @@ -24,6 +24,8 @@ ui_ncurses_libpbnc_la_SOURCES = \
> ui/ncurses/nc-config.c \
> ui/ncurses/nc-config.h \
> ui/ncurses/nc-config-help.c \
> + ui/ncurses/console-codes.c \
> + ui/ncurses/console-codes.h \
> ui/ncurses/nc-cui.c \
> ui/ncurses/nc-cui.h \
> ui/ncurses/nc-cui-help.c \
> diff --git a/ui/ncurses/console-codes.c b/ui/ncurses/console-codes.c
> new file mode 100644
> index 0000000..71d24ed
> --- /dev/null
> +++ b/ui/ncurses/console-codes.c
> @@ -0,0 +1,170 @@
> +/*
> + * Copyright (C) 2017 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#if defined(HAVE_CONFIG_H)
> +#include "config.h"
> +#endif
> +
> +#include <ctype.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdbool.h>
> +
> +#include "talloc/talloc.h"
> +
> +#ifndef PETITBOOT_TEST
> +#include "log/log.h"
> +#include "nc-scr.h"
> +#endif
> +
> +#include "console-codes.h"
> +
> +#define CSI_CHAR '['
> +#define INTER_CHAR_START 040
> +#define INTER_CHAR_END 057
> +#define ESC_SEQUENCE_FINAL_START 060
> +#define ESC_SEQUENCE_FINAL_END 0176
> +#define CTRL_SEQUENCE_FINAL_START 0100
> +#define CTRL_SEQUENCE_FINAL_END 0176
> +#define DEC_PARAMETER 077
> +
> +enum console_sequence_state {
> + CONSOLE_STATE_START,
> + CONSOLE_STATE_ESC_SEQ,
> + CONSOLE_STATE_CTRL_SEQ_START,
> + CONSOLE_STATE_CTRL_SEQ,
> + CONSOLE_STATE_DONE,
> + CONSOLE_STATE_CONFUSED,
> +};
> +
> +static inline bool is_intermediate(char c)
> +{
> + return c > INTER_CHAR_START && c < INTER_CHAR_END;
> +}
> +
> +static inline bool is_parameter(char c)
> +{
> + return (c >= 060 && c <= 071) || c == 073;
> +}
> +
> +static inline bool is_escape_final(char c)
> +{
> + return c >= ESC_SEQUENCE_FINAL_START && c < ESC_SEQUENCE_FINAL_END;
> +}
> +
> +static inline bool is_control_final(char c)
> +{
> + return c >= CTRL_SEQUENCE_FINAL_START && c <= CTRL_SEQUENCE_FINAL_END;
> +}
> +
> +/*
> + * Catch terminal control sequences that have accidentally been sent to
> + * Petitboot. These are of the form
> + * ESC I .. I F
> + * where I is an Intermediate Character and F is a Final Character, eg:
> + * ESC ^ [ ? 1 ; 0 c
> + * or ESC # 6
> + *
> + * This is based off the definitions provided by
> + * https://vt100.net/docs/vt100-ug/contents.html
> + */
> +char *handle_control_sequence(void *ctx, char start)
^ spare space
> +{
> + enum console_sequence_state state = CONSOLE_STATE_START;
> + bool in_sequence = true;
> + signed char c;
> + char *seq;
> +
Looks like start here must be an escape, is it worth asserting that?
> + seq = talloc_asprintf(ctx, "%c", start);
> +
> + while (in_sequence) {
> + switch (state) {
> + case CONSOLE_STATE_START:
> + c = getch();
> + if (c == CSI_CHAR)
Just in the interest of pretty code I wonder if you could have a helper
for c == CSI_CHAR... probably overkill... especially since its only
called here.
> + state = CONSOLE_STATE_CTRL_SEQ_START;
> + else if (is_intermediate(c))
> + state = CONSOLE_STATE_ESC_SEQ;
> + else if (is_escape_final(c))
> + state = CONSOLE_STATE_DONE;
> + else if (c != ERR) {
> + /* wait on c == ERR */
> + pb_debug("Unexpected start: \\x%x\n", c);
> + state = CONSOLE_STATE_CONFUSED;
> + }
> + if (c != ERR)
> + seq = talloc_asprintf_append(seq, "%c", c);
> + break;
> + case CONSOLE_STATE_ESC_SEQ:
> + c = getch();
> + if (is_intermediate(c))
> + state = CONSOLE_STATE_ESC_SEQ;
> + else if (is_escape_final(c))
> + state = CONSOLE_STATE_DONE;
> + else if (c != ERR) {
> + /* wait on c == ERR */
> + pb_debug("Unexpected character after intermediate: \\x%x\n",
> + c);
> + state = CONSOLE_STATE_CONFUSED;
> + }
> + if (c != ERR)
> + seq = talloc_asprintf_append(seq, "%c", c);
> + break;
> + case CONSOLE_STATE_CTRL_SEQ_START:
> + c= getch();
> + if (is_intermediate(c) || is_parameter(c) ||
> + c == DEC_PARAMETER)
> + state = CONSOLE_STATE_CTRL_SEQ;
> + else if (is_control_final(c))
> + state = CONSOLE_STATE_DONE;
> + else if (c != ERR) {
> + /* wait on c == ERR */
> + pb_debug("Unexpected character in param string: \\x%x\n",
> + c);
> + state = CONSOLE_STATE_CONFUSED;
> + }
> + if (c != ERR)
> + seq = talloc_asprintf_append(seq, "%c", c);
> + break;
> + case CONSOLE_STATE_CTRL_SEQ:
> + c = getch();
> + if (is_intermediate(c) || is_parameter(c))
> + state = CONSOLE_STATE_CTRL_SEQ;
> + else if (is_control_final(c))
> + state = CONSOLE_STATE_DONE;
> + else if (c != ERR) {
> + /* wait on c == ERR */
> + pb_debug("Unexpected character in param string: \\x%x\n",
> + c);
> + state = CONSOLE_STATE_CONFUSED;
> + }
> + if (c != ERR)
> + seq = talloc_asprintf_append(seq, "%c", c);
So basically whenever you call getch() you also log the returned
character. Is it worth doing something like this?
static signed char console_sequence_getch(char **seq)
{
signed char c = getch()
if (c != ERR)
*seq = talloc_asprintf_append(seq, "%c", c);
return c;
}
And then get rid of all those:
if (c != ERR)
seq = talloc_asprintf_append(seq, "%c", c);
> + break;
> + case CONSOLE_STATE_DONE:
> + in_sequence = false;
> + break;
> + case CONSOLE_STATE_CONFUSED:
> + /* fall-through */
> + default:
> + pb_debug("We got lost interpreting a control sequence!\n");
> + seq = talloc_asprintf_append(seq, "...");
> + in_sequence = false;
> + break;
> + };
> + }
> +
> + return seq;
> +}
> diff --git a/ui/ncurses/console-codes.h b/ui/ncurses/console-codes.h
> new file mode 100644
> index 0000000..6de54a7
> --- /dev/null
> +++ b/ui/ncurses/console-codes.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2017 IBM Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#if !defined(_PB_UI_CONSOLE_CODES)
> +#define _PB_UI_CONSOLE_CODES
> +
> +char *handle_control_sequence(void *ctx, char start);
> +
> +#endif
> diff --git a/ui/ncurses/nc-cui.c b/ui/ncurses/nc-cui.c
> index 6ced24c..44cce44 100644
> --- a/ui/ncurses/nc-cui.c
> +++ b/ui/ncurses/nc-cui.c
> @@ -21,6 +21,7 @@
> #endif
>
> #include <assert.h>
> +#include <ctype.h>
> #include <errno.h>
> #include <stdlib.h>
> #include <string.h>
> @@ -45,6 +46,7 @@
> #include "nc-statuslog.h"
> #include "nc-subset.h"
> #include "nc-plugin.h"
> +#include "console-codes.h"
>
> extern const struct help_text main_menu_help_text;
> extern const struct help_text plugin_menu_help_text;
> @@ -524,6 +526,9 @@ static bool process_global_keys(struct cui *cui, int key)
> static int cui_process_key(void *arg)
> {
> struct cui *cui = cui_from_arg(arg);
> + unsigned int i;
> + char *sequence;
> + int grab;
>
> assert(cui->current);
>
> @@ -535,6 +540,26 @@ static int cui_process_key(void *arg)
> if (c == ERR)
> break;
>
> + if (c == 27) {
> + /*
> + * If this is a console code sequence try to parse it
> + * and don't treat this as a key press.
> + */
> + grab = getch();
> + if (grab != ERR && grab != 27) {
> + ungetch(grab);
> + pb_debug("%s: Caught unhandled command sequence\n",
> + __func__);
> + sequence = handle_control_sequence(cui, c);
Not sure if you want to NULL check sequence here? It will always return
the start char and in petitboot talloc never fails right? I suggest:
pb_debug("Caught sequence ");
if (sequence) {
pb_debug(" (%zu): ", strlen(sequence));
for (i = 0; i < strlen(sequence); i++)
pb_debug("%x", sequence[i]);
^ Maybe octal?
pb_debug("\n");
} else {
pb_debug(" (0): (none)\n");
}
Something like that... in before I've made a horrific mistake.
> + pb_debug("Caught sequence (%zu): ",
> + strlen(sequence));
> + for (i = 0; i < strlen(sequence); i++)
> + pb_debug("%x", sequence[i]);
> + pb_debug("%s\n", sequence ? "" : "(none)");
> + continue;
> + }
> + }
> +
> if (!cui->has_input) {
> cui->has_input = true;
> if (cui->client) {
More information about the Petitboot
mailing list