Bug 116586 - All uses of absolute timeouts should correctly handle negative times
Summary: All uses of absolute timeouts should correctly handle negative times
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 15.0
: P3 normal
Target Milestone: 16.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 121141 (view as bug list)
Depends on: 118093
Blocks:
  Show dependency treegraph
 
Reported: 2024-09-03 15:13 UTC by Jonathan Wakely
Modified: 2025-10-14 20:51 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2025-01-15 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2024-09-03 15:13:09 UTC
POSIX doesn't require any APIs to handle times before the epoch, so any time we have a chrono::time_point that gets converted to a timespec and passed to something like pthread_cond_timedwait or sem_timedwait we should check for negative time points.

It seems reasonable to assume the system clock is not set to a time before the epoch, so we can just treat any negative time_point as being in the past and so timeout immediately.

For timeouts that use the steady_clock I think we can also assume the clock's unspecified epoch is in the past, i.e. that steady_clock::now() is always a non-negative time point. That's true for linux. POSIX says the clock gives you the time since some unspecified point in the past, which I hope means "since an unspecified epoch, which is guaranteed to be in the past".

We have a patch for std::future timeouts:
https://patchwork.sourceware.org/project/gcc/patch/20240724131238.271095-1-william.tsai@sifive.com/

condition_variable and timed mutexes and __platform_semaphore need a fix.
Comment 1 Jonathan Wakely 2025-01-15 14:04:35 UTC
r15-6690-g8ade3c3ea77e16 fixed the std::future case.
Comment 2 Jonathan Wakely 2025-07-17 10:02:28 UTC
*** Bug 121141 has been marked as a duplicate of this bug. ***
Comment 3 Mike Crowe 2025-07-18 20:34:45 UTC
The particularly nasty thing about this for shared_timed_mutex is that if _GLIBCXX_ASSERTIONS is not defined then std::shared_timed_mutex::try_lock_until() returns true even though it hasn't successfully taken the lock! That feels like a particularly nasty failure mode for such a bug. :(

