From: Paul Floyd Date: Sat, 25 Jan 2025 13:54:08 +0000 (+0100) Subject: Bug 492678 - SIGALRM race condition (sighandler called after timer disarmed X-Git-Tag: VALGRIND_3_25_0~169 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3a1498f13f1676a20ab80fc553fd4f6b94b3a347;p=thirdparty%2Fvalgrind.git Bug 492678 - SIGALRM race condition (sighandler called after timer disarmed --- diff --git a/.gitignore b/.gitignore index d44246b8b..4a6874e5c 100644 --- a/.gitignore +++ b/.gitignore @@ -1534,6 +1534,7 @@ /none/tests/bug129866 /none/tests/bug234814 /none/tests/bug491394 +/none/tests/bug492678 /none/tests/closeall /none/tests/coolo_sigaction /none/tests/coolo_strlen @@ -1645,6 +1646,7 @@ /none/tests/thread-exits /none/tests/threaded-fork /none/tests/threadederrno +/none/tests/timer_delete /none/tests/timestamp /none/tests/tls /none/tests/track-fds-exec-children diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index d358c90eb..339724938 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -2411,6 +2411,7 @@ PRE(sys_timer_delete) { PRINT("sys_timer_delete( %#" FMT_REGWORD "x )", ARG1); PRE_REG_READ1(long, "timer_delete", vki_timer_t, timerid); + *flags |= SfPollAfter; } // SYS_ktimer_settime 237 diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c index a74b8f1d2..c281021a9 100644 --- a/coregrind/m_syswrap/syswrap-generic.c +++ b/coregrind/m_syswrap/syswrap-generic.c @@ -2959,6 +2959,14 @@ PRE(sys_setitimer) &(value->it_interval)); PRE_timeval_READ( "setitimer(&value->it_value)", &(value->it_value)); + // "Setting it_value to 0 disables a timer" + // poll for signals in that case + if (ML_(safe_to_deref)(value, sizeof(*value))) { + if (value->it_value.tv_sec == 0 && + value->it_value.tv_usec == 0) { + *flags |= SfPollAfter; + } + } } if (ARG3 != (Addr)NULL) { struct vki_itimerval *ovalue = (struct vki_itimerval*)(Addr)ARG3; diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c index e8978b5bc..83af91344 100644 --- a/coregrind/m_syswrap/syswrap-linux.c +++ b/coregrind/m_syswrap/syswrap-linux.c @@ -3244,6 +3244,7 @@ PRE(sys_timer_delete) { PRINT("sys_timer_delete( %#" FMT_REGWORD "x )", ARG1); PRE_REG_READ1(long, "timer_delete", vki_timer_t, timerid); + *flags |= SfPollAfter; } /* --------------------------------------------------------------------- diff --git a/coregrind/m_syswrap/syswrap-solaris.c b/coregrind/m_syswrap/syswrap-solaris.c index 104a809d3..9abeb8500 100644 --- a/coregrind/m_syswrap/syswrap-solaris.c +++ b/coregrind/m_syswrap/syswrap-solaris.c @@ -8531,6 +8531,7 @@ PRE(sys_timer_delete) /* int timer_delete(timer_t timerid); */ PRINT("sys_timer_delete ( %ld )", SARG1); PRE_REG_READ1(long, "timer_delete", vki_timer_t, timerid); + *flags |= SfPollAfter; } PRE(sys_timer_settime) diff --git a/none/tests/Makefile.am b/none/tests/Makefile.am index 3dc876251..2fa43587e 100644 --- a/none/tests/Makefile.am +++ b/none/tests/Makefile.am @@ -103,6 +103,7 @@ EXTRA_DIST = \ bug129866.vgtest bug129866.stderr.exp bug129866.stdout.exp \ bug234814.vgtest bug234814.stderr.exp bug234814.stdout.exp \ bug491394.vgtest bug491394.stderr.exp \ + bug492678.vgtest bug492678.stderr.exp \ closeall.stderr.exp closeall.vgtest \ cmdline0.stderr.exp cmdline0.stdout.exp cmdline0.vgtest \ cmdline1.stderr.exp cmdline1.stdout.exp cmdline1.vgtest \ @@ -241,6 +242,7 @@ EXTRA_DIST = \ threaded-fork.stderr.exp threaded-fork.stdout.exp threaded-fork.vgtest \ threadederrno.stderr.exp threadederrno.stdout.exp \ threadederrno.vgtest \ + timer_delete.vgtest timer_delete.stderr.exp \ timestamp.stderr.exp timestamp.vgtest \ tls.vgtest tls.stderr.exp tls.stdout.exp \ track-fds-exec-children.vgtest track-fds-exec-children.stderr.exp \ @@ -267,7 +269,7 @@ check_PROGRAMS = \ args \ async-sigs \ bitfield1 \ - bug129866 bug234814 \ + bug129866 bug234814 bug492678\ closeall coolo_strlen \ discard exec-sigmask execve faultstatus fcntl_setown \ fdleak_cmsg fdleak_creat fdleak_dup fdleak_dup2 \ @@ -300,6 +302,7 @@ check_PROGRAMS = \ thread-exits \ threaded-fork \ threadederrno \ + timer_delete \ timestamp \ tls \ tls.so \ @@ -439,6 +442,7 @@ thread_exits_LDADD = -lpthread threaded_fork_LDADD = -lpthread threadederrno_CFLAGS = $(AM_CFLAGS) threadederrno_LDADD = -lpthread +timer_delete_LDADD = -lrt timestamp_CFLAGS = ${AM_CFLAGS} @FLAG_W_NO_USE_AFTER_FREE@ tls_SOURCES = tls.c tls2.c tls_DEPENDENCIES = tls.so tls2.so diff --git a/none/tests/bug492678.c b/none/tests/bug492678.c new file mode 100644 index 000000000..6e4ca80f4 --- /dev/null +++ b/none/tests/bug492678.c @@ -0,0 +1,104 @@ +#include +#include +#include +#include +#include +#include + +#define CHECK_RES(res) \ + if (res == -1) \ + { \ + fprintf(stderr, "unexpected error at line %d: %s", __LINE__, strerror(errno)); \ + exit(EXIT_FAILURE); \ + } + +static int tries = 0; + +static void sigalrm_handler(int signum) +{ + (void)signum; + // no need to do anything for the experiment +} + +// using this handler in place of SIG_DFL to monitor when the "default" handler is called +static void sigalrm_default_handler(int signum) +{ + (void)signum; + fprintf(stderr, + "ERROR: unreachable code reached: sigalarm timer was disarmed, but still received SIGALRM (race condition " + "tried %d times)\n", + tries); + exit(EXIT_FAILURE); +} + +void sigalrm_timer_destroy(void) +{ + int res; + + // REMOVE TIMER + const struct itimerval zero = { + .it_interval = + { + .tv_sec = 0, + .tv_usec = 0, + }, + .it_value = + { + .tv_sec = 0, + .tv_usec = 0, + }, + }; + res = setitimer(ITIMER_REAL, &zero, NULL); + CHECK_RES(res) + + // AND THEN REMOVE SIGNAL HANDLER + struct sigaction sigact; + sigact.sa_flags = 0; + sigact.sa_handler = sigalrm_default_handler; // using this handler in place of SIG_DFL to monitor when the "default" + // handler is called + res = sigemptyset(&sigact.sa_mask); + CHECK_RES(res) + res = sigaction(SIGALRM, &sigact, NULL); + CHECK_RES(res) +} + +void sigalrm_timer_setup(const struct timeval *period) +{ + int res; + + // SIGNAL + struct sigaction sigact; + sigact.sa_flags = 0; + sigact.sa_handler = sigalrm_handler; + res = sigemptyset(&sigact.sa_mask); + CHECK_RES(res) + res = sigaction(SIGALRM, &sigact, NULL); + CHECK_RES(res) + + // TIMER + const struct itimerval timer_val = { + .it_interval = *period, + .it_value = *period, + }; + res = setitimer(ITIMER_REAL, &timer_val, NULL); + CHECK_RES(res); +} + +static void try_race_condition(void) +{ + ++tries; + const struct timeval clk_period = { + .tv_sec = 0, + .tv_usec = 1, + }; + sigalrm_timer_setup(&clk_period); + sigalrm_timer_destroy(); +} + +int main(void) +{ + for (int i = 0; i < 10000; ++i) + { + try_race_condition(); + } +} diff --git a/none/tests/bug492678.stderr.exp b/none/tests/bug492678.stderr.exp new file mode 100644 index 000000000..e69de29bb diff --git a/none/tests/bug492678.vgtest b/none/tests/bug492678.vgtest new file mode 100644 index 000000000..39ff27650 --- /dev/null +++ b/none/tests/bug492678.vgtest @@ -0,0 +1,3 @@ +prog: bug492678 +vgopts: -q + diff --git a/none/tests/timer_delete.c b/none/tests/timer_delete.c new file mode 100644 index 000000000..4a103d8f8 --- /dev/null +++ b/none/tests/timer_delete.c @@ -0,0 +1,102 @@ +#include +#include +#include +#include +#include +#include + +timer_t timerid; + +#define CHECK_RES(res) \ + if (res == -1) \ + { \ + fprintf(stderr, "unexpected error at line %d: %s", __LINE__, strerror(errno)); \ + exit(EXIT_FAILURE); \ + } + +static int tries = 0; + +static void sigalrm_handler(int signum) +{ + (void)signum; + // no need to do anything for the experiment +} + +// using this handler in place of SIG_DFL to monitor when the "default" handler is called +static void sigalrm_default_handler(int signum) +{ + (void)signum; + fprintf(stderr, + "ERROR: unreachable code reached: sigalarm timer was disarmed, but still received SIGALRM (race condition " + "tried %d times)\n", + tries); + exit(EXIT_FAILURE); +} + +void sigalrm_timer_destroy(void) +{ + int res; + + // REMOVE TIMER + res = timer_delete(timerid); + CHECK_RES(res) + + // AND THEN REMOVE SIGNAL HANDLER + struct sigaction sigact; + sigact.sa_flags = 0; + sigact.sa_handler = sigalrm_default_handler; // using this handler in place of SIG_DFL to monitor when the "default" + // handler is called + res = sigemptyset(&sigact.sa_mask); + CHECK_RES(res) + res = sigaction(SIGALRM, &sigact, NULL); + CHECK_RES(res) +} + +void sigalrm_timer_setup(const struct timespec *period) +{ + int res; + + // SIGNAL + struct sigaction sigact; + sigact.sa_flags = 0; + sigact.sa_handler = sigalrm_handler; + res = sigemptyset(&sigact.sa_mask); + CHECK_RES(res) + res = sigaction(SIGALRM, &sigact, NULL); + CHECK_RES(res) + + // SIGEVENT + struct sigevent sev; + sev.sigev_notify = SIGEV_SIGNAL; + sev.sigev_signo = SIGALRM; + sev.sigev_value.sival_ptr = &timerid; + res = timer_create(CLOCK_REALTIME, &sev, &timerid); + CHECK_RES(res); + + // TIMER + const struct itimerspec timer_val = { + .it_interval = *period, + .it_value = *period, + }; + res = timer_settime(timerid, 0, &timer_val, NULL); + CHECK_RES(res); +} + +static void try_race_condition(void) +{ + ++tries; + const struct timespec clk_period = { + .tv_sec = 0, + .tv_nsec = 20000 + }; + sigalrm_timer_setup(&clk_period); + sigalrm_timer_destroy(); +} + +int main(void) +{ + for (int i = 0; i < 10000; ++i) + { + try_race_condition(); + } +} diff --git a/none/tests/timer_delete.stderr.exp b/none/tests/timer_delete.stderr.exp new file mode 100644 index 000000000..e69de29bb