From dd586f2ebd0c42904e699f3634568a38c97d4da7 Mon Sep 17 00:00:00 2001
From: Elliott Hughes <enh@google.com>
Date: Wed, 16 Dec 2015 15:15:58 -0800
Subject: [PATCH] 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           |  2 +-
 libc/bionic/pthread_mutex.cpp          |  4 ++--
 libc/bionic/pthread_rwlock.cpp         |  4 ++--
 libc/bionic/semaphore.cpp              |  2 +-
 libc/private/bionic_time_conversions.h | 19 +++++++++++--------
 tests/semaphore_test.cpp               |  7 +++++++
 6 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/libc/bionic/pthread_cond.cpp b/libc/bionic/pthread_cond.cpp
index adbce07f1..d36426c16 100644
--- a/libc/bionic/pthread_cond.cpp
+++ b/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;
   }
diff --git a/libc/bionic/pthread_mutex.cpp b/libc/bionic/pthread_mutex.cpp
index cad138ca3..7d8e8a820 100644
--- a/libc/bionic/pthread_mutex.cpp
+++ b/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;
         }
diff --git a/libc/bionic/pthread_rwlock.cpp b/libc/bionic/pthread_rwlock.cpp
index b1c48c8f2..a0652953d 100644
--- a/libc/bionic/pthread_rwlock.cpp
+++ b/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;
     }
diff --git a/libc/bionic/semaphore.cpp b/libc/bionic/semaphore.cpp
index 79b5d6372..b30c0b06e 100644
--- a/libc/bionic/semaphore.cpp
+++ b/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;
diff --git a/libc/private/bionic_time_conversions.h b/libc/private/bionic_time_conversions.h
index 294c29a79..a83484370 100644
--- a/libc/private/bionic_time_conversions.h
+++ b/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;
 }
diff --git a/tests/semaphore_test.cpp b/tests/semaphore_test.cpp
index b65bfb859..84343dadd 100644
--- a/tests/semaphore_test.cpp
+++ b/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));