Merge "sem_timedwait with a null timeout doesn't mean "forever"."

libc/bionic/pthread_cond.cpp

@@ -172,7 +172,7 @@ static int __pthread_cond_pulse(pthread_cond_internal_t* cond, int thread_count)
 
 static int __pthread_cond_timedwait(pthread_cond_internal_t* cond, pthread_mutex_t* mutex,
                                     bool use_realtime_clock, const timespec* abs_timeout_or_null) {
-  int result = check_timespec(abs_timeout_or_null);
+  int result = check_timespec(abs_timeout_or_null, true);
   if (result != 0) {
     return result;
   }

libc/bionic/pthread_mutex.cpp

@@ -304,7 +304,7 @@ static inline __always_inline int __pthread_normal_mutex_lock(pthread_mutex_inte
     if (__predict_true(__pthread_normal_mutex_trylock(mutex, shared) == 0)) {
         return 0;
     }
-    int result = check_timespec(abs_timeout_or_null);
+    int result = check_timespec(abs_timeout_or_null, true);
     if (result != 0) {
         return result;
     }
@@ -487,7 +487,7 @@ static int __pthread_mutex_lock_with_timeout(pthread_mutex_internal_t* mutex,
             old_state = new_state;
         }
 
-        int result = check_timespec(abs_timeout_or_null);
+        int result = check_timespec(abs_timeout_or_null, true);
         if (result != 0) {
             return result;
         }

libc/bionic/pthread_rwlock.cpp

@@ -298,7 +298,7 @@ static int __pthread_rwlock_timedrdlock(pthread_rwlock_internal_t* rwlock,
     if (result == 0 || result == EAGAIN) {
       return result;
     }
-    result = check_timespec(abs_timeout_or_null);
+    result = check_timespec(abs_timeout_or_null, true);
     if (result != 0) {
       return result;
     }
@@ -370,7 +370,7 @@ static int __pthread_rwlock_timedwrlock(pthread_rwlock_internal_t* rwlock,
     if (result == 0) {
       return result;
     }
-    result = check_timespec(abs_timeout_or_null);
+    result = check_timespec(abs_timeout_or_null, true);
     if (result != 0) {
       return result;
     }

libc/bionic/semaphore.cpp

@@ -235,7 +235,7 @@ int sem_timedwait(sem_t* sem, const timespec* abs_timeout) {
   }
 
   // Check it as per POSIX.
-  int result = check_timespec(abs_timeout);
+  int result = check_timespec(abs_timeout, false);
   if (result != 0) {
     errno = result;
     return -1;

libc/private/bionic_time_conversions.h

@@ -47,14 +47,17 @@ __LIBC_HIDDEN__ void absolute_timespec_from_timespec(timespec& abs_ts, const tim
 
 __END_DECLS
 
-static inline int check_timespec(const timespec* ts) {
-  if (ts != nullptr) {
-    if (ts->tv_nsec < 0 || ts->tv_nsec >= NS_PER_S) {
-      return EINVAL;
-    }
-    if (ts->tv_sec < 0) {
-      return ETIMEDOUT;
-    }
+static inline int check_timespec(const timespec* ts, bool null_allowed) {
+  if (null_allowed && ts == nullptr) {
+    return 0;
+  }
+  // glibc just segfaults if you pass a null timespec.
+  // That seems a lot more likely to catch bad code than returning EINVAL.
+  if (ts->tv_nsec < 0 || ts->tv_nsec >= NS_PER_S) {
+    return EINVAL;
+  }
+  if (ts->tv_sec < 0) {
+    return ETIMEDOUT;
   }
   return 0;
 }

tests/semaphore_test.cpp

@@ -131,6 +131,13 @@ TEST(semaphore, sem_timedwait) {
   ASSERT_EQ(0, sem_destroy(&s));
 }
 
+TEST(semaphore_DeathTest, sem_timedwait_null_timeout) {
+  sem_t s;
+  ASSERT_EQ(0, sem_init(&s, 0, 0));
+
+  ASSERT_EXIT(sem_timedwait(&s, nullptr), testing::KilledBySignal(SIGSEGV), "");
+}
+
 TEST(semaphore, sem_getvalue) {
   sem_t s;
   ASSERT_EQ(0, sem_init(&s, 0, 0));