libstdc++: Fix future.wait_until when given a negative time_point

  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.

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.tsai@sifive.com>
---
 libstdc++-v3/include/bits/atomic_futex.h | 4 ++++
 1 file changed, 4 insertions(+)
  

On Wed, 24 Jul 2024 at 14:14, William Tsai <william.tsai@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".

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.tsai@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
>
  

On Wed, 31 Jul 2024 at 22:39, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Wed, 24 Jul 2024 at 14:14, William Tsai <william.tsai@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.tsai@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
> >
  

On Tue, 17 Dec 2024 at 19:38, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Wed, 31 Jul 2024 at 22:39, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Wed, 24 Jul 2024 at 14:14, William Tsai <william.tsai@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.

Here's what I'm testing.
commit a3f4ab3c7bfda80b5dd3ffe56b2accad15de3d71
Author:     Jonathan Wakely <jwakely@redhat.com>
AuthorDate: Tue Dec 17 21:32:19 2024
Commit:     Jonathan Wakely <redi@gcc.gnu.org>
CommitDate: Wed Dec 18 00:35:16 2024

    libstdc++: Fix std::future::wait_until for subsecond negative times [PR118093]
    
    The current check for negative times (i.e. before the epoch) only checks
    for a negative number of seconds. For a time 1ms before the epoch the
    seconds part will be zero, but the futex syscall will still fail with an
    EINVAL error. Extend the check to handle this case.
    
    This change adds a redundant check in the headers too, so that we avoid
    even calling into the library for negative times. Both checks can be
    marked [[unlikely]].
    
    libstdc++-v3/ChangeLog:
    
            PR libstdc++/118093
            * include/bits/atomic_futex.h (_M_load_and_test_until_impl):
            Return false for times before the epoch.
            * src/c++11/futex.cc (_M_futex_wait_until): Extend check for
            negative times to check for subsecond times. Add unlikely
            attribute.
            (_M_futex_wait_until_steady): Likewise.
            * testsuite/30_threads/future/members/118093.cc: New test.

diff --git a/libstdc++-v3/include/bits/atomic_futex.h b/libstdc++-v3/include/bits/atomic_futex.h
index c7d90466418..fe13508462a 100644
--- a/libstdc++-v3/include/bits/atomic_futex.h
+++ b/libstdc++-v3/include/bits/atomic_futex.h
@@ -172,11 +172,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	bool __equal, memory_order __mo,
 	const chrono::time_point<std::chrono::system_clock, _Dur>& __atime)
     {
-      auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
-      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
-      // XXX correct?
+      auto __d = __atime.time_since_epoch();
+      if (__d < __d.zero()) [[__unlikely__]]
+	return false;
+      auto __s = chrono::duration_cast<chrono::seconds>(__d);
+      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
       return _M_load_and_test_until(__assumed, __operand, __equal, __mo,
-	  true, __s.time_since_epoch(), __ns);
+				    true, __s, __ns);
     }
 
     template<typename _Dur>
@@ -185,11 +187,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	bool __equal, memory_order __mo,
 	const chrono::time_point<std::chrono::steady_clock, _Dur>& __atime)
     {
-      auto __s = chrono::time_point_cast<chrono::seconds>(__atime);
-      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__atime - __s);
-      // XXX correct?
+      auto __d = __atime.time_since_epoch();
+      if (__d < __d.zero()) [[__unlikely__]]
+	return false;
+      auto __s = chrono::duration_cast<chrono::seconds>(__d);
+      auto __ns = chrono::duration_cast<chrono::nanoseconds>(__d - __s);
       return _M_load_and_test_until_steady(__assumed, __operand, __equal, __mo,
-	  true, __s.time_since_epoch(), __ns);
+	  true, __s, __ns);
     }
 
   public:
diff --git a/libstdc++-v3/src/c++11/futex.cc b/libstdc++-v3/src/c++11/futex.cc
index ada2fdf3c41..6139fc354c6 100644
--- 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;
diff --git a/libstdc++-v3/testsuite/30_threads/future/members/118093.cc b/libstdc++-v3/testsuite/30_threads/future/members/118093.cc
new file mode 100644
index 00000000000..2bb1e1c6dad
--- /dev/null
+++ b/libstdc++-v3/testsuite/30_threads/future/members/118093.cc
@@ -0,0 +1,26 @@
+// { dg-do run { target c++11 } }
+
+#include <chrono>
+#include <future>
+
+void
+test_sys()
+{
+  std::promise<void> p;
+  std::chrono::system_clock::time_point tp(std::chrono::milliseconds{-10});
+  (void) p.get_future().wait_until(tp);
+}
+
+void
+test_steady()
+{
+  std::promise<void> p;
+  std::chrono::steady_clock::time_point tp(std::chrono::milliseconds{-10});
+  (void) p.get_future().wait_until(tp);
+}
+
+int main()
+{
+  test_sys();
+  test_steady();
+}
  

On Wed, 18 Dec 2024 at 00:35, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Tue, 17 Dec 2024 at 19:38, Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> > On Wed, 31 Jul 2024 at 22:39, Jonathan Wakely <jwakely@redhat.com> wrote:
> > >
> > > On Wed, 24 Jul 2024 at 14:14, William Tsai <william.tsai@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.
>
> Here's what I'm testing.

Pushed to trunk now.