commit 7cb3c4af134cfa4308742c86086bccdd06463c0c
author Elliott Hughes <enh@google.com> Thu Dec 17 01:15:15 2015 +0000
committer Gerrit Code Review <noreply-gerritcodereview@google.com> Thu Dec 17 01:15:15 2015 +0000
tree 310887025a62c2f03c7db8885c9dcb6920e7925f
parent d4f86aed42a97e1d7c502595b0fe1270f7989c7a
parent dd586f2ebd0c42904e699f3634568a38c97d4da7

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

libc/bionic/pthread_cond.cpp

diff --git a/libc/bionic/pthread_cond.cpp b/libc/bionic/pthread_cond.cpp
index adbce07..d36426c 100644
--- a/libc/bionic/pthread_cond.cpp
+++ b/libc/bionic/pthread_cond.cpp
@@ -172,7 +172,7 @@
 
 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

diff --git a/libc/bionic/pthread_mutex.cpp b/libc/bionic/pthread_mutex.cpp
index cad138c..7d8e8a8 100644
--- a/libc/bionic/pthread_mutex.cpp
+++ b/libc/bionic/pthread_mutex.cpp
@@ -304,7 +304,7 @@
     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 @@
             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

diff --git a/libc/bionic/pthread_rwlock.cpp b/libc/bionic/pthread_rwlock.cpp
index b1c48c8..a065295 100644
--- a/libc/bionic/pthread_rwlock.cpp
+++ b/libc/bionic/pthread_rwlock.cpp
@@ -298,7 +298,7 @@
     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 @@
     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

diff --git a/libc/bionic/semaphore.cpp b/libc/bionic/semaphore.cpp
index 79b5d63..b30c0b0 100644
--- a/libc/bionic/semaphore.cpp
+++ b/libc/bionic/semaphore.cpp
@@ -235,7 +235,7 @@
   }
 
   // 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

diff --git a/libc/private/bionic_time_conversions.h b/libc/private/bionic_time_conversions.h
index 294c29a..a834843 100644
--- a/libc/private/bionic_time_conversions.h
+++ b/libc/private/bionic_time_conversions.h
@@ -47,14 +47,17 @@
 
 __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

diff --git a/tests/semaphore_test.cpp b/tests/semaphore_test.cpp
index b65bfb8..84343da 100644
--- a/tests/semaphore_test.cpp
+++ b/tests/semaphore_test.cpp
@@ -131,6 +131,13 @@
   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));