public inbox for libstdc++-cvs@sourceware.org
help / color / mirror / Atom feed
From: Jonathan Wakely <redi@gcc.gnu.org>
To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org
Subject: [gcc r10-9941] libstdc++: Fix null dereferences in std::promise
Date: Fri, 18 Jun 2021 12:56:49 +0000 (GMT)	[thread overview]
Message-ID: <20210618125649.789D139DC4E7@sourceware.org> (raw)

https://gcc.gnu.org/g:98efaa7ea4d24355024eae7874309efd60baf97e

commit r10-9941-g98efaa7ea4d24355024eae7874309efd60baf97e
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue May 4 16:28:57 2021 +0100

    libstdc++: Fix null dereferences in std::promise
    
    This fixes some ubsan errors in std::promise:
    
    future:1153:34: runtime error: member call on null pointer of type 'struct element_type'
    future:1153:34: runtime error: member access within null pointer of type 'struct element_type'
    
    The problem is that the check for a null pointer is done inside the
    _State::__Setter function, which is only evaluated after evaluating the
    _M_future->_M_set_result postfix-expression.
    
    This change adds a new promise::_M_state() helper for accessing
    _M_future, and moves the check for no shared state into there, instead
    of inside the __setter functions. The __setter functions are made
    always_inline, to avoid the situation where the linker selects the old
    version of set_value (without the _S_check call) and the new version of
    __setter (without the _S_check call) and so there is no check. With the
    always_inline attribute the old version of set_value will either inline
    the old __setter or call an extern definition of it, and the new
    set_value will do the check itself, so both versions do the check.
    
    libstdc++-v3/ChangeLog:
    
            * include/std/future (promise::set_value): Check for existence
            of shared state before dereferncing it.
            (promise::set_exception, promise::set_value_at_thread_exit)
            (promise::set_exception_at_thread_exit): Likewise.
            (promise<R&>::set_value, promise<R&>::set_exception)
            (promise<R&>::set_value_at_thread_exit)
            (promise<R&>::set_exception_at_thread_exit): Likewise.
            (promise<void>::set_value, promise<void>::set_exception)
            (promise<void>::set_value_at_thread_exit)
            (promise<void>::set_exception_at_thread_exit): Likewise.
            * testsuite/30_threads/promise/members/at_thread_exit2.cc:
            Remove unused variable.
    
    (cherry picked from commit 058d6acefe8bac4a66c8e7fb4951276db188e2d8)

Diff:
---
 libstdc++-v3/include/std/future                    | 64 +++++++++++++++-------
 .../30_threads/promise/members/at_thread_exit2.cc  |  1 -
 2 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/libstdc++-v3/include/std/future b/libstdc++-v3/include/std/future
index ceb1959e3b7..a036dc15682 100644
--- a/libstdc++-v3/include/std/future
+++ b/libstdc++-v3/include/std/future
@@ -533,26 +533,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
         };
 
       template<typename _Res, typename _Arg>
+	__attribute__((__always_inline__))
         static _Setter<_Res, _Arg&&>
-        __setter(promise<_Res>* __prom, _Arg&& __arg)
+        __setter(promise<_Res>* __prom, _Arg&& __arg) noexcept
         {
-	  _S_check(__prom->_M_future);
           return _Setter<_Res, _Arg&&>{ __prom, std::__addressof(__arg) };
         }
 
       template<typename _Res>
+	__attribute__((__always_inline__))
         static _Setter<_Res, __exception_ptr_tag>
-        __setter(exception_ptr& __ex, promise<_Res>* __prom)
+        __setter(exception_ptr& __ex, promise<_Res>* __prom) noexcept
         {
-	  _S_check(__prom->_M_future);
           return _Setter<_Res, __exception_ptr_tag>{ __prom, &__ex };
         }
 
       template<typename _Res>
+	__attribute__((__always_inline__))
 	static _Setter<_Res, void>
-	__setter(promise<_Res>* __prom)
+	__setter(promise<_Res>* __prom) noexcept
 	{
-	  _S_check(__prom->_M_future);
 	  return _Setter<_Res, void>{ __prom };
 	}
 
@@ -1118,36 +1118,44 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Setting the result
       void
       set_value(const _Res& __r)
-      { _M_future->_M_set_result(_State::__setter(this, __r)); }
+      { _M_state()._M_set_result(_State::__setter(this, __r)); }
 
       void
       set_value(_Res&& __r)
