[git commit] shell: fix race between signal handlers setting bb_got_signal and poll()
Denys Vlasenko
vda.linux at googlemail.com
Wed Jul 2 20:42:47 UTC 2025
commit: https://git.busybox.net/busybox/commit/?id=36ac283682bb176987c0c09e5f8c7e7e96fb1651
branch: https://git.busybox.net/busybox/commit/?id=refs/heads/master
function old new delta
__ppoll_time64 - 211 +211
check_got_signal_and_poll - 164 +164
read_key 607 601 -6
shell_builtin_read 1328 1318 -10
------------------------------------------------------------------------------
(add/remove: 4/0 grow/shrink: 0/2 up/down: 375/-16) Total: 359 bytes
Signed-off-by: Denys Vlasenko <vda.linux at googlemail.com>
---
include/libbb.h | 1 +
libbb/lineedit.c | 1 -
libbb/poll_with_signals.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
libbb/read_key.c | 14 +++++++++-----
miscutils/less.c | 2 +-
shell/ash.c | 2 +-
shell/hush.c | 2 +-
shell/shell_common.c | 9 +++++----
8 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/include/libbb.h b/include/libbb.h
index 287103756..801f072fa 100644
--- a/include/libbb.h
+++ b/include/libbb.h
@@ -1951,6 +1951,7 @@ int64_t read_key(int fd, char *buffer, int timeout) FAST_FUNC;
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;
+int check_got_signal_and_poll(struct pollfd pfd[1], int timeout) FAST_FUNC;
#if ENABLE_FEATURE_EDITING
/* It's NOT just ENABLEd or disabled. It's a number: */
diff --git a/libbb/lineedit.c b/libbb/lineedit.c
index ea216e16c..10a83bcb7 100644
--- a/libbb/lineedit.c
+++ b/libbb/lineedit.c
@@ -2195,7 +2195,6 @@ static int lineedit_read_key(char *read_key_buffer, int timeout)
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;)
/* errno = 0; - read_key does this itself */
ic = read_key(STDIN_FILENO, read_key_buffer, timeout);
diff --git a/libbb/poll_with_signals.c b/libbb/poll_with_signals.c
new file mode 100644
index 000000000..47419f240
--- /dev/null
+++ b/libbb/poll_with_signals.c
@@ -0,0 +1,48 @@
+/* vi: set sw=4 ts=4: */
+/*
+ * Utility routines.
+ *
+ * Copyright (C) 2025 Denys Vlasenko <vda.linux at googlemail.com>
+ *
+ * Licensed under GPLv2, see file LICENSE in this source tree.
+ */
+//kbuild:lib-y += poll_with_signals.o
+
+#include "libbb.h"
+
+/* Shells, for example, need their line input and "read" builtin
+ * to be interruptible, and the naive handling of it a-la:
+ * if (bb_got_signal) {
+ * errno = EINTR;
+ * return -1;
+ * }
+ * poll(pfd, 1, -1); // signal here would set EINTR
+ * is racy.
+ * This is a bit heavy-handed, but safe wrt races:
+ */
+int FAST_FUNC check_got_signal_and_poll(struct pollfd pfd[1], int timeout)
+{
+ int n;
+ struct timespec tv;
+ sigset_t orig_mask;
+
+ if (bb_got_signal) /* optimization */
+ goto eintr;
+
+ if (timeout >= 0) {
+ tv.tv_sec = timeout / 1000;
+ tv.tv_nsec = (timeout % 1000) * 1000000;
+ }
+ /* test bb_got_signal, then poll(), atomically wrt signals */
+ sigfillset(&orig_mask);
+ sigprocmask2(SIG_BLOCK, &orig_mask);
+ if (bb_got_signal) {
+ sigprocmask2(SIG_SETMASK, &orig_mask);
+ eintr:
+ errno = EINTR; /* inform the caller that we got a signal */
+ return -1;
+ }
+ n = ppoll(pfd, 1, timeout >= 0 ? &tv : NULL, &orig_mask);
+ sigprocmask2(SIG_SETMASK, &orig_mask);
+ return n;
+}
diff --git a/libbb/read_key.c b/libbb/read_key.c
index 1bea75fcf..3df9769f7 100644
--- a/libbb/read_key.c
+++ b/libbb/read_key.c
@@ -121,12 +121,16 @@ int64_t FAST_FUNC read_key(int fd, char *buffer, int timeout)
errno = 0;
n = (unsigned char)buffer[-1];
if (n == 0) {
- /* If no data, wait for input.
- * If requested, wait TIMEOUT ms. TIMEOUT = -1 is useful
- * if fd can be in non-blocking mode.
- */
+ /* No data. Wait for input. */
+
+ /* timeout == -2 means "do not poll". Else: */
if (timeout >= -1) {
- n = poll(&pfd, 1, timeout);
+ /* We must poll even if timeout == -1:
+ * we want to be interrupted if signal arrives,
+ * regardless of SA_RESTART-ness of that signal!
+ */
+ /* test bb_got_signal, then poll(), atomically wrt signals */
+ n = check_got_signal_and_poll(pfd, timeout);
if (n < 0 && errno == EINTR)
return n;
if (n == 0) {
diff --git a/miscutils/less.c b/miscutils/less.c
index 8a0525cb7..2204b1cec 100644
--- a/miscutils/less.c
+++ b/miscutils/less.c
@@ -1139,7 +1139,7 @@ static int64_t getch_nowait(void)
/* We have kbd_fd in O_NONBLOCK mode, read inside safe_read_key()
* would not block even if there is no input available */
- key64 = safe_read_key(kbd_fd, kbd_input, /*timeout off:*/ -2);
+ key64 = safe_read_key(kbd_fd, kbd_input, /*do not poll:*/ -2);
if ((int)key64 == -1) {
if (errno == EAGAIN) {
/* No keyboard input available. Since poll() did return,
diff --git a/shell/ash.c b/shell/ash.c
index 92b1df5f8..16eb88a7b 100644
--- a/shell/ash.c
+++ b/shell/ash.c
@@ -3687,7 +3687,7 @@ signal_handler(int signo)
return;
}
#if ENABLE_FEATURE_EDITING
- bb_got_signal = signo; /* for read_line_input: "we got a signal" */
+ bb_got_signal = signo; /* for read_line_input / read builtin: "we got a signal" */
#endif
gotsig[signo - 1] = 1;
pending_sig = signo;
diff --git a/shell/hush.c b/shell/hush.c
index 37cfecc08..70b730f67 100644
--- a/shell/hush.c
+++ b/shell/hush.c
@@ -1973,7 +1973,7 @@ static void record_pending_signo(int sig)
|| (G_traps && G_traps[SIGCHLD] && G_traps[SIGCHLD][0])
/* ^^^ if SIGCHLD, interrupt line reading only if it has a trap */
) {
- bb_got_signal = sig; /* for read_line_input: "we got a signal" */
+ bb_got_signal = sig; /* for read_line_input / read builtin: "we got a signal" */
}
#endif
#if ENABLE_HUSH_FAST
diff --git a/shell/shell_common.c b/shell/shell_common.c
index 2baa9d3a8..754fef34b 100644
--- a/shell/shell_common.c
+++ b/shell/shell_common.c
@@ -214,12 +214,13 @@ shell_builtin_read(struct builtin_read_params *params)
* regardless of SA_RESTART-ness of that signal!
*/
errno = 0;
- pfd[0].events = POLLIN;
-//TODO race with a signal arriving just before the poll!
- if (poll(pfd, 1, timeout) <= 0) {
+ pfd->events = POLLIN;
+
+ /* test bb_got_signal, then poll(), atomically wrt signals */
+ if (check_got_signal_and_poll(pfd, timeout) <= 0) {
/* timed out, or some error */
err = errno;
- if (!err) {
+ if (!err) { /* timed out */
retval = (const char *)(uintptr_t)2;
break;
}
More information about the busybox-cvs
mailing list