[PATCH] libstdc++: Avoid overflow in timeout conversions [PR113327]
Tomasz Kaminski
tkaminsk@redhat.com
Tue Oct 14 10:24:20 GMT 2025
On Tue, Oct 14, 2025 at 11:57 AM Tomasz Kaminski <tkaminsk@redhat.com>
wrote:
>
>
> 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.
>
LGTM with above change.
> }
>>
>> // 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/fe49cfef/attachment-0001.htm>
More information about the Libstdc++
mailing list