[PATCH] libstdc++: Avoid overflow in timeout conversions [PR113327]
Tomasz Kaminski
tkaminsk@redhat.com
Tue Oct 14 09:57:18 GMT 2025
On Mon, Oct 13, 2025 at 11:00 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>
>
> 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);
>
Couldn't this still overflow? __ns may be really close to 1 second,
and thus larger than the difference
between chrono::duration_cast<chrono::seconds>(ms) - s.
We could stay on safe side, and remove one second from years of ms, i.e.
check:
if (__s < chrono::duration_cast<chrono::seconds>(ms) - chrono::seconds(1))
This wii guarantee that adding __ns will never overflow.
> }
>
> // 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/20251014/c53c71fd/attachment-0001.htm>
More information about the Libstdc++
mailing list