IMO returning success when we know that _something_ went wrong isn't really a great idea. Since we're checking the return value anyway it feels like returning false for unrecognised errors would be safer and barely any more expensive. It looks like I just blindly copied the _timedlock code in ab40695a46c664 to exacerbate this problem. :(

I'd be interested in views on whether std::shared_timed_mutex::try_lock_until() should return false on all unrecognised errors if the assert allows it to continue. Perhaps something like:

--8<--
         int __ret = pthread_rwlock_clockwrlock(&_M_rwlock, CLOCK_MONOTONIC,                                                                                  
                                                &__ts);                                                                                                       
         if (__ret == 0)  /// ***NEW***                                                                                                                                    
           return true;   /// ***NEW***                                                                                                                                    
                                                                                                                                                              
         // On self-deadlock, we just fail to acquire the lock.  Technically,                                                                                 
         // the program violated the precondition.                                                                                                            
         if (__ret == ETIMEDOUT || __ret == EDEADLK)                                                                                                          
           return false;                                                                                                                                      
         // Errors not handled: EINVAL                                                                                                                        
         __glibcxx_assert(__ret == 0);                                                                                                                        
         return false;  /// ***INVERTED***                                                                                                                                       
-->8--

As would be expected, this appears to generate ever-so-slightly-less-efficient code with -O2 and _GLIBCXX_ASSERTIONS[1], but more efficient code without _GLIBCXX_ASSERTIONS[2] because the compiler can throw away the checks against the exact value of __ret.

In the meantime I'll write test cases for at least the other functions I added _clock* versions of and ensure that they cope with negative timeouts.

[1] https://godbolt.org/z/b3dKGWeW6
[2] https://godbolt.org/z/a59G7nE7j
Comment 4 Mike Crowe 2025-10-05 14:12:00 UTC
> condition_variable and timed mutexes and __platform_semaphore need a fix.

condition_variable no longer appears to suffer from this problem.

__platform_semaphore (as used by std::counting_semaphore and std::binary_semaphore) still does.

shared_timed_mutex (bug 121141) still does too.

Tests and fixes for these are available in the series posted at https://gcc.gnu.org/pipermail/libstdc++/2025-September/063516.html .
Comment 5 GCC Commits 2025-10-14 16:34:43 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:5dba17a3e709859968f939354e6e5e8d796012d3

commit r16-4425-g5dba17a3e709859968f939354e6e5e8d796012d3
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Oct 9 11:09:34 2025 +0100

    libstdc++: Avoid overflow in timeout conversions [PR113327]
    
    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.
    
    The new __to_timeout_gthread_time_t function in <bits/std_mutex.h>
    requires adding <bits/chrono.h> to that header, but that only affects
    <syncstream>. All other consumers of <bits/std_mutex.h> were already
    including <bits/chrono.h> for timeouts (e.g. <shared_mutex> and
    <condition_variable>).
    
    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.
            * include/bits/std_mutex.h (__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>
    Reviewed-by: Tomasz KamiÅski <tkaminsk@redhat.com>
Comment 6 GCC Commits 2025-10-14 16:34:52 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:e0525a6d2bba07d623424ab6f10a023de90e65a2

commit r16-4426-ge0525a6d2bba07d623424ab6f10a023de90e65a2
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 14 21:21:28 2025 +0100

    libstdc++: Add std::binary_semaphore tests for negative timeouts [PR116586]
    
    Add test cases to prove that negative timeouts are correctly handled by
    std::binary_semaphore (which is just an alias for
    std::counting_semaphore<1>).  The tests exercise cases that aren't
    problematic with the current code since system_clock is converted to
    steady_clock before calling __platform_wait_until() is called but they
    will protect against changes in the implementation reintroducing this
    bug.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/semaphore/try_acquire_for.cc: Add tests.
            * testsuite/30_threads/semaphore/try_acquire_until.cc: Add
            tests.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 7 GCC Commits 2025-10-14 16:35:05 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:fb558b78108ff2974248190bb64b4c19215535bc

commit r16-4427-gfb558b78108ff2974248190bb64b4c19215535bc
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Oct 9 11:11:44 2025 +0100

    libstdc++: Add std::shared_timed_mutex tests for negative timeouts [PR116586]
    
    Add tests to show that std::shared_timed_mutex correctly handles
    negative timeouts.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/shared_timed_mutex/try_lock_until/116586.cc:
            New test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 8 GCC Commits 2025-10-14 16:35:08 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:06782e7aac42a38c5e04a223cc73e1964a64c7d1

commit r16-4431-g06782e7aac42a38c5e04a223cc73e1964a64c7d1
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 14 21:21:33 2025 +0100

    libstdc++: Add std::timed_mutex tests for negative timeouts [PR116586]
    
    Add tests to show that std::timed_mutex::try_lock_until and
    std::timed_mutex::try_lock_for correctly handle negative timeouts.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/timed_mutex/try_lock_until/116586.cc: New
            test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 9 GCC Commits 2025-10-14 16:35:12 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:87c994095b17892922bfcd3c154c51526ece2260

commit r16-4428-g87c994095b17892922bfcd3c154c51526ece2260
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 14 21:21:30 2025 +0100

    libstdc++: Add std::condition_variable tests for negative timeouts [PR116586]
    
    Add tests to show that std::condition_variable::wait_until and
    std::condition_variable::wait_for correctly handle negative timeouts.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/condition_variable/members/116586.cc: New
            test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 10 GCC Commits 2025-10-14 16:35:25 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:bb96099a8dc8cf69bb0d2e424c3b38e6365f90f6

commit r16-4429-gbb96099a8dc8cf69bb0d2e424c3b38e6365f90f6
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 14 21:21:31 2025 +0100

    libstdc++: Add std::future tests for negative timeouts [PR116586]
    
    Add tests to show that std::future::wait_until and
    std::future::wait_for correctly handle negative timeouts.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/future/members/116586.cc: New test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 11 GCC Commits 2025-10-14 16:35:29 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:9a3ff39d6b4a3e900533db64c8071317bd7b8a17

commit r16-4430-g9a3ff39d6b4a3e900533db64c8071317bd7b8a17
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 14 21:21:32 2025 +0100

    libstdc++: Add std::recursive_timed_mutex tests for negative timeouts [PR116586]
    
    Add tests to show that std::recursive_timed_mutex::try_lock_until and
    std::recursive_timed_mutex::try_lock_for correctly handle negative
    timeouts.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/recursive_timed_mutex/try_lock_until/116586.cc:
            New test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 12 GCC Commits 2025-10-14 16:35:45 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:6e9ab669b584496dd628700d0d69bd20c5c12ca7

commit r16-4432-g6e9ab669b584496dd628700d0d69bd20c5c12ca7
Author: Mike Crowe <mac@mcrowe.com>
Date:   Sun Sep 21 17:15:52 2025 +0100

    libstdc++: Add negative this_thread::sleep tests [PR116586]
    
    Add tests to ensure that std::this_thread::sleep_for() and
    std::this_thread::sleep_until() cope with being passed negative times
    correctly. These tests prove that the functions don't suffer from
    libstdc++/PR116586, and will stay that way.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/116586
            * testsuite/30_threads/this_thread/sleep_for.cc: Add
            test_negative() test.
            * testsuite/30_threads/this_thread/sleep_until.cc: Make existing
            test use both system_clock and steady_clock. Add test_negative()
            test.
    
    Signed-off-by: Mike Crowe <mac@mcrowe.com>
Comment 13 Jonathan Wakely 2025-10-14 20:51:26 UTC
Fixed for GCC 16. The issues in shared_timed_mutex can be handled separately.