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
This commit is contained in:
parent
30438e4cea
commit
ac49cedc7e
7 changed files with 92 additions and 12 deletions
|
@ -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",
|
||||
|
|
|
@ -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);
|
||||
}
|
||||
}
|
||||
|
|
59
libc/bionic/bionic_futex.cpp
Normal file
59
libc/bionic/bionic_futex.cpp
Normal file
|
@ -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 <time.h>
|
||||
|
||||
#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);
|
||||
}
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -36,13 +36,10 @@
|
|||
#include <sys/syscall.h>
|
||||
#include <unistd.h>
|
||||
|
||||
__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 */
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
|
|
|
@ -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) {
|
||||
|
|
Loading…
Reference in a new issue