[PATCH] libstdc++: Avoid overflow in timeout conversions [PR113327]
Jonathan Wakely
jwakely@redhat.com
Mon Oct 13 20:59:58 GMT 2025
On Fri, 10 Oct 2025 at 09:19, Tomasz Kaminski <tkaminsk@redhat.com> wrote:
>
>
> On Thu, Oct 9, 2025 at 4:26 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>
>> When converting from a coarse duration with a very large value, the
>> existing code scales that up to chrono::seconds which overflows the
>> chrono::seconds::rep type. For example, sleep_for(chrono::hours::max())
>> tries to calculate LLONG_MAX * 3600, which overflows to -3600 and so the
>> sleep returns immediately.
>>
>> The solution in this commit is inspired by this_thread::sleep_for in
>> libc++ which compares the duration argument to
>> chrono::duration<long double>(nanoseconds::max()) and limits the
>> duration to nanoseconds::max(). Because we split the duration into
>> seconds and nanoseconds, we can use seconds::max() as our upper limit.
>>
>> We might need to limit further if seconds::max() doesn't fit in the
>> type used for sleeping, which is one of std::time_t, unsigned int, or
>> chrono::milliseconds.
>>
>> To fix this everywhere that uses timeouts, new functions are introduced
>> for converting from a chrono::duration or chrono::time_point to a
>> timespec (or __gthread_time_t which is just a timespec on Linux). These
>> functions provide one central place where we can avoid overflow and also
>> handle negative timeouts (as these produce errors when passed to OS
>> functions that do not accept absolute times before the epoch). All
>> negative durations are converted to zero, and negative time_points are
>> converted to the epoch.
>
>
>> libstdc++-v3/ChangeLog:
>>
>> PR libstdc++/113327
>> PR libstdc++/116586
>> PR libstdc++/119258
>> PR libstdc++/58931
>> * include/bits/chrono.h (__to_timeout_timespec): New overloaded
>> function templates for converting chrono types to timespec.
>> (__to_timeout_gthread_time_t): New function template for
>> converting time_point to __gthread_time_t.
>> * include/bits/this_thread_sleep.h (sleep_for): Use
>> __to_timeout_timespec.
>> (__sleep_for): Remove namespace-scope declaration.
>> * include/std/condition_variable: Likewise.
>> * include/std/mutex: Likewise.
>> * include/std/shared_mutex: Likewise.
>> * src/c++11/thread.cc (limit): New helper function.
>> (__sleep_for): Use limit to prevent overflow when converting
>> chrono::seconds to time_t, unsigned, or chrono::milliseconds.
>> * src/c++20/atomic.cc: Use __to_timeout_timespec and
>> __to_timeout_gthread_time_t for timeouts.
>> * testsuite/30_threads/this_thread/113327.cc: New test.
>>
>> Reviewed-by: Mike Crowe <mac@mcrowe.com>
>> ---
>>
>> v2: followed Mike's suggestion to rename the functions from
>> __to_timespec and __to_gthread_time_t to __to_timeout_timespec and
>> __to_timeout_gthread_time_t.
>>
> Some comments below, I think I found an issue in __sleep_for
> implementation.
>
>>
>> Tested x86_64-linux.
>>
>> libstdc++-v3/include/bits/chrono.h | 95 +++++++++++++++++++
>> libstdc++-v3/include/bits/this_thread_sleep.h | 20 ++--
>> libstdc++-v3/include/std/condition_variable | 20 +---
>> libstdc++-v3/include/std/mutex | 18 +---
>> libstdc++-v3/include/std/shared_mutex | 39 +-------
>> libstdc++-v3/src/c++11/thread.cc | 32 ++++++-
>> libstdc++-v3/src/c++20/atomic.cc | 18 +---
>> .../30_threads/this_thread/113327.cc | 29 ++++++
>> 8 files changed, 172 insertions(+), 99 deletions(-)
>> create mode 100644
>> libstdc++-v3/testsuite/30_threads/this_thread/113327.cc
>>
>> diff --git a/libstdc++-v3/include/bits/chrono.h
>> b/libstdc++-v3/include/bits/chrono.h
>> index 8de8e756c714..c20b68140192 100644
>> --- a/libstdc++-v3/include/bits/chrono.h
>> +++ b/libstdc++-v3/include/bits/chrono.h
>> @@ -50,6 +50,9 @@
>>
>> #include <bits/version.h>
>>
>> +// TODO move __to_gthread_time_t to a better place
>> +#include <bits/gthr.h> // for __gthread_time_t
>> +
>> namespace std _GLIBCXX_VISIBILITY(default)
>> {
>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> @@ -1515,6 +1518,98 @@ _GLIBCXX_END_INLINE_ABI_NAMESPACE(_V2)
>> } // namespace filesystem
>> #endif // C++17 && HOSTED
>>
>> +#if defined _GLIBCXX_USE_NANOSLEEP || defined
>> _GLIBCXX_USE_CLOCK_REALTIME \
>> + || defined _GLIBCXX_HAS_GTHREADS
>> +#pragma GCC diagnostic push
>> +#pragma GCC diagnostic ignored "-Wc++17-extensions"
>> +namespace chrono
>> +{
>> +/// @cond undocumented
>> +
>> + // Convert a chrono::duration to a relative time represented as
>> timespec
>> + // (e.g. for use with nanosleep).
>> + template<typename _Rep, typename _Period>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + struct ::timespec
>> + __to_timeout_timespec(const duration<_Rep, _Period>& __d)
>> + {
>> + struct ::timespec __ts{};
>> +
>> + if (__d < __d.zero()) // Negative timeouts don't make sense.
>> + return __ts;
>> +
>> + if constexpr (__or_<ratio_greater<_Period, ratio<1>>,
>> + treat_as_floating_point<_Rep>>::value)
>
> I think simple || here would be fine, they treate_as_floting_point does not
> seem to be that expensive to instantiate, to outweight instantiation __or.
>
>>
>
> + {
>> + // Converting from e.g. chrono::hours::max() to chrono::seconds
>> + // would evaluate LLONG_MAX * 3600 which would overflow.
>> + // Limit to chrono::seconds::max().
>> + chrono::duration<double> __fmax(chrono::seconds::max());
>> + if (__d > __fmax) [[__unlikely__]]
>> + return chrono::__to_timeout_timespec(chrono::seconds::max());
>> + }
>> +
>> + auto __s = chrono::duration_cast<chrono::seconds>(__d);
>> +
>> + if constexpr (is_integral<time_t>::value) // POSIX.1-2001 allows
>> floating
>> + {
>> + // Also limit to time_t maximum (only relevant for 32-bit
>> time_t).
>> + constexpr auto __tmax = numeric_limits<time_t>::max();
>> + if (__s.count() > __tmax) [[__unlikely__]]
>> + {
>> + __ts.tv_sec = __tmax;
>> + return __ts;
>> + }
>> + }
>> +
>> + auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
>> +
>> + if constexpr (treat_as_floating_point<_Rep>::value)
>> + if (__ns.count() > 999999999) [[__unlikely__]]
>> + __ns = chrono::nanoseconds(999999999);
>>
> This seemed strange at first, but I guess this is the result of using
> floating point arithmetic.
>
Yes, it might not be necessary, but I'd rather just have this conditional
branch (which is discarded for non-floating-point reps) than get EINVAL
from some pthreads API for using ts.tv_nsec == 1'000'000'000.
> +
>> + __ts.tv_sec = static_cast<time_t>(__s.count());
>> + __ts.tv_nsec = static_cast<long>(__ns.count());
>> + return __ts;
>> + }
>> +
>> + // Convert a chrono::time_point to an absolute time represented as
>> timespec.
>> + // All times before the epoch get converted to the epoch, so this
>> assumes
>> + // that we only use it for clocks where that's true.
>> + // It should be safe to use this for system_clock and steady_clock.
>
> + template<typename _Clock, typename _Dur>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + struct ::timespec
>> + __to_timeout_timespec(const time_point<_Clock, _Dur>& __t)
>> + {
>> + return chrono::__to_timeout_timespec(__t.time_since_epoch());
>> + }
>> +
>> +#ifdef _GLIBCXX_HAS_GTHREADS
>> + // Convert a time_point to an absolute time represented as
>> __gthread_time_t
>> + // (which is typically just a typedef for struct timespec).
>> + template<typename _Clock, typename _Dur>
>> + [[__nodiscard__]] _GLIBCXX14_CONSTEXPR inline
>> + __gthread_time_t
>> + __to_timeout_gthread_time_t(const time_point<_Clock, _Dur>& __t)
>> + {
>> + auto __ts = chrono::__to_timeout_timespec(__t.time_since_epoch());
>> + if constexpr (is_same<::timespec, __gthread_time_t>::value)
>> + return __ts;
>> + else if constexpr (is_convertible<::timespec,
>> __gthread_time_t>::value)
>> + return __ts;
>> + else if constexpr (is_scalar<__gthread_time_t>::value)
>>
> Could you add some comment here, tha __gthread_time_it is count of second
> in scalar, in contrast to being milliseconds for example.
>
Done.
> + return static_cast<__gthread_time_t>(__ts.tv_sec);
>> + else // Assume this works:
>> + return __gthread_time_t{ __ts.tv_sec, __ts.tv_nsec };
>> + }
>> +#endif // HAS_GTHREADS
>> +
>> +/// @endcond
>> +} // namespace chrono
>> +#pragma GCC diagnostic pop
>> +#endif // USE_NANOSLEEP || USE_CLOCK_REALTIME || HAS_GTHREADS
>> +
>> _GLIBCXX_END_NAMESPACE_VERSION
>> } // namespace std
>>
>>
[...]
> diff --git a/libstdc++-v3/src/c++11/thread.cc
>> b/libstdc++-v3/src/c++11/thread.cc
>> index 6c2ec2978f88..0768a99d6741 100644
>> --- a/libstdc++-v3/src/c++11/thread.cc
>> +++ b/libstdc++-v3/src/c++11/thread.cc
>> @@ -231,10 +231,30 @@ namespace std _GLIBCXX_VISIBILITY(default)
>> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> namespace this_thread
>> {
>> +namespace
>> +{
>> + // returns min(s, Dur::max())
>> + template<typename Dur>
>> + inline chrono::seconds
>> + limit(chrono::seconds s)
>> + {
>> + static_assert(ratio_equal<typename Dur::period, ratio<1>>::value,
>> + "period must be seconds to avoid potential overflow");
>> +
>> + if (s > Dur::max()) [[__unlikely__]]
>> + s = chrono::duration_cast<chrono::seconds>(Dur::max());
>> + return s;
>> + }
>> +}
>> +
>> void
>> __sleep_for(chrono::seconds __s, chrono::nanoseconds __ns)
>> {
>> #ifdef _GLIBCXX_USE_NANOSLEEP
>> +#pragma GCC diagnostic ignored "-Wc++17-extensions"
>> + if constexpr (is_integral<time_t>::value) // POSIX.1-2001 allows
>> floating
>> + __s = limit<chrono::duration<time_t>>(__s);
>> +
>> struct ::timespec __ts =
>> {
>> static_cast<std::time_t>(__s.count()),
>> @@ -246,6 +266,8 @@ namespace this_thread
>> const auto target = chrono::steady_clock::now() + __s + __ns;
>> while (true)
>> {
>> + __s = limit<chrono::duration<unsigned>>(__s);
>> +
>> unsigned secs = __s.count();
>> if (__ns.count() > 0)
>> {
>> @@ -271,11 +293,19 @@ namespace this_thread
>> break;
>> __s = chrono::duration_cast<chrono::seconds>(target - now);
>> __ns = chrono::duration_cast<chrono::nanoseconds>(target - (now +
>> __s));
>> - }
>> + }
>> #elif defined(_GLIBCXX_USE_WIN32_SLEEP)
>> + // Can't use limit<chrono::milliseconds>(__s) here because it would
>> + // multiply __s by 1000 which could overflow.
>> + auto max_ms = chrono::milliseconds::max() / 1000;
>>
> This does not seem right to me. millseconds::max() returns milliseconds
> (duration object)
> and not rep. Then we divide it by 1000 to get the number of seconds, but
> we still have milliseconds.
>
Yes, this code is nonsense.
> + auto max_ms_in_s = chrono::duration_cast<chrono::seconds>(max_ms);
> So the conversions to seconds, will divide it by 1000 again, should this
> be:
> auto max_ms_in_s
> =
> chrono::duration_cast<chrono::seconds>(std::chrono::milliseconds::max);
> Or at least chrono::duration_cast<chrono::seconds>(max_ms.count())
> I prefer the former.
>
OK, how's this:
#elif defined(_GLIBCXX_USE_WIN32_SLEEP)
// Can't use limit<chrono::milliseconds>(__s) here because it would
// multiply __s by 1000 which could overflow.
// Limit to milliseconds::max() and truncate to seconds:
chrono::milliseconds ms = chrono::milliseconds::max();
if (__s < chrono::duration_cast<chrono::seconds>(ms))
{
ms = __s;
ms += chrono::__detail::ceil<chrono::milliseconds>(__ns);
}
// Use Sleep(DWORD millis) where DWORD is uint32_t.
constexpr chrono::milliseconds max_sleep(INFINITE - 1u);
while (ms > max_sleep)
{
::Sleep(max_sleep.count());
ms -= max_sleep;
}
::Sleep(ms.count());
#endif
This fixes the duplicated division by 1000, and adds a loop to sleep longer
than INFINITE milliseconds (which is approx. 49 days).
We could use duration<uint64_t, milli> instead of milliseconds, which would
double the upper limit by using an unsigned type, but milliseconds::max()
is already huge.
+ if (__s > max_ms_in_s)
>> + __s = max_ms_in_s;
>> +
>> unsigned long ms = __ns.count() / 1000000;
>> if (__ns.count() > 0 && ms == 0)
>> ms = 1;
>> +
>> ::Sleep(chrono::milliseconds(__s).count() + ms);
>> #endif
>> }
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20251013/9029a02d/attachment-0001.htm>
More information about the Libstdc++
mailing list