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
Change-Id: I73192c915cdacf608521b1792c54e5af14a34907

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<PosixTimer*>(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

tests/time_test.cpp

@@ -205,24 +205,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<Counter*>(value.sival_ptr);
     ++cd->value;
@@ -233,17 +255,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.
@@ -252,15 +274,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;
@@ -320,17 +341,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);
@@ -403,3 +424,45 @@ TEST(time, clock_gettime) {
   ASSERT_EQ(0, ts2.tv_sec);
   ASSERT_LT(ts2.tv_nsec, 1000000);
 }
+
+// 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);
+}