[PATCH] libstdc++: Avoid overflow in timeout conversions [PR113327]
Jonathan Wakely
jwakely@redhat.com
Tue Oct 14 10:49:14 GMT 2025
On Tue, 14 Oct 2025 at 11:44 +0100, Jonathan Wakely wrote:
>On Tue, 14 Oct 2025 at 11:57 +0200, Tomasz Kaminski 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.
>
>I don't think it can.
>
>milliseconds::max() is 9223372036854775807ms
>
>duration_cast<seconds>(milliseconds::max()) is 9223372036854775s
>
>We know that the caller ensures that __ns <= 999'999'999ns, so
>chrono::__detail::ceil<chrono::milliseconds>(__ns) <= 1000ms
>
>The check is that __s is less than that (not less than or equal), so
>we know that __s is at most 9223372036854774s. We convert that to
>milliseconds and add 1000ms. That gives 9223372036854775s, which is
>the same value as duration_cast<seconds>(milliseconds::max()) and is
>807ms less than milliseconds::max().
>
There is a "bug" where __s == 9223372036854775s && __ns < 807ms,
because the logic above will use ms = milliseconds::max() which is too
large by (807ms - __ns). So we will wait for up to 807ms too long.
But I think waking up 807ms late after sleeping for 292 millions years
is probably acceptable ;-)
>
>>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
>>>>> }
>>>>>
>>>>>
More information about the Libstdc++
mailing list