-      { _M_future->_M_set_result(_State::__setter(this, std::move(__r))); }
+      { _M_state()._M_set_result(_State::__setter(this, std::move(__r))); }
 
       void
       set_exception(exception_ptr __p)
-      { _M_future->_M_set_result(_State::__setter(__p, this)); }
+      { _M_state()._M_set_result(_State::__setter(__p, this)); }
 
       void
       set_value_at_thread_exit(const _Res& __r)
       {
-	_M_future->_M_set_delayed_result(_State::__setter(this, __r),
+	_M_state()._M_set_delayed_result(_State::__setter(this, __r),
 					 _M_future);
       }
 
       void
       set_value_at_thread_exit(_Res&& __r)
       {
-	_M_future->_M_set_delayed_result(
+	_M_state()._M_set_delayed_result(
 	    _State::__setter(this, std::move(__r)), _M_future);
       }
 
       void
       set_exception_at_thread_exit(exception_ptr __p)
       {
-	_M_future->_M_set_delayed_result(_State::__setter(__p, this),
+	_M_state()._M_set_delayed_result(_State::__setter(__p, this),
 					 _M_future);
       }
+
+    private:
+      _State&
+      _M_state()
+      {
+	__future_base::_State_base::_S_check(_M_future);
+	return *_M_future;
+      }
     };
 
   template<typename _Res>
@@ -1229,25 +1237,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Setting the result
       void
       set_value(_Res& __r)
-      { _M_future->_M_set_result(_State::__setter(this, __r)); }
+      { _M_state()._M_set_result(_State::__setter(this, __r)); }
 
       void
       set_exception(exception_ptr __p)
-      { _M_future->_M_set_result(_State::__setter(__p, this)); }
+      { _M_state()._M_set_result(_State::__setter(__p, this)); }
 
       void
       set_value_at_thread_exit(_Res& __r)
       {
-	_M_future->_M_set_delayed_result(_State::__setter(this, __r),
+	_M_state()._M_set_delayed_result(_State::__setter(this, __r),
 					 _M_future);
       }
 
       void
       set_exception_at_thread_exit(exception_ptr __p)
       {
-	_M_future->_M_set_delayed_result(_State::__setter(__p, this),
+	_M_state()._M_set_delayed_result(_State::__setter(__p, this),
 					 _M_future);
       }
+
+    private:
+      _State&
+      _M_state()
+      {
+	__future_base::_State_base::_S_check(_M_future);
+	return *_M_future;
+      }
     };
 
   /// Explicit specialization for promise<void>
@@ -1321,22 +1337,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // Setting the result
       void
       set_value()
-      { _M_future->_M_set_result(_State::__setter(this)); }
+      { _M_state()._M_set_result(_State::__setter(this)); }
 
       void
       set_exception(exception_ptr __p)
-      { _M_future->_M_set_result(_State::__setter(__p, this)); }
+      { _M_state()._M_set_result(_State::__setter(__p, this)); }
 
       void
       set_value_at_thread_exit()
-      { _M_future->_M_set_delayed_result(_State::__setter(this), _M_future); }
+      { _M_state()._M_set_delayed_result(_State::__setter(this), _M_future); }
 
       void
       set_exception_at_thread_exit(exception_ptr __p)
       {
-	_M_future->_M_set_delayed_result(_State::__setter(__p, this),
+	_M_state()._M_set_delayed_result(_State::__setter(__p, this),
 					 _M_future);
       }
+
+    private:
+      _State&
+      _M_state()
+      {
+	__future_base::_State_base::_S_check(_M_future);
+	return *_M_future;
+      }
     };
 
   template<typename _Ptr_type, typename _Fn, typename _Res>
diff --git a/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc b/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc
index 59eeae823ec..7db52a6336e 100644
--- a/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc
+++ b/libstdc++-v3/testsuite/30_threads/promise/members/at_thread_exit2.cc
@@ -119,7 +119,6 @@ void test02()
 void test03()
 {
   std::promise<void> p1;
-  int i = 0;
   p1.set_value();
   try {
     p1.set_value_at_thread_exit();


                 reply	other threads:[~2021-06-18 12:56 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210618125649.789D139DC4E7@sourceware.org \
    --to=redi@gcc.gnu.org \
    --cc=gcc-cvs@gcc.gnu.org \
    --cc=libstdc++-cvs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).