Bug 93456 - No overflow check in __atomic_futex_unsigned_base::_M_futex_wait_until
Summary: No overflow check in __atomic_futex_unsigned_base::_M_futex_wait_until
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 10.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-27 14:58 UTC by Jonathan Wakely
Modified: 2021-04-19 10:40 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-01-27 00:00:00


Attachments
Avoid overflow and loop (808 bytes, patch)
2020-01-27 15:31 UTC, Jonathan Wakely
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Wakely 2020-01-27 14:58:43 UTC
#include <future>
#include <climits>

int main()
{
  std::promise<void> p;
  auto f = p.get_future();
  auto now = std::chrono::system_clock::now();
  std::chrono::seconds s(INT_MAX + 10ull);
  f.wait_until(now + s);
}

On 32-bit linux with a 32-bit time_t this fails to wait and returns immediately.

The problem is __atomic_futex_unsigned_base::_M_futex_wait_until which doesn't check whether __s.count() - tv.tv_sec fits in time_t.
Comment 1 Jonathan Wakely 2020-01-27 15:31:16 UTC
Created attachment 47716 [details]
Avoid overflow and loop

This is a patch to make the function loop if necessary, waiting no more than numeric_limits<time_t>::max() at a time.

The change is slightly more convoluted than might seem necessary, because it is intended to continue working after the type of rt.tv_sec changes (for PR 93421).

I'll wait for stage 1 to make this change.
Comment 2 Jonathan Wakely 2020-01-27 15:37:28 UTC
Testcase that terminates in reasonable time:

#include <future>
#include <thread>
#include <chrono>
#include <climits>
#include <cassert>

int main()
{
  using namespace std;
  using namespace std::chrono;
  auto now = system_clock::now();
  seconds s(INT_MAX + 10ull);
  auto f = async(launch::async, [] { this_thread::sleep_for(2s); });
  f.wait_until(now + seconds(INT_MAX) + 10s );
  auto then = system_clock::now();
  assert( (then - now) > 1s );
  f.wait();
}

The assertion fails today, but passes with the patch above.
Comment 3 GCC Commits 2020-11-14 00:16:13 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

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

commit r11-5021-gb8d36dcc917e8a06d8c20b9f5ecc920ed2b9e947
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 13 20:57:15 2020 +0000

    libstdc++: Remove redundant overflow check for futex timeout [PR 93456]
    
    The relative_timespec function already checks for the case where the
    specified timeout is in the past, so the difference can never be
    negative. That means we dn't need to check if it's more negative than
    the minimum time_t value.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/93456
            * src/c++11/futex.cc (relative_timespec): Remove redundant check
            negative values.
            * testsuite/30_threads/future/members/wait_until_overflow.cc: Moved to...
            * testsuite/30_threads/future/members/93456.cc: ...here.
Comment 4 Jonathan Wakely 2020-11-14 00:25:26 UTC
Fixed on trunk by the patch above and a few other recent commits today.
Comment 5 GCC Commits 2020-11-16 21:15:26 UTC
The releases/gcc-10 branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:730e1357dfb9aff481d6c47a21ef748f0d810d4f

commit r10-9031-g730e1357dfb9aff481d6c47a21ef748f0d810d4f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Nov 13 19:11:02 2020 +0000

    libstdc++: Remove redundant overflow check for futex timeout [PR 93456]
    
    The relative_timespec function already checks for the case where the
    specified timeout is in the past, so the difference can never be
    negative. That means we dn't need to check if it's more negative than
    the minimum time_t value.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/93456
            * src/c++11/futex.cc (relative_timespec): Remove redundant check
            negative values.
            * testsuite/30_threads/future/members/93456.cc: New.
    
    (cherry picked from commit b8d36dcc917e8a06d8c20b9f5ecc920ed2b9e947)
Comment 6 GCC Commits 2020-11-19 13:33:21 UTC
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

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

commit r11-5167-gb108faa9400e13a3d00dd7f71cff0ac45e29c5c9
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 19 13:33:11 2020 +0000

    libstdc++: Fix overflow checks to use the correct "time_t" [PR 93456]
    
    I recently added overflow checks to src/c++11/futex.cc for PR 93456, but
    then changed the type of the timespec for PR 93421. This meant the
    overflow checks were no longer using the right range, because the
    variable being written to might be smaller than time_t.
    
    This introduces new typedef that corresponds to the tv_sec member of the
    struct being passed to the syscall, and uses that typedef in the range
    checks.
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/93421
            PR libstdc++/93456
            * src/c++11/futex.cc (syscall_time_t): New typedef for
            the type of the syscall_timespec::tv_sec member.
            (relative_timespec, _M_futex_wait_until)
            (_M_futex_wait_until_steady): Use syscall_time_t in overflow
            checks, not time_t.
Comment 7 Jonathan Wakely 2020-11-25 15:11:22 UTC
(In reply to CVS Commits from comment #3)
>             * testsuite/30_threads/future/members/93456.cc: ...here.

This test is failing for powerpc-darwin9, probably because I added overflow checks for the futex version of __atomic_futex_unsigned_base but not the generic version that uses a condition variable.
Comment 8 Alexandre Oliva 2020-12-29 13:12:41 UTC
On arm-vxworks, that has 32-bit time_t, this also fails.  The initial gthread_cond_timedwait sleeps for a second or two, then it times out and returns.  However, the wider C++ types used in the condition_variable::__wait_until_impl check for a timeout and decide we have NOT timed out, so the __Predicate version of __wait_until sees the condition it's waiting for hasn't been met, and attempts to wait again.  But since the timeout has already been reached, gthread_cond_wait returns immediately, and we busy-loop.  Since vxworks won't preempt threads, and we're not in a SMP configuration, the async call never gets a chance to complete, and the test runs till the rlimit runs out.

While investigating this, I noticed that adding __gthread_yield calls in the __wait_until_impl loop, the other thread gets enough cycles to complete, and the test passes, but I thought that would defeat the point of the test, right?