On Wed, 31 Jul 2024 at 22:39, Jonathan Wakely <jwak...@redhat.com> wrote:
>
> On Wed, 24 Jul 2024 at 14:14, William Tsai <william.t...@sifive.com> wrote:
> >
> > The template `future.wait_until` will expand to
> > `_M_load_and_test_until_impl` where it will call
> > `_M_load_and_test_until*` with given time_point casted into second and
> > nanosecond. The callee expects the caller to provide the values
> > correctly from caller while the caller did not make check with those
> > values. One possible error is that if `future.wait_until` is given with
> > a negative time_point, the underlying system call will raise an error as
> > the system call does not accept second < 0 and nanosecond < 1.
>
> Thanks for the patch, it looks correct. The futex syscall returns
> EINVAL in this case, which we don't handle, so the caller loops and
> keeps calling the syscall again, which fails again the same way.
>
> I think it would be good to mention EINVAL, e.g. "will raise an EINVAL
> error" instead of just "will raise an error".

I was finally getting around to merging this patch and realised the
fix is actually much simpler. We do already check for negative
timeouts, we just didn't handle values between -1s and 0s. We can fix
it in the library, without changing the headers:

--- a/libstdc++-v3/src/c++11/futex.cc
+++ b/libstdc++-v3/src/c++11/futex.cc
@@ -128,7 +128,7 @@ namespace
       if (!futex_clock_realtime_unavailable.load(std::memory_order_relaxed))
         {
           // futex sets errno=EINVAL for absolute timeouts before the epoch.
-           if (__s.count() < 0)
+           if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
             return false;

           syscall_timespec rt;
@@ -204,7 +204,7 @@ namespace
       if (!futex_clock_monotonic_unavailable.load(std::memory_order_relaxed))
         {
           // futex sets errno=EINVAL for absolute timeouts before the epoch.
-           if (__s.count() < 0) [[unlikely]]
+           if (__s.count() < 0 || __ns.count() < 0) [[unlikely]]
             return false;

           syscall_timespec rt;


A timeout of 10ms before the epoch gets broken down into 0s and -10ms
and so the __s.count() < 0 check is false, but the futex syscall still
returns EINVAL.

So I'll fix this that way instead.


>
> It would also be good to add a test to the testsuite for this.
>
> Do you have git write access, or do you need me to push it once it's approved?
>
>
> >
> > Following is a simple testcase:
> > ```
> > #include <chrono>
> > #include <future>
> > #include <iostream>
> >
> > using namespace std;
> >
> > int main() {
> >     promise<void> p;
> >     future<void> f = p.get_future();
> >     chrono::steady_clock::time_point tp(chrono::milliseconds{-10});
> >     future_status status = f.wait_until(tp);
> >     if (status == future_status::timeout) {
> >         cout << "Timed out" << endl;
> >     }
> >     return 0;
> > }
> > ```
> >
> > libstdc++-v3/ChangeLog:
> >
> >         * include/bits/atomic_futex.h: Check if __s and __ns is valid before
> >         calling before calling _M_load_and_test_until*
> >
> > Signed-off-by: William Tsai <william.t...@sifive.com>
> > ---
> >  libstdc++-v3/include/bits/atomic_futex.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/libstdc++-v3/include/bits/atomic_futex.h 
> > b/libstdc++-v3/include/bits/atomic_futex.h
> > index dd654174873..4c31946a97f 100644
> > --- a/libstdc++-v3/include/bits/atomic_futex.h
> > +++ b/libstdc++-v3/include/bits/atomic_futex.h
> > @@ -173,6 +173,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
> >        auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - 
> > __s);
> >        // XXX correct?
> > +      if ((__s.time_since_epoch().count() < 0) || (__ns.count() < 0))
> > +       return false;
> >        return _M_load_and_test_until(__assumed, __operand, __equal, __mo,
> >           true, __s.time_since_epoch(), __ns);
> >      }
> > @@ -186,6 +188,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >        auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
> >        auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - 
> > __s);
> >        // XXX correct?
> > +      if ((__s.time_since_epoch().count() < 0) || (__ns.count() < 0))
> > +       return false;
> >        return _M_load_and_test_until_steady(__assumed, __operand, __equal, 
> > __mo,
> >           true, __s.time_since_epoch(), __ns);
> >      }
> > --
> > 2.37.1
> >

Reply via email to