On 13/11/20 20:29 +0000, Mike Crowe via Libstdc++ wrote:
On Friday 13 November 2020 at 17:25:22 +0000, Jonathan Wakely wrote:
+ // 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;
I may be missing something, but if the line above executes...
+
+ // 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;
...and so does this line above, then I think that we'll end up
underflowing. (Presumably rt.tv_sec will wrap round to being some time in
2038 on most 32-bit targets.)
Ugh.
I'm currently trying to persuade myself that this can actually happen and
if so work out how to come up with a test case for it.
Maybe something like:
auto d = chrono::floor<chrono::seconds>(system_clock::now().time_since_epoch()
- seconds(INT_MAX + 2LL));
fut.wait_until(system_clock::time_point(d));
This will create a sys_time with a value that is slightly more than
INT_MAX seconds before the current time, with a zero nanoseconds
component. The difference between the gettimeofday result and this
time will be slightly more negative than INT_MIN and so will overflow
a 32-bit time_t, causing the code to use __int_traits<time_t>::__min.
As long as the gettimeofday call doesn't happen to also have a zero
nanoseconds component, the difference of the nanoseconds values will
be negative, and we will decrement the time_t value.
I don't believe that this part changed in your later patch.
Right. Thanks for catching this.
The attached patch should fix it. There's no point clamping very
negative values to the minimum time_t value, since any relative
timeout less than zero has already passed. So we can just use -1
there (and not bother with the tv_nsec field at all):
else if (rel_s <= 0) [[unlikely]]
{
rt.tv_sec = -1;
}
But please check my working.
Oh how I wish all these functions didn't expect already cracked seconds and
nanoseconds. :(
Indeed.
At some point (probably not for GCC 11) I'm going to add a proper
wrapper class for all these futex calls, and make it robust, and then
replace the various hand-crafted uses of syscall(SYS_futex, ...) with
that type so we only need to fix all this in one place. And it will
just take a duration or time_point.
commit a77f131249202c7342cc385f2d0473ee032ae0eb
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Fri Nov 13 20:57:15 2020
libstdc++: Fix another 32-bit time_t overflow in futex timeouts
libstdc++-v3/ChangeLog:
* src/c++11/futex.cc (relative_timespec): Avoid overflow if the
difference of seconds is the minimum time_t value and the
difference of nanoseconds is also negative.
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index 15959cebee57..50768403cc63 100644
--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -75,19 +75,25 @@ namespace
auto rel_s = abs_s.count() - now_s;
- // Avoid overflows
+ // Convert the absolute timeout to a relative timeout, without overflow.
if (rel_s > __int_traits<time_t>::__max) [[unlikely]]
- rel_s = __int_traits<time_t>::__max;
- else if (rel_s < __int_traits<time_t>::__min) [[unlikely]]
- rel_s = __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;
+ rt.tv_sec = __int_traits<time_t>::__max;
+ rt.tv_nsec = 999999999;
+ }
+ else if (rel_s <= 0) [[unlikely]]
+ {
+ rt.tv_sec = -1;
+ }
+ else
+ {
+ 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;
