make futexWaitUntil tolerant of invalid time_point-s

Summary:
futexWaitUntil could generate an invalid timespec when presented
with a time_point from before the epoch, which resulted in an EINVAL from
the futex syscall.  Debug builds crashed on this unexpected return value,
while release builds soldiered on.  This path happened to be exercised
by the unit test.  This diff fixes the unintentional deadline overflow
in the test, adds explicit testing of overflow behavior, and makes
futexWaitUntil handle invalid time_points in a sensible manner.

Test Plan:
1. new unit tests
2. fbmake runtests_dbg

Reviewed By: mssarang@fb.com

Subscribers: strager, njormrod, folly-diffs@

FB internal diff: D1747972

Tasks: 5853949

Signature: t1:1747972:1419382193:862c193a13428d96acb33c85f962f59203661f40

diff --git a/folly/detail/Futex.cpp b/folly/detail/Futex.cpp
index d18614491162b0e2bdff9751d6ce2d5c400f49bc..89d99722d9cb8ae1025a67aef71ae6f8778b7531 100644 (file)
--- a/folly/detail/Futex.cpp
+++ b/folly/detail/Futex.cpp
@@ -59,6 +59,10 @@ struct timespec
 timeSpecFromTimePoint(time_point<Clock> absTime)
 {
   auto duration = absTime.time_since_epoch();
+  if (duration.count() < 0) {
+    // kernel timespec_valid requires non-negative seconds and nanos in [0,1G)
+    duration = Clock::duration::zero();
+  }
   auto secs = duration_cast<seconds>(duration);
   auto nanos = duration_cast<nanoseconds>(duration - secs);
   struct timespec result = { secs.count(), nanos.count() };
@@ -108,8 +112,11 @@ FutexResult nativeFutexWaitImpl(void* addr,
         return FutexResult::VALUE_CHANGED;
       default:
         assert(false);
-        // EACCESS, EFAULT, or EINVAL. All of these mean *addr point to
-        // invalid memory (or I misunderstand the API).  We can either
+        // EINVAL, EACCESS, or EFAULT.  EINVAL means there was an invalid
+        // op (should be impossible) or an invalid timeout (should have
+        // been sanitized by timeSpecFromTimePoint).  EACCESS or EFAULT
+        // means *addr points to invalid memory, which is unlikely because
+        // the caller should have segfaulted already.  We can either
         // crash, or return a value that lets the process continue for
         // a bit. We choose the latter. VALUE_CHANGED probably turns the
         // caller into a spin lock.

diff --git a/folly/test/FutexTest.cpp b/folly/test/FutexTest.cpp
index 71392bfef1396a56c56ac7a8e4856674d5c5e984..477a069bf185c382d2ddd8d852492721e37607a0 100644 (file)
--- a/folly/test/FutexTest.cpp
+++ b/folly/test/FutexTest.cpp
@@ -64,9 +64,9 @@ void liveClockWaitUntilTests() {
     auto fp = &f; // workaround for t5336595
     auto thrA = DSched::thread([fp,stress]{
       while (true) {
-        auto deadline = time_point_cast<Duration>(
+        const auto deadline = time_point_cast<Duration>(
             Clock::now() + microseconds(1 << (stress % 20)));
-        auto res = fp->futexWaitUntil(0, deadline);
+        const auto res = fp->futexWaitUntil(0, deadline);
         EXPECT_TRUE(res == FutexResult::TIMEDOUT || res == FutexResult::AWOKEN);
         if (res == FutexResult::AWOKEN) {
           break;
@@ -81,12 +81,26 @@ void liveClockWaitUntilTests() {
     DSched::join(thrA);
   }
 
-  auto start = Clock::now();
-  EXPECT_EQ(f.futexWaitUntil(0, start + milliseconds(100)),
-            FutexResult::TIMEDOUT);
-  LOG(INFO) << "Futex wait timed out after waiting for "
-            << duration_cast<milliseconds>(Clock::now() - start).count()
-            << "ms, should be ~100ms";
+  {
+    const auto start = Clock::now();
+    const auto deadline = time_point_cast<Duration>(start + milliseconds(100));
+    EXPECT_EQ(f.futexWaitUntil(0, deadline), FutexResult::TIMEDOUT);
+    LOG(INFO) << "Futex wait timed out after waiting for "
+              << duration_cast<milliseconds>(Clock::now() - start).count()
+              << "ms using clock with " << Duration::period::den
+              << " precision, should be ~100ms";
+  }
+
+  {
+    const auto start = Clock::now();
+    const auto deadline = time_point_cast<Duration>(
+        start - 2 * start.time_since_epoch());
+    EXPECT_EQ(f.futexWaitUntil(0, deadline), FutexResult::TIMEDOUT);
+    LOG(INFO) << "Futex wait with invalid deadline timed out after waiting for "
+              << duration_cast<milliseconds>(Clock::now() - start).count()
+              << "ms using clock with " << Duration::period::den
+              << " precision, should be ~0ms";
+  }
 }
 
 template <typename Clock>
@@ -95,7 +109,7 @@ void deterministicAtomicWaitUntilTests() {
 
   // Futex wait must eventually fail with either FutexResult::TIMEDOUT or
   // FutexResult::INTERRUPTED
-  auto res = f.futexWaitUntil(0, Clock::now() + milliseconds(100));
+  const auto res = f.futexWaitUntil(0, Clock::now() + milliseconds(100));
   EXPECT_TRUE(res == FutexResult::TIMEDOUT || res == FutexResult::INTERRUPTED);
 }
 
@@ -104,8 +118,8 @@ void run_wait_until_tests() {
   liveClockWaitUntilTests<Atom, system_clock, system_clock::duration>();
   liveClockWaitUntilTests<Atom, steady_clock, steady_clock::duration>();
 
-  typedef duration<int64_t, pico> picoseconds;
-  liveClockWaitUntilTests<Atom, system_clock, picoseconds>();
+  typedef duration<int64_t, std::ratio<1, 10000000>> decimicroseconds;
+  liveClockWaitUntilTests<Atom, system_clock, decimicroseconds>();
 }
 
 template <>
@@ -124,7 +138,7 @@ void run_system_clock_test() {
   struct timespec ts;
   const int maxIters = 1000;
   int iter = 0;
-  uint64_t delta = 10000000 /* 10 ms */;
+  const uint64_t delta = 10000000 /* 10 ms */;
 
   /** The following loop is only to make the test more robust in the presence of
    * clock adjustments that can occur. We just run the loop maxIter times and
@@ -156,15 +170,15 @@ void run_steady_clock_test() {
    * for the time_points */
   EXPECT_TRUE(steady_clock::is_steady);
 
-  uint64_t A = duration_cast<nanoseconds>(steady_clock::now()
-                                          .time_since_epoch()).count();
+  const uint64_t A = duration_cast<nanoseconds>(steady_clock::now()
+                                                .time_since_epoch()).count();
 
   struct timespec ts;
   clock_gettime(CLOCK_MONOTONIC, &ts);
-  uint64_t B = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+  const uint64_t B = ts.tv_sec * 1000000000ULL + ts.tv_nsec;
 
-  uint64_t C = duration_cast<nanoseconds>(steady_clock::now()
-                                          .time_since_epoch()).count();
+  const uint64_t C = duration_cast<nanoseconds>(steady_clock::now()
+                                                .time_since_epoch()).count();
   EXPECT_TRUE(A <= B && B <= C);
 }