Merge "Fix race condition in timer disarm/delete." into lmp-mr1-dev

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) {
+    if (timer_valid) {
-      abort();
+      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);
+  counter.SetTime(0, 1, 0, 10);
-  usleep(500000);
+  ASSERT_TRUE(counter.ValueUpdated());
-
+  ASSERT_TRUE(counter.ValueUpdated());
-  // The count should just be > 1 because we let the timer repeat.
+  ASSERT_TRUE(counter.ValueUpdated());
-  ASSERT_GT(counter.value, 1);
 }
 
 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);
+}