From ac49cedc7e2bab2073a0895ef01e31dac84f590b Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Thu, 17 Aug 2017 13:18:52 -0700 Subject: [PATCH] Always use CLOCK_MONOTONIC for pthreads and semaphores pthread's and semaphore's default behavior is to use CLOCK_REALTIME, however this behavior is essentially never intended, as that clock is prone to change discontinuously. What users really intend is to use CLOCK_MONOTONIC, however only pthread_cond_timedwait() provides this as an option and even there, a large amount of existing code does not opt into CLOCK_MONOTONIC. We have seen numerous bugs directly attributable to this difference. Therefore, we provide this general workaround to always use CLOCK_MONOTONIC for waiting, regardless of what the input timespec is. Specifically this impacts the below APIs: pthread_mutex_timedlock() pthread_cond_timedwait() pthread_rwlock_timedrdlock() pthread_rwlock_timedwrlock() sem_timedwait() Test: boot bullhead, boot sailfish Test: bionic pthread/semaphore unit tests Test: check that pthread_cond_timedwait() timeouts are uneffected by CLOCK_REALTIME time changes Bug: 64694413 Bug: 64623895 Bug: 35756266 Bug: 35678943 Change-Id: Ibba98f5d88be1c306d14e9b9366302ecbef6d534 --- libc/Android.bp | 1 + libc/bionic/__cxa_guard.cpp | 2 +- libc/bionic/bionic_futex.cpp | 59 +++++++++++++++++++++++++ libc/bionic/bionic_time_conversions.cpp | 21 +++++++++ libc/private/bionic_futex.h | 16 +++---- libc/private/bionic_lock.h | 2 +- libc/private/bionic_time_conversions.h | 3 ++ 7 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 libc/bionic/bionic_futex.cpp diff --git a/libc/Android.bp b/libc/Android.bp index b0e0d8c8b..42d091456 100644 --- a/libc/Android.bp +++ b/libc/Android.bp @@ -1399,6 +1399,7 @@ cc_library_static { "bionic/assert.cpp", "bionic/atof.cpp", "bionic/bionic_arc4random.cpp", + "bionic/bionic_futex.cpp", "bionic/bionic_netlink.cpp", "bionic/bionic_systrace.cpp", "bionic/bionic_time_conversions.cpp", diff --git a/libc/bionic/__cxa_guard.cpp b/libc/bionic/__cxa_guard.cpp index 06926dff3..30b5f41bd 100644 --- a/libc/bionic/__cxa_guard.cpp +++ b/libc/bionic/__cxa_guard.cpp @@ -104,7 +104,7 @@ extern "C" int __cxa_guard_acquire(_guard_t* gv) { } } - __futex_wait_ex(&gv->state, false, CONSTRUCTION_UNDERWAY_WITH_WAITER, false, nullptr); + __futex_wait_ex(&gv->state, false, CONSTRUCTION_UNDERWAY_WITH_WAITER); old_value = atomic_load_explicit(&gv->state, memory_order_acquire); } } diff --git a/libc/bionic/bionic_futex.cpp b/libc/bionic/bionic_futex.cpp new file mode 100644 index 000000000..dd66e405e --- /dev/null +++ b/libc/bionic/bionic_futex.cpp @@ -0,0 +1,59 @@ +/* + * Copyright (C) 2017 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include "private/bionic_futex.h" + +#include + +#include "private/bionic_time_conversions.h" + +int __futex_wait_ex(volatile void* ftx, bool shared, int value, bool use_realtime_clock, + const timespec* abs_timeout) { + const timespec* futex_abs_timeout = abs_timeout; + // pthread's and semaphore's default behavior is to use CLOCK_REALTIME, however this behavior is + // essentially never intended, as that clock is prone to change discontinuously. + // + // What users really intend is to use CLOCK_MONOTONIC, however only pthread_cond_timedwait() + // provides this as an option and even there, a large amount of existing code does not opt into + // CLOCK_MONOTONIC. + // + // We have seen numerous bugs directly attributable to this difference. Therefore, we provide + // this general workaround to always use CLOCK_MONOTONIC for waiting, regardless of what the input + // timespec is. + timespec converted_monotonic_abs_timeout; + if (abs_timeout && use_realtime_clock) { + monotonic_time_from_realtime_time(converted_monotonic_abs_timeout, *abs_timeout); + if (converted_monotonic_abs_timeout.tv_sec < 0) { + return -ETIMEDOUT; + } + futex_abs_timeout = &converted_monotonic_abs_timeout; + } + + return __futex(ftx, (shared ? FUTEX_WAIT_BITSET : FUTEX_WAIT_BITSET_PRIVATE), value, + futex_abs_timeout, FUTEX_BITSET_MATCH_ANY); +} diff --git a/libc/bionic/bionic_time_conversions.cpp b/libc/bionic/bionic_time_conversions.cpp index ade3a553e..d21e12eb8 100644 --- a/libc/bionic/bionic_time_conversions.cpp +++ b/libc/bionic/bionic_time_conversions.cpp @@ -51,3 +51,24 @@ void timeval_from_timespec(timeval& tv, const timespec& ts) { tv.tv_sec = ts.tv_sec; tv.tv_usec = ts.tv_nsec / 1000; } + +void monotonic_time_from_realtime_time(timespec& monotonic_time, const timespec& realtime_time) { + monotonic_time = realtime_time; + + timespec cur_monotonic_time; + clock_gettime(CLOCK_MONOTONIC, &cur_monotonic_time); + timespec cur_realtime_time; + clock_gettime(CLOCK_REALTIME, &cur_realtime_time); + + monotonic_time.tv_nsec -= cur_realtime_time.tv_nsec; + monotonic_time.tv_nsec += cur_monotonic_time.tv_nsec; + if (monotonic_time.tv_nsec >= NS_PER_S) { + monotonic_time.tv_nsec -= NS_PER_S; + monotonic_time.tv_sec += 1; + } else if (monotonic_time.tv_nsec < 0) { + monotonic_time.tv_nsec += NS_PER_S; + monotonic_time.tv_sec -= 1; + } + monotonic_time.tv_sec -= cur_realtime_time.tv_sec; + monotonic_time.tv_sec += cur_monotonic_time.tv_sec; +} diff --git a/libc/private/bionic_futex.h b/libc/private/bionic_futex.h index 946d9dd1f..9b89131bd 100644 --- a/libc/private/bionic_futex.h +++ b/libc/private/bionic_futex.h @@ -36,13 +36,10 @@ #include #include -__BEGIN_DECLS - struct timespec; static inline __always_inline int __futex(volatile void* ftx, int op, int value, - const struct timespec* timeout, - int bitset) { + const timespec* timeout, int bitset) { // Our generated syscall assembler sets errno, but our callers (pthread functions) don't want to. int saved_errno = errno; int result = syscall(__NR_futex, ftx, op, value, timeout, NULL, bitset); @@ -61,17 +58,16 @@ static inline int __futex_wake_ex(volatile void* ftx, bool shared, int count) { return __futex(ftx, shared ? FUTEX_WAKE : FUTEX_WAKE_PRIVATE, count, NULL, 0); } -static inline int __futex_wait(volatile void* ftx, int value, const struct timespec* timeout) { +static inline int __futex_wait(volatile void* ftx, int value, const timespec* timeout) { return __futex(ftx, FUTEX_WAIT, value, timeout, 0); } -static inline int __futex_wait_ex(volatile void* ftx, bool shared, int value, - bool use_realtime_clock, const struct timespec* abs_timeout) { - return __futex(ftx, (shared ? FUTEX_WAIT_BITSET : FUTEX_WAIT_BITSET_PRIVATE) | - (use_realtime_clock ? FUTEX_CLOCK_REALTIME : 0), value, abs_timeout, +static inline int __futex_wait_ex(volatile void* ftx, bool shared, int value) { + return __futex(ftx, (shared ? FUTEX_WAIT_BITSET : FUTEX_WAIT_BITSET_PRIVATE), value, nullptr, FUTEX_BITSET_MATCH_ANY); } -__END_DECLS +__LIBC_HIDDEN__ int __futex_wait_ex(volatile void* ftx, bool shared, int value, + bool use_realtime_clock, const timespec* abs_timeout); #endif /* _BIONIC_FUTEX_H */ diff --git a/libc/private/bionic_lock.h b/libc/private/bionic_lock.h index 3dbafe07e..b38924716 100644 --- a/libc/private/bionic_lock.h +++ b/libc/private/bionic_lock.h @@ -64,7 +64,7 @@ class Lock { } while (atomic_exchange_explicit(&state, LockedWithWaiter, memory_order_acquire) != Unlocked) { // TODO: As the critical section is brief, it is a better choice to spin a few times befor sleeping. - __futex_wait_ex(&state, process_shared, LockedWithWaiter, false, nullptr); + __futex_wait_ex(&state, process_shared, LockedWithWaiter); } return; } diff --git a/libc/private/bionic_time_conversions.h b/libc/private/bionic_time_conversions.h index b9eaad2e7..fb049f238 100644 --- a/libc/private/bionic_time_conversions.h +++ b/libc/private/bionic_time_conversions.h @@ -42,6 +42,9 @@ __LIBC_HIDDEN__ void timespec_from_ms(timespec& ts, const int ms); __LIBC_HIDDEN__ void timeval_from_timespec(timeval& tv, const timespec& ts); +__LIBC_HIDDEN__ void monotonic_time_from_realtime_time(timespec& monotonic_time, + const timespec& realtime_time); + __END_DECLS static inline int check_timespec(const timespec* ts, bool null_allowed) {