From 62d84b19359a8ddd3df5b6293d1b05ef5281f532 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 20 Oct 2014 19:09:19 -0700 Subject: [PATCH] Fix race condition in timer disarm/delete. When setting a repeat timer using the SIGEV_THREAD mechanism, it's possible that the callback can be called after the timer is disarmed or deleted. This happens because the kernel can generate signals that the timer thread will continue to handle even after the timer is supposed to be off. Add two new tests to verify that disarming/deleting doesn't continue to call the callback. Modify the repeat test to finish more quickly than before. Refactor the Counter implementation a bit. Bug: 18039727 (cherry pick from commit 0724132c3263145f2a667f453a199d313a5b3d9f) Change-Id: I135726ea4038a47920a6c511708813b1a9996c42 --- libc/bionic/posix_timers.cpp | 20 +++++++- tests/time_test.cpp | 95 ++++++++++++++++++++++++++++++------ 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/libc/bionic/posix_timers.cpp b/libc/bionic/posix_timers.cpp index 7ad0ef1fb..3c664d9c8 100644 --- a/libc/bionic/posix_timers.cpp +++ b/libc/bionic/posix_timers.cpp @@ -62,6 +62,7 @@ struct PosixTimer { pthread_t callback_thread; void (*callback)(sigval_t); sigval_t callback_argument; + volatile bool armed; }; static __kernel_timer_t to_kernel_timer_id(timer_t timer) { @@ -83,7 +84,7 @@ static void* __timer_thread_start(void* arg) { continue; } - if (si.si_code == SI_TIMER) { + if (si.si_code == SI_TIMER && timer->armed) { // This signal was sent because a timer fired, so call the callback. timer->callback(timer->callback_argument); } else if (si.si_code == SI_TKILL) { @@ -95,6 +96,9 @@ static void* __timer_thread_start(void* arg) { } static void __timer_thread_stop(PosixTimer* timer) { + // Immediately mark the timer as disarmed so even if some events + // continue to happen, the callback won't be called. + timer->armed = false; pthread_kill(timer->callback_thread, TIMER_SIGNAL); } @@ -121,6 +125,7 @@ int timer_create(clockid_t clock_id, sigevent* evp, timer_t* timer_id) { // Otherwise, this must be SIGEV_THREAD timer... timer->callback = evp->sigev_notify_function; timer->callback_argument = evp->sigev_value; + timer->armed = false; // Check arguments that the kernel doesn't care about but we do. if (timer->callback == NULL) { @@ -200,7 +205,18 @@ int timer_gettime(timer_t id, itimerspec* ts) { // http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html int timer_settime(timer_t id, int flags, const itimerspec* ts, itimerspec* ots) { - return __timer_settime(to_kernel_timer_id(id), flags, ts, ots); + PosixTimer* timer= reinterpret_cast(id); + int rc = __timer_settime(timer->kernel_timer_id, flags, ts, ots); + if (rc == 0) { + // Mark the timer as either being armed or disarmed. This avoids the + // callback being called after the disarm for SIGEV_THREAD timers only. + if (ts->it_value.tv_sec != 0 || ts->it_value.tv_nsec != 0) { + timer->armed = true; + } else { + timer->armed = false; + } + } + return rc; } // http://pubs.opengroup.org/onlinepubs/9699919799/functions/timer_getoverrun.html diff --git a/tests/time_test.cpp b/tests/time_test.cpp index 26c699339..7a551b42a 100644 --- a/tests/time_test.cpp +++ b/tests/time_test.cpp @@ -206,24 +206,46 @@ struct Counter { volatile int value; timer_t timer_id; sigevent_t se; + bool timer_valid; - Counter(void (*fn)(sigval_t)) : value(0) { + Counter(void (*fn)(sigval_t)) : value(0), timer_valid(false) { memset(&se, 0, sizeof(se)); se.sigev_notify = SIGEV_THREAD; se.sigev_notify_function = fn; se.sigev_value.sival_ptr = this; + Create(); } void Create() { + ASSERT_FALSE(timer_valid); ASSERT_EQ(0, timer_create(CLOCK_REALTIME, &se, &timer_id)); + timer_valid = true; + } + + void DeleteTimer() { + ASSERT_TRUE(timer_valid); + ASSERT_EQ(0, timer_delete(timer_id)); + timer_valid = false; } ~Counter() { - if (timer_delete(timer_id) != 0) { - abort(); + if (timer_valid) { + DeleteTimer(); } } + void SetTime(time_t value_s, time_t value_ns, time_t interval_s, time_t interval_ns) { + ::SetTime(timer_id, value_s, value_ns, interval_s, interval_ns); + } + + bool ValueUpdated() { + volatile int current_value = value; + time_t start = time(NULL); + while (current_value == value && (time(NULL) - start) < 5) { + } + return current_value != value; + } + static void CountNotifyFunction(sigval_t value) { Counter* cd = reinterpret_cast(value.sival_ptr); ++cd->value; @@ -234,17 +256,17 @@ struct Counter { ++cd->value; // Setting the initial expiration time to 0 disarms the timer. - SetTime(cd->timer_id, 0, 0, 1, 0); + cd->SetTime(0, 0, 1, 0); } }; TEST(time, timer_settime_0) { Counter counter(Counter::CountAndDisarmNotifyFunction); - counter.Create(); + ASSERT_TRUE(counter.timer_valid); ASSERT_EQ(0, counter.value); - SetTime(counter.timer_id, 0, 1, 1, 0); + counter.SetTime(0, 1, 1, 0); usleep(500000); // The count should just be 1 because we disarmed the timer the first time it fired. @@ -253,15 +275,14 @@ TEST(time, timer_settime_0) { TEST(time, timer_settime_repeats) { Counter counter(Counter::CountNotifyFunction); - counter.Create(); + ASSERT_TRUE(counter.timer_valid); ASSERT_EQ(0, counter.value); - SetTime(counter.timer_id, 0, 1, 0, 10); - usleep(500000); - - // The count should just be > 1 because we let the timer repeat. - ASSERT_GT(counter.value, 1); + counter.SetTime(0, 1, 0, 10); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); } static int timer_create_NULL_signal_handler_invocation_count = 0; @@ -321,17 +342,17 @@ TEST(time, timer_delete_multiple) { TEST(time, timer_create_multiple) { Counter counter1(Counter::CountNotifyFunction); - counter1.Create(); + ASSERT_TRUE(counter1.timer_valid); Counter counter2(Counter::CountNotifyFunction); - counter2.Create(); + ASSERT_TRUE(counter2.timer_valid); Counter counter3(Counter::CountNotifyFunction); - counter3.Create(); + ASSERT_TRUE(counter3.timer_valid); ASSERT_EQ(0, counter1.value); ASSERT_EQ(0, counter2.value); ASSERT_EQ(0, counter3.value); - SetTime(counter2.timer_id, 0, 1, 0, 0); + counter2.SetTime(0, 1, 0, 0); usleep(500000); EXPECT_EQ(0, counter1.value); @@ -425,3 +446,45 @@ TEST(time, clock_nanosleep) { timespec out; ASSERT_EQ(EINVAL, clock_nanosleep(-1, 0, &in, &out)); } + +// Test to verify that disarming a repeatable timer disables the +// callbacks. +TEST(time, timer_disarm_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_TRUE(counter.timer_valid); + + ASSERT_EQ(0, counter.value); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.SetTime(0, 0, 1, 0); + volatile int value = counter.value; + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.value); +} + +// Test to verify that deleting a repeatable timer disables the +// callbacks. +TEST(time, timer_delete_terminates) { + Counter counter(Counter::CountNotifyFunction); + ASSERT_TRUE(counter.timer_valid); + + ASSERT_EQ(0, counter.value); + + counter.SetTime(0, 1, 0, 1); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + ASSERT_TRUE(counter.ValueUpdated()); + + counter.DeleteTimer(); + volatile int value = counter.value; + usleep(500000); + + // Verify the counter has not been incremented. + ASSERT_EQ(value, counter.value); +}