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

It actually means "crash immediately". Well, it's an error. And callers are
much more likely to realize their mistake if we crash immediately rather
than return EINVAL. Historically, glibc has crashed and bionic -- before
the recent changes -- returned EINVAL, so this is a behavior change.

Change-Id: I0c2373a6703b20b8a97aacc1e66368a5885e8c51

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));