[PATCH] ui/ncurses: Safely handle lost terminal control commands
Samuel Mendoza-Jonas
sam at mendozajonas.com
Mon Nov 13 15:01:20 AEDT 2017
On Fri, 2017-11-10 at 15:48 +1100, Cyril Bur wrote:
> 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?
Yep good catch.
>
> > + 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.
Yeah it's riiiight on the border
>
> > + 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);
For sure - I switched the logic around here a few times which probably
led to me leaving some of the duplication around.
>
> > + 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:
As much as I wish it were so...
> 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