From 12566e7f9b5e5c5d445bc4d36991d134b431dc6c Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 17 Jan 2022 03:02:40 +0100 Subject: [PATCH] ash,hush: fix handling of SIGINT while waiting for interactive input function old new delta lineedit_read_key 160 237 +77 __pgetc 522 589 +67 fgetc_interactive 244 309 +65 safe_read_key - 39 +39 read_key 588 607 +19 record_pending_signo 23 32 +9 signal_handler 75 81 +6 .rodata 104312 104309 -3 ------------------------------------------------------------------------------ (add/remove: 1/0 grow/shrink: 6/1 up/down: 282/-3) Total: 279 bytes Signed-off-by: Denys Vlasenko --- editors/vi.c | 4 +-- include/libbb.h | 5 +++- libbb/lineedit.c | 24 ++++++++++++++-- libbb/read_key.c | 16 +++++++++-- miscutils/hexedit.c | 2 +- miscutils/less.c | 4 +-- procps/top.c | 2 +- shell/ash.c | 39 ++++++++++++++++++++------ shell/hush.c | 67 +++++++++++++++++++++++++++++++-------------- 9 files changed, 122 insertions(+), 41 deletions(-) diff --git a/editors/vi.c b/editors/vi.c index 3dbe5b471..d37cd48a3 100644 --- a/editors/vi.c +++ b/editors/vi.c @@ -1122,7 +1122,7 @@ static int readit(void) // read (maybe cursor) key from stdin // on nonblocking stdin. // Note: read_key sets errno to 0 on success. again: - c = read_key(STDIN_FILENO, readbuffer, /*timeout:*/ -1); + c = safe_read_key(STDIN_FILENO, readbuffer, /*timeout:*/ -1); if (c == -1) { // EOF/error if (errno == EAGAIN) // paranoia goto again; @@ -4770,7 +4770,7 @@ static void edit_file(char *fn) uint64_t k; write1(ESC"[999;999H" ESC"[6n"); fflush_all(); - k = read_key(STDIN_FILENO, readbuffer, /*timeout_ms:*/ 100); + k = safe_read_key(STDIN_FILENO, readbuffer, /*timeout_ms:*/ 100); if ((int32_t)k == KEYCODE_CURSOR_POS) { uint32_t rc = (k >> 32); columns = (rc & 0x7fff); diff --git a/include/libbb.h b/include/libbb.h index 91b456915..b45ce91c5 100644 --- a/include/libbb.h +++ b/include/libbb.h @@ -1908,6 +1908,8 @@ enum { * >=0: poll() for TIMEOUT milliseconds, return -1/EAGAIN on timeout */ int64_t read_key(int fd, char *buffer, int timeout) FAST_FUNC; +/* This version loops on EINTR: */ +int64_t safe_read_key(int fd, char *buffer, int timeout) FAST_FUNC; void read_key_ungets(char *buffer, const char *str, unsigned len) FAST_FUNC; @@ -1961,7 +1963,8 @@ enum { USERNAME_COMPLETION = 4 * ENABLE_FEATURE_USERNAME_COMPLETION, VI_MODE = 8 * ENABLE_FEATURE_EDITING_VI, WITH_PATH_LOOKUP = 0x10, - FOR_SHELL = DO_HISTORY | TAB_COMPLETION | USERNAME_COMPLETION, + LI_INTERRUPTIBLE = 0x20, + FOR_SHELL = DO_HISTORY | TAB_COMPLETION | USERNAME_COMPLETION | LI_INTERRUPTIBLE, }; line_input_t *new_line_input_t(int flags) FAST_FUNC; #if ENABLE_FEATURE_EDITING_SAVEHISTORY diff --git a/libbb/lineedit.c b/libbb/lineedit.c index e14c78707..f76afd37d 100644 --- a/libbb/lineedit.c +++ b/libbb/lineedit.c @@ -2161,12 +2161,30 @@ static int lineedit_read_key(char *read_key_buffer, int timeout) * insist on full MB_CUR_MAX buffer to declare input like * "\xff\n",pause,"ls\n" invalid and thus won't lose "ls". * + * If LI_INTERRUPTIBLE, return -1 if got EINTR in poll() + * inside read_key, or if bb_got_signal != 0 (IOW: if signal + * arrived before poll() is reached). + * * Note: read_key sets errno to 0 on success. */ - IF_FEATURE_EDITING_WINCH(S.ok_to_redraw = 1;) - ic = read_key(STDIN_FILENO, read_key_buffer, timeout); - IF_FEATURE_EDITING_WINCH(S.ok_to_redraw = 0;) + do { + if ((state->flags & LI_INTERRUPTIBLE) && bb_got_signal) { + errno = EINTR; + return -1; + } +//FIXME: still races here with signals, but small window to poll() inside read_key + IF_FEATURE_EDITING_WINCH(S.ok_to_redraw = 1;) + ic = read_key(STDIN_FILENO, read_key_buffer, timeout); + IF_FEATURE_EDITING_WINCH(S.ok_to_redraw = 0;) + } while (!(state->flags & LI_INTERRUPTIBLE) && errno == EINTR); + if (errno) { + /* LI_INTERRUPTIBLE can bail out with EINTR here, + * but nothing really guarantees that bb_got_signal + * is nonzero. Follow the least surprise principle: + */ + if (errno == EINTR && bb_got_signal == 0) + bb_got_signal = 255; /* something nonzero */ #if ENABLE_UNICODE_SUPPORT if (errno == EAGAIN && unicode_idx != 0) goto pushback; diff --git a/libbb/read_key.c b/libbb/read_key.c index 03b7da656..829ae215c 100644 --- a/libbb/read_key.c +++ b/libbb/read_key.c @@ -126,7 +126,10 @@ int64_t FAST_FUNC read_key(int fd, char *buffer, int timeout) * if fd can be in non-blocking mode. */ if (timeout >= -1) { - if (safe_poll(&pfd, 1, timeout) == 0) { + n = poll(&pfd, 1, timeout); + if (n < 0 && errno == EINTR) + return n; + if (n == 0) { /* Timed out */ errno = EAGAIN; return -1; @@ -138,7 +141,7 @@ int64_t FAST_FUNC read_key(int fd, char *buffer, int timeout) * When we were reading 3 bytes here, we were eating * "li" too, and cat was getting wrong input. */ - n = safe_read(fd, buffer, 1); + n = read(fd, buffer, 1); if (n <= 0) return -1; } @@ -284,6 +287,15 @@ int64_t FAST_FUNC read_key(int fd, char *buffer, int timeout) goto start_over; } +int64_t FAST_FUNC safe_read_key(int fd, char *buffer, int timeout) +{ + int64_t r; + do { + r = read_key(fd, buffer, timeout); + } while (errno == EINTR); + return r; +} + void FAST_FUNC read_key_ungets(char *buffer, const char *str, unsigned len) { unsigned cur_len = (unsigned char)buffer[0]; diff --git a/miscutils/hexedit.c b/miscutils/hexedit.c index f8ff9b62b..15ad78377 100644 --- a/miscutils/hexedit.c +++ b/miscutils/hexedit.c @@ -292,7 +292,7 @@ int hexedit_main(int argc UNUSED_PARAM, char **argv) fflush_all(); G.in_read_key = 1; if (!bb_got_signal) - key = read_key(STDIN_FILENO, G.read_key_buffer, -1); + key = safe_read_key(STDIN_FILENO, G.read_key_buffer, -1); G.in_read_key = 0; if (bb_got_signal) key = CTRL('X'); diff --git a/miscutils/less.c b/miscutils/less.c index 82c4b21f0..8a0525cb7 100644 --- a/miscutils/less.c +++ b/miscutils/less.c @@ -1137,9 +1137,9 @@ static int64_t getch_nowait(void) #endif } - /* We have kbd_fd in O_NONBLOCK mode, read inside read_key() + /* We have kbd_fd in O_NONBLOCK mode, read inside safe_read_key() * would not block even if there is no input available */ - key64 = read_key(kbd_fd, kbd_input, /*timeout off:*/ -2); + key64 = safe_read_key(kbd_fd, kbd_input, /*timeout off:*/ -2); if ((int)key64 == -1) { if (errno == EAGAIN) { /* No keyboard input available. Since poll() did return, diff --git a/procps/top.c b/procps/top.c index 4cd545c69..804d6f258 100644 --- a/procps/top.c +++ b/procps/top.c @@ -913,7 +913,7 @@ static unsigned handle_input(unsigned scan_mask, duration_t interval) while (1) { int32_t c; - c = read_key(STDIN_FILENO, G.kbd_input, interval * 1000); + c = safe_read_key(STDIN_FILENO, G.kbd_input, interval * 1000); if (c == -1 && errno != EAGAIN) { /* error/EOF */ option_mask32 |= OPT_EOF; diff --git a/shell/ash.c b/shell/ash.c index 086773dd7..55df54bd0 100644 --- a/shell/ash.c +++ b/shell/ash.c @@ -3679,7 +3679,9 @@ signal_handler(int signo) if (!trap[SIGCHLD]) return; } - +#if ENABLE_FEATURE_EDITING + bb_got_signal = signo; /* for read_line_input: "we got a signal" */ +#endif gotsig[signo - 1] = 1; pending_sig = signo; @@ -10784,33 +10786,52 @@ preadfd(void) # endif reinit_unicode_for_ash(); again: -//BUG: not in INT_OFF/INT_ON section - SIGINT et al would longjmp out of read_line_input()! -//This would cause a memory leak in interactive shell -//(repeated internal allocations in read_line_input): -// (while kill -INT $$; do :; done) & + /* For shell, LI_INTERRUPTIBLE is set: + * read_line_input will abort on either + * getting EINTR in poll(), or if it sees bb_got_signal != 0 + * (IOW: if signal arrives before poll() is reached). + * Interactive testcases: + * (while kill -INT $$; do sleep 1; done) & + * #^^^ prints ^C, prints prompt, repeats + * trap 'echo I' int; (while kill -INT $$; do sleep 1; done) & + * #^^^ prints ^C, prints "I", prints prompt, repeats + * trap 'echo T' term; (while kill $$; do sleep 1; done) & + * #^^^ prints "T", prints prompt, repeats + * #(bash 5.0.17 exits after first "T", looks like a bug) + */ + bb_got_signal = 0; + INT_OFF; /* no longjmp'ing out of read_line_input please */ nr = read_line_input(line_input_state, cmdedit_prompt, buf, IBUFSIZ); + if (bb_got_signal == SIGINT) + write(STDOUT_FILENO, "^C\n", 3); + INT_ON; /* here non-blocked SIGINT will longjmp */ if (nr == 0) { /* ^C pressed, "convert" to SIGINT */ - write(STDOUT_FILENO, "^C", 2); - raise(SIGINT); + write(STDOUT_FILENO, "^C\n", 3); + raise(SIGINT); /* here non-blocked SIGINT will longjmp */ /* raise(SIGINT) did not work! (e.g. if SIGINT * is SIG_IGNed on startup, it stays SIG_IGNed) */ if (trap[SIGINT]) { + empty_line_input: buf[0] = '\n'; buf[1] = '\0'; return 1; } exitstatus = 128 + SIGINT; /* bash behavior on ^C + ignored SIGINT: */ - write(STDOUT_FILENO, "\n", 1); goto again; } if (nr < 0) { if (errno == 0) { - /* Ctrl+D pressed */ + /* ^D pressed */ nr = 0; } + else if (errno == EINTR) { /* got signal? */ + if (bb_got_signal != SIGINT) + write(STDOUT_FILENO, "\n", 1); + goto empty_line_input; + } # if ENABLE_ASH_IDLE_TIMEOUT else if (errno == EAGAIN && timeout > 0) { puts("\007timed out waiting for input: auto-logout"); diff --git a/shell/hush.c b/shell/hush.c index 7d0dc67e4..6dc2ecaac 100644 --- a/shell/hush.c +++ b/shell/hush.c @@ -918,6 +918,7 @@ struct globals { #if ENABLE_HUSH_INTERACTIVE smallint promptmode; /* 0: PS1, 1: PS2 */ #endif + /* set by signal handler if SIGINT is received _and_ its trap is not set */ smallint flag_SIGINT; #if ENABLE_HUSH_LOOPS smallint flag_break_continue; @@ -1944,6 +1945,9 @@ enum { static void record_pending_signo(int sig) { sigaddset(&G.pending_set, sig); +#if ENABLE_FEATURE_EDITING + bb_got_signal = sig; /* for read_line_input: "we got a signal" */ +#endif #if ENABLE_HUSH_FAST if (sig == SIGCHLD) { G.count_SIGCHLD++; @@ -2652,30 +2656,53 @@ static int get_user_input(struct in_str *i) for (;;) { reinit_unicode_for_hush(); G.flag_SIGINT = 0; - /* buglet: SIGINT will not make new prompt to appear _at once_, - * only after . (^C works immediately) */ - r = read_line_input(G.line_input_state, prompt_str, + + bb_got_signal = 0; + if (!sigisemptyset(&G.pending_set)) { + /* Whoops, already got a signal, do not call read_line_input */ + bb_got_signal = r = -1; + } else { + /* For shell, LI_INTERRUPTIBLE is set: + * read_line_input will abort on either + * getting EINTR in poll(), or if it sees bb_got_signal != 0 + * (IOW: if signal arrives before poll() is reached). + * Interactive testcases: + * (while kill -INT $$; do sleep 1; done) & + * #^^^ prints ^C, prints prompt, repeats + * trap 'echo I' int; (while kill -INT $$; do sleep 1; done) & + * #^^^ prints ^C, prints "I", prints prompt, repeats + * trap 'echo T' term; (while kill $$; do sleep 1; done) & + * #^^^ prints "T", prints prompt, repeats + * #(bash 5.0.17 exits after first "T", looks like a bug) + */ + r = read_line_input(G.line_input_state, prompt_str, G.user_input_buf, CONFIG_FEATURE_EDITING_MAX_LEN-1 - ); - /* read_line_input intercepts ^C, "convert" it to SIGINT */ - if (r == 0) { - raise(SIGINT); + ); + /* read_line_input intercepts ^C, "convert" it to SIGINT */ + if (r == 0) + raise(SIGINT); + } + /* bash prints ^C (before running a trap, if any) + * both on keyboard ^C and on real SIGINT (non-kbd generated). + */ + if (sigismember(&G.pending_set, SIGINT)) { + write(STDOUT_FILENO, "^C\n", 3); + G.last_exitcode = 128 | SIGINT; } check_and_run_traps(); - if (r != 0 && !G.flag_SIGINT) + if (r == 0) /* keyboard ^C? */ + continue; /* go back, read another input line */ + if (r > 0) /* normal input? (no ^C, no ^D, no signals) */ break; - /* ^C or SIGINT: repeat */ - /* bash prints ^C even on real SIGINT (non-kbd generated) */ - write(STDOUT_FILENO, "^C\n", 3); - G.last_exitcode = 128 | SIGINT; - } - if (r < 0) { - /* EOF/error detected */ - /* ^D on interactive input goes to next line before exiting: */ - write(STDOUT_FILENO, "\n", 1); - i->p = NULL; - i->peek_buf[0] = r = EOF; - return r; + if (!bb_got_signal) { + /* r < 0: ^D/EOF/error detected (but not signal) */ + /* ^D on interactive input goes to next line before exiting: */ + write(STDOUT_FILENO, "\n", 1); + i->p = NULL; + i->peek_buf[0] = r = EOF; + return r; + } + /* it was a signal: go back, read another input line */ } i->p = G.user_input_buf; return (unsigned char)*i->p++;