From 9886750f0bf271507d3893376e0cbe369ffcad84 Mon Sep 17 00:00:00 2001 From: Anton Lindqvist Date: Mon, 11 Sep 2017 19:58:20 +0200 Subject: [PATCH] Explicitly handle control characters The current SIGINT handler is not considered safe since it invokes functions that are not considered asynchronous safe[1]. Quoting the CERT C Coding Standard: In general, it is not safe to invoke I/O functions from within signal handlers. Instead, do not turn control characters into signal but instead handle the relevant ones explicitly. A pleasant side-effect is being able to test the Ctrl-C error path. While here, fix the broken suspend/resume behavior by resetting the initial terminal state prior suspending and re-read it upon resume. I do actually use suspend/resume while running pick when I forgot to check something that will affect the choice to select. [1] https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers --- Makefile.am | 2 +- pick.1 | 5 ++++ pick.c | 75 +++++++++++++++++++++++++++++++++-------------------- tests/64.t | 5 ++++ 4 files changed, 58 insertions(+), 29 deletions(-) create mode 100644 tests/64.t diff --git a/Makefile.am b/Makefile.am index 9382536c..24f0253b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -20,7 +20,7 @@ TESTS=tests/01.t tests/02.t tests/03.t tests/04.t tests/05.t tests/06.t \ tests/43.t tests/44.t tests/45.t tests/46.t tests/47.t tests/48.t \ tests/49.t tests/50.t tests/51.t tests/52.t tests/53.t tests/54.t \ tests/55.t tests/56.t tests/57.t tests/58.t tests/59.t tests/60.t \ - tests/61.t tests/62.t tests/63.t + tests/61.t tests/62.t tests/63.t tests/64.t TEST_EXTENSIONS=.t T_LOG_COMPILER=$(top_srcdir)/tests/pick-test.sh AM_COLOR_TESTS=no diff --git a/pick.1 b/pick.1 index f134dbac..3691c039 100644 --- a/pick.1 +++ b/pick.1 @@ -51,6 +51,11 @@ Disable the use of the alternate screen terminal feature. .Bl -tag -width XXXX .It Ic Ctrl-C Exit with a erroneous status without outputting the selected choice. +While this command often being defined as Ctrl-C it is determined by the +.Dv VINTR +control character, +see +.Xr termios 4 . .It Ic Ctrl-L Redraw interface with respect to the current size of the terminal. .It Ic Up Ns / Ns Ic Down | Ic Ctrl-P Ns / Ns Ic Ctrl-N diff --git a/pick.c b/pick.c index d426bbfa..91500427 100644 --- a/pick.c +++ b/pick.c @@ -40,11 +40,13 @@ enum key { DEL, ENTER, CTRL_A, + CTRL_C, CTRL_E, CTRL_K, CTRL_L, CTRL_U, CTRL_W, + CTRL_Z, RIGHT, LEFT, LINE_DOWN, @@ -71,7 +73,6 @@ static char *eager_strpbrk(const char *, const char *); static void filter_choices(void); static char *get_choices(void); static enum key get_key(const char **); -static __dead void handle_sigint(int); static void handle_sigwinch(int); static int isu8cont(unsigned char); static int isu8start(unsigned char); @@ -86,15 +87,15 @@ static const char *strcasechr(const char *, const char *); static void toggle_sigwinch(int); static int tty_getc(void); static const char *tty_getcap(char *); -static void tty_init(void); +static void tty_init(int); static const char *tty_parm1(const char *, int); static int tty_putc(int); -static void tty_restore(void); +static void tty_restore(int); static void tty_size(void); static __dead void usage(void); static int xmbtowc(wchar_t *, const char *); -static struct termios original_attributes; +static struct termios tio; static struct { size_t size; size_t length; @@ -116,6 +117,7 @@ main(int argc, char *argv[]) char *input; int c; int output_description = 0; + int rc = 0; setlocale(LC_CTYPE, ""); @@ -169,7 +171,7 @@ main(int argc, char *argv[]) } input = get_choices(); - tty_init(); + tty_init(1); #ifdef HAVE_PLEDGE if (pledge("stdio tty", NULL) == -1) @@ -177,18 +179,20 @@ main(int argc, char *argv[]) #endif choice = selected_choice(); - tty_restore(); + tty_restore(1); if (choice != NULL) { printf("%s\n", choice->string); if (output_description) printf("%s\n", choice->description); + } else { + rc = 1; } free(input); free(choices.v); free(query); - return 0; + return rc; } __dead void @@ -355,6 +359,13 @@ selected_choice(void) choices.v[choices.length].string = query; choices.v[choices.length].description = ""; return &choices.v[choices.length]; + case CTRL_C: + return NULL; + case CTRL_Z: + tty_restore(0); + kill(getpid(), SIGTSTP); + tty_init(0); + break; case BACKSPACE: if (cursor_position > 0) { for (length = 1; @@ -644,25 +655,26 @@ strcasechr(const char *s1, const char *s2) } void -tty_init(void) +tty_init(int doinit) { struct termios new_attributes; - if ((tty_in = fopen("/dev/tty", "r")) == NULL) + if (doinit && (tty_in = fopen("/dev/tty", "r")) == NULL) err(1, "fopen"); - tcgetattr(fileno(tty_in), &original_attributes); - new_attributes = original_attributes; + tcgetattr(fileno(tty_in), &tio); + new_attributes = tio; new_attributes.c_iflag |= ICRNL; /* map CR to NL */ - new_attributes.c_lflag &= ~(ECHO | ICANON | IEXTEN); + new_attributes.c_lflag &= ~(ECHO | ICANON | IEXTEN | ISIG); new_attributes.c_cc[VMIN] = 1; new_attributes.c_cc[VTIME] = 0; tcsetattr(fileno(tty_in), TCSANOW, &new_attributes); - if ((tty_out = fopen("/dev/tty", "w")) == NULL) + if (doinit && (tty_out = fopen("/dev/tty", "w")) == NULL) err(1, "fopen"); - setupterm((char *)0, fileno(tty_out), (int *)0); + if (doinit) + setupterm((char *)0, fileno(tty_out), (int *)0); tty_size(); @@ -671,7 +683,6 @@ tty_init(void) tty_putp(keypad_xmit, 0); - signal(SIGINT, handle_sigint); toggle_sigwinch(0); } @@ -684,13 +695,6 @@ tty_putc(int c) return c; } -__dead void -handle_sigint(int sig __attribute__((unused))) -{ - tty_restore(); - exit(1); -} - void handle_sigwinch(int sig) { @@ -711,10 +715,11 @@ toggle_sigwinch(int enable) } void -tty_restore(void) +tty_restore(int doclose) { - tcsetattr(fileno(tty_in), TCSANOW, &original_attributes); - fclose(tty_in); + tcsetattr(fileno(tty_in), TCSANOW, &tio); + if (doclose) + fclose(tty_in); tty_putp(keypad_local, 0); tty_putp(carriage_return, 1); /* move cursor to first column */ @@ -723,7 +728,10 @@ tty_restore(void) if (use_alternate_screen) tty_putp(exit_ca_mode, 0); - fclose(tty_out); + if (doclose) + fclose(tty_out); + else + fflush(tty_out); } void @@ -873,18 +881,21 @@ print_choices(size_t offset, size_t selection) enum key get_key(const char **key) { -#define CAP(k, s) { k, s, NULL, 0 } -#define KEY(k, s) { k, NULL, s, sizeof(s) - 1 } +#define CAP(k, s) { k, s, NULL, 0, -1 } +#define KEY(k, s) { k, NULL, s, sizeof(s) - 1, -1 } +#define TIO(k, i) { k, NULL, NULL, 0, i } static struct { enum key key; char *cap; const char *str; size_t len; + int tio; } keys[] = { KEY(ALT_ENTER, "\033\n"), KEY(BACKSPACE, "\177"), KEY(BACKSPACE, "\b"), KEY(CTRL_A, "\001"), + TIO(CTRL_C, VINTR), KEY(CTRL_E, "\005"), KEY(CTRL_K, "\013"), KEY(CTRL_L, "\014"), @@ -892,6 +903,7 @@ get_key(const char **key) KEY(CTRL_W, "\027"), KEY(CTRL_W, "\033\177"), KEY(CTRL_W, "\033\b"), + TIO(CTRL_Z, VSUSP), CAP(DEL, "kdch1"), KEY(DEL, "\004"), CAP(END, "kend"), @@ -936,6 +948,13 @@ get_key(const char **key) for (;;) { for (i = 0; keys[i].key != UNKNOWN; i++) { + if (keys[i].tio >= 0) { + if (len == 1 && + CCEQ(tio.c_cc[keys[i].tio], *buf)) + return keys[i].key; + continue; + } + if (keys[i].str == NULL) { keys[i].str = tty_getcap(keys[i].cap); keys[i].len = strlen(keys[i].str); diff --git a/tests/64.t b/tests/64.t new file mode 100644 index 00000000..49f51e3e --- /dev/null +++ b/tests/64.t @@ -0,0 +1,5 @@ +description: Ctrl-C aborts without outputting the selected choice +keys: \003 # CTRL_C +exit: 1 +stdin: +a