On 13/11/20 11:02 +0000, Jonathan Wakely wrote:
On 12/11/20 23:49 +0000, Jonathan Wakely wrote:
To poll a std::future to see if it's ready you have to call one of the
timed waiting functions. The most obvious way is wait_for(0s) but this
was previously very inefficient because it would turn the relative
timeout to an absolute one by calling system_clock::now(). When the
relative timeout is zero (or less) we're obviously going to get a time
that has already passed, but the overhead of obtaining the current time
can be dozens of microseconds. The alternative is to call wait_until
with an absolute timeout that is in the past. If you know the clock's
epoch is in the past you can use a default constructed time_point.
Alternatively, using some_clock::time_point::min() gives the earliest
time point supported by the clock, which should be safe to assume is in
the past. However, using a futex wait with an absolute timeout before
the UNIX epoch fails and sets errno=EINVAL. The new code using futex
waits with absolute timeouts was not checking for this case, which could
result in hangs (or killing the process if the libray is built with
assertions enabled).
This patch checks for times before the epoch before attempting to wait
on a futex with an absolute timeout, which fixes the hangs or crashes.
It also makes it very fast to poll using an absolute timeout before the
epoch (because we skip the futex syscall).
It also makes future::wait_for avoid waiting at all when the relative
timeout is zero or less, to avoid the unnecessary overhead of getting
the current time. This makes polling with wait_for(0s) take only a few
cycles instead of dozens of milliseconds.
libstdc++-v3/ChangeLog:
* include/std/future (future::wait_for): Do not wait for
durations less than or equal to zero.
* src/c++11/futex.cc (_M_futex_wait_until)
(_M_futex_wait_until_steady): Do not wait for timeouts before
the epoch.
* testsuite/30_threads/future/members/poll.cc: New test.
Tested powerpc64le-linux. Committed to trunk.
I think the shortcut in future::wait_for is worth backporting. The
changes in src/c++11/futex.cc are not needed because the code using
absolute timeouts with futex waits is not present on any release
branch.
I've committed this fix for the new test.
Backporting the change to gcc-10 revealed an overflow bug in the
existing code, resulting in blocking for years when given an absolute
timeout in the distant past. There's still a similar bug in the new
code (using futexes with absolute timeouts against clocks) where a
large chrono::seconds value can overflow and produce an incorrect
tv_sec value. Apart from the overflow itself being UB, the result in
that case is just a spurious wakeup (the call says it timed out when
it didn't reach the specified time). That should still be fixed, but
I'll do it separately.
Tested x86_64-linux. Committed to trunk.
commit e7e0eeeb6e6707be2a6c6da49d4b6be3199e2af8
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Fri Nov 13 15:19:04 2020
libstdc++: Avoid 32-bit time_t overflows in futex calls
The existing code doesn't check whether the chrono::seconds value is out
of range of time_t. When using a timeout before the epoch (with a
negative value) subtracting the current time (as time_t) and then
assigning it to a time_t can overflow to a large positive value. This
means that we end up waiting several years even though the specific
timeout was in the distant past.
We do have a check for negative timeouts, but that happens after the
conversion to time_t so happens after the overflow.
The conversion to a relative timeout is done in two places, so this
factors it into a new function and adds the overflow checks there.
libstdc++-v3/ChangeLog:
* src/c++11/futex.cc (relative_timespec): New function to
create relative time from two absolute times.
(__atomic_futex_unsigned_base::_M_futex_wait_until)
(__atomic_futex_unsigned_base::_M_futex_wait_until_steady):
Use relative_timespec.
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 57f7dfe87e9e..c2b2d32e8c43 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -31,6 +31,7 @@
#include <unistd.h>
#include <sys/time.h>
#include <errno.h>
+#include <ext/numeric_traits.h>
#include <debug/debug.h>
#ifdef _GLIBCXX_USE_CLOCK_GETTIME_SYSCALL
@@ -46,20 +47,55 @@ const unsigned futex_clock_realtime_flag = 256;
const unsigned futex_bitset_match_any = ~0;
const unsigned futex_wake_op = 1;
-namespace
-{
- std::atomic<bool> futex_clock_realtime_unavailable;
- std::atomic<bool> futex_clock_monotonic_unavailable;
-}
-
namespace std _GLIBCXX_VISIBILITY(default)
{
_GLIBCXX_BEGIN_NAMESPACE_VERSION
+namespace
+{
+ std::atomic<bool> futex_clock_realtime_unavailable;
+ std::atomic<bool> futex_clock_monotonic_unavailable;
+
+ // Return the relative duration from (now_s + now_ns) to (abs_s + abs_ns)
+ // as a timespec.
+ struct timespec
+ relative_timespec(chrono::seconds abs_s, chrono::nanoseconds abs_ns,
+ time_t now_s, long now_ns)
+ {
+ struct timespec rt;
+
+ // Did we already time out?
+ if (now_s > abs_s.count())
+ {
+ rt.tv_sec = -1;
+ return rt;
+ }
+
+ auto rel_s = abs_s.count() - now_s;
+
+ // Avoid overflows
+ if (rel_s > __gnu_cxx::__int_traits<time_t>::__max)
+ rel_s = __gnu_cxx::__int_traits<time_t>::__max;
+ else if (rel_s < __gnu_cxx::__int_traits<time_t>::__min)
+ rel_s = __gnu_cxx::__int_traits<time_t>::__min;
+
+ // Convert the absolute timeout value to a relative timeout
+ rt.tv_sec = rel_s;
+ rt.tv_nsec = abs_ns.count() - now_ns;
+ if (rt.tv_nsec < 0)
+ {
+ rt.tv_nsec += 1000000000;
+ --rt.tv_sec;
+ }
+
+ return rt;
+ }
+} // namespace
+
bool
- __atomic_futex_unsigned_base::_M_futex_wait_until(unsigned *__addr,
- unsigned __val,
- bool __has_timeout, chrono::seconds __s, chrono::nanoseconds __ns)
+ __atomic_futex_unsigned_base::
+ _M_futex_wait_until(unsigned *__addr, unsigned __val, bool __has_timeout,
+ chrono::seconds __s, chrono::nanoseconds __ns)
{
if (!__has_timeout)
{
@@ -109,15 +145,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// true or has just been set to true.
struct timeval tv;
gettimeofday (&tv, NULL);
+
// Convert the absolute timeout value to a relative timeout
- struct timespec rt;
- rt.tv_sec = __s.count() - tv.tv_sec;
- rt.tv_nsec = __ns.count() - tv.tv_usec * 1000;
- if (rt.tv_nsec < 0)
- {
- rt.tv_nsec += 1000000000;
- --rt.tv_sec;
- }
+ auto rt = relative_timespec(__s, __ns, tv.tv_sec, tv.tv_usec * 1000);
+
// Did we already time out?
if (rt.tv_sec < 0)
return false;
@@ -134,9 +165,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
bool
- __atomic_futex_unsigned_base::_M_futex_wait_until_steady(unsigned *__addr,
- unsigned __val,
- bool __has_timeout, chrono::seconds __s, chrono::nanoseconds __ns)
+ __atomic_futex_unsigned_base::
+ _M_futex_wait_until_steady(unsigned *__addr, unsigned __val,
+ bool __has_timeout,
+ chrono::seconds __s, chrono::nanoseconds __ns)
{
if (!__has_timeout)
{
@@ -188,15 +220,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
#else
clock_gettime(CLOCK_MONOTONIC, &ts);
#endif
+
// Convert the absolute timeout value to a relative timeout
- struct timespec rt;
- rt.tv_sec = __s.count() - ts.tv_sec;
- rt.tv_nsec = __ns.count() - ts.tv_nsec;
- if (rt.tv_nsec < 0)
- {
- rt.tv_nsec += 1000000000;
- --rt.tv_sec;
- }
+ auto rt = relative_timespec(__s, __ns, ts.tv_sec, ts.tv_nsec);
+
// Did we already time out?
if (rt.tv_sec < 0)
return false;
