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