[PATCH] PR libstdc++/88782 avoid ODR problems in std::make_shared

Jonathan Wakely jwakely@redhat.com
Fri Jan 18 21:28:00 GMT 2019


The old version of _Sp_counted_ptr_inplace::_M_get_deleter (up to GCC
8.2.0) expects to be passed a real std::typeinfo object, so mixing that
with the new definition of the __shared_ptr constructor (which always
passes the fake tag) leads to accessing the fake object as a real
std::typeinfo. Instead of trying to make it safe to mix the old and new
definitions, just stop using that function. By passing a reference to
__shared_ptr::_M_ptr to the __shared_count constructor it can be set
directly, without needing to obtain the pointer via the _M_get_deleter
back-channel. This avoids a virtual dispatch (which fixes PR 87514).

This means that code built against new libstdc++ headers doesn't use
_M_get_deleter at all, and so make_shared works the same whether RTTI is
enabled or not.

Also change _M_get_deleter so that it checks for a real type_info object
even when RTTI is disabled, by calling a library function. Unless
libstdc++ itself is built without RTTI that library function will be
able to test if it's the right type_info. This means the new definition
of _M_get_deleter can handle both the fake type_info tag and a real
type_info object, even if built without RTTI.

If linking to objects built against older versions of libstdc++ then if
all objects use -frtti or all use -fno-rtti, then the caller of
_M_get_deleter and the definition of _M_get_deleter will be consistent
and it will work. If mixing -frtti with -fno-rtti it can still fail if
the linker picks an old definition of _M_get_deleter and an old
__shared_ptr constructor that are incompatible. In that some or all
objects might need to be recompiled.

	PR libstdc++/87514
	PR libstdc++/87520
	PR libstdc++/88782
	* config/abi/pre/gnu.ver (GLIBCXX_3.4.26): Export new symbol.
	* include/bits/shared_ptr.h
	(shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...))
	(allocate_shared): Change to use new tag type.
	* include/bits/shared_ptr_base.h (_Sp_make_shared_tag::_S_eq):
	Declare new member function.
	(_Sp_alloc_shared_tag): Define new type.
	(_Sp_counted_ptr_inplace): Declare __shared_count<_Lp> as a friend.
	(_Sp_counted_ptr_inplace::_M_get_deleter) [!__cpp_rtti]: Use
	_Sp_make_shared_tag::_S_eq to check type_info.
	(__shared_count(Ptr, Deleter),__shared_count(Ptr, Deleter, Alloc)):
	Constrain to prevent being called with _Sp_alloc_shared_tag.
	(__shared_count(_Sp_make_shared_tag, const _Alloc&, Args&&...)):
	Replace constructor with ...
	(__shared_count(Tp*&, _Sp_alloc_shared_tag<_Alloc>, Args&&...)): Use
	reference parameter so address of the new object can be returned to
	the caller. Obtain the allocator from the tag type.
	(__shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...)): Replace
	constructor with ...
	(__shared_ptr(_Sp_alloc_shared_tag<Alloc>, Args&&...)): Pass _M_ptr
	to the __shared_count constructor.
	(__allocate_shared): Change to use new tag type.
	* src/c++11/shared_ptr.cc (_Sp_make_shared_tag::_S_eq): Define.

Tested powerpc64le-linux, committed to trunk.

I'll backport this to gcc-8-branch without the new symbol in the library.


-------------- next part --------------
commit f4334034ec92d0a6cdf6dc3244a25108f32fc89a
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 17 20:46:09 2019 +0000

    PR libstdc++/88782 avoid ODR problems in std::make_shared
    
    The old version of _Sp_counted_ptr_inplace::_M_get_deleter (up to GCC
    8.2.0) expects to be passed a real std::typeinfo object, so mixing that
    with the new definition of the __shared_ptr constructor (which always
    passes the fake tag) leads to accessing the fake object as a real
    std::typeinfo. Instead of trying to make it safe to mix the old and new
    definitions, just stop using that function. By passing a reference to
    __shared_ptr::_M_ptr to the __shared_count constructor it can be set
    directly, without needing to obtain the pointer via the _M_get_deleter
    back-channel. This avoids a virtual dispatch (which fixes PR 87514).
    
    This means that code built against new libstdc++ headers doesn't use
    _M_get_deleter at all, and so make_shared works the same whether RTTI is
    enabled or not.
    
    Also change _M_get_deleter so that it checks for a real type_info object
    even when RTTI is disabled, by calling a library function. Unless
    libstdc++ itself is built without RTTI that library function will be
    able to test if it's the right type_info. This means the new definition
    of _M_get_deleter can handle both the fake type_info tag and a real
    type_info object, even if built without RTTI.
    
    If linking to objects built against older versions of libstdc++ then if
    all objects use -frtti or all use -fno-rtti, then the caller of
    _M_get_deleter and the definition of _M_get_deleter will be consistent
    and it will work. If mixing -frtti with -fno-rtti it can still fail if
    the linker picks an old definition of _M_get_deleter and an old
    __shared_ptr constructor that are incompatible. In that some or all
    objects might need to be recompiled.
    
            PR libstdc++/87514
            PR libstdc++/87520
            PR libstdc++/88782
            * config/abi/pre/gnu.ver (GLIBCXX_3.4.26): Export new symbol.
            * include/bits/shared_ptr.h
            (shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...))
            (allocate_shared): Change to use new tag type.
            * include/bits/shared_ptr_base.h (_Sp_make_shared_tag::_S_eq):
            Declare new member function.
            (_Sp_alloc_shared_tag): Define new type.
            (_Sp_counted_ptr_inplace): Declare __shared_count<_Lp> as a friend.
            (_Sp_counted_ptr_inplace::_M_get_deleter) [!__cpp_rtti]: Use
            _Sp_make_shared_tag::_S_eq to check type_info.
            (__shared_count(Ptr, Deleter),__shared_count(Ptr, Deleter, Alloc)):
            Constrain to prevent being called with _Sp_alloc_shared_tag.
            (__shared_count(_Sp_make_shared_tag, const _Alloc&, Args&&...)):
            Replace constructor with ...
            (__shared_count(Tp*&, _Sp_alloc_shared_tag<_Alloc>, Args&&...)): Use
            reference parameter so address of the new object can be returned to
            the caller. Obtain the allocator from the tag type.
            (__shared_ptr(_Sp_make_shared_tag, const Alloc&, Args&&...)): Replace
            constructor with ...
            (__shared_ptr(_Sp_alloc_shared_tag<Alloc>, Args&&...)): Pass _M_ptr
            to the __shared_count constructor.
            (__allocate_shared): Change to use new tag type.
            * src/c++11/shared_ptr.cc (_Sp_make_shared_tag::_S_eq): Define.

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index 3b254b2289f..34c70b6cb8f 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -2235,6 +2235,9 @@ GLIBCXX_3.4.26 {
     _ZNSolsEDn;
     _ZNSt13basic_ostreamIwSt11char_traitsIwEElsEDn;
 
+    # _Sp_make_shared_tag::_S_eq
+    _ZNSt19_Sp_make_shared_tag5_S_eqERKSt9type_info;
+
 } GLIBCXX_3.4.25;
 
 # Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/include/bits/shared_ptr.h b/libstdc++-v3/include/bits/shared_ptr.h
index d504627d1a0..ee815f0d0a1 100644
--- a/libstdc++-v3/include/bits/shared_ptr.h
+++ b/libstdc++-v3/include/bits/shared_ptr.h
@@ -355,9 +355,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     private:
       // This constructor is non-standard, it is used by allocate_shared.
       template<typename _Alloc, typename... _Args>
-	shared_ptr(_Sp_make_shared_tag __tag, const _Alloc& __a,
-		   _Args&&... __args)
-	: __shared_ptr<_Tp>(__tag, __a, std::forward<_Args>(__args)...)
+	shared_ptr(_Sp_alloc_shared_tag<_Alloc> __tag, _Args&&... __args)
+	: __shared_ptr<_Tp>(__tag, std::forward<_Args>(__args)...)
 	{ }
 
       template<typename _Yp, typename _Alloc, typename... _Args>
@@ -699,7 +698,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline shared_ptr<_Tp>
     allocate_shared(const _Alloc& __a, _Args&&... __args)
     {
-      return shared_ptr<_Tp>(_Sp_make_shared_tag(), __a,
+      return shared_ptr<_Tp>(_Sp_alloc_shared_tag<_Alloc>{__a},
 			     std::forward<_Args>(__args)...);
     }
 
diff --git a/libstdc++-v3/include/bits/shared_ptr_base.h b/libstdc++-v3/include/bits/shared_ptr_base.h
index b45cbf73667..0367c2d51a5 100644
--- a/libstdc++-v3/include/bits/shared_ptr_base.h
+++ b/libstdc++-v3/include/bits/shared_ptr_base.h
@@ -501,8 +501,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   struct _Sp_make_shared_tag
   {
   private:
-    template<typename _Tp, _Lock_policy _Lp>
-      friend class __shared_ptr;
     template<typename _Tp, typename _Alloc, _Lock_policy _Lp>
       friend class _Sp_counted_ptr_inplace;
 
@@ -512,8 +510,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       alignas(type_info) static constexpr char __tag[sizeof(type_info)] = { };
       return reinterpret_cast<const type_info&>(__tag);
     }
+
+    static bool _S_eq(const type_info&) noexcept;
   };
 
+  template<typename _Alloc>
+    struct _Sp_alloc_shared_tag
+    {
+      const _Alloc& _M_a;
+    };
+
   template<typename _Tp, typename _Alloc, _Lock_policy _Lp>
     class _Sp_counted_ptr_inplace final : public _Sp_counted_base<_Lp>
     {
@@ -560,24 +566,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	this->~_Sp_counted_ptr_inplace();
       }
 
-      // Sneaky trick so __shared_ptr can get the managed pointer.
+    private:
+      friend class __shared_count<_Lp>; // To be able to call _M_ptr().
+
+      // No longer used, but code compiled against old libstdc++ headers
+      // might still call it from __shared_ptr ctor to get the pointer out.
       virtual void*
       _M_get_deleter(const std::type_info& __ti) noexcept override
       {
+	auto __ptr = const_cast<typename remove_cv<_Tp>::type*>(_M_ptr());
 	// Check for the fake type_info first, so we don't try to access it
-	// as a real type_info object.
-	if (&__ti == &_Sp_make_shared_tag::_S_ti())
-	  return const_cast<typename remove_cv<_Tp>::type*>(_M_ptr());
+	// as a real type_info object. Otherwise, check if it's the real
+	// type_info for this class. With RTTI enabled we can check directly,
+	// or call a library function to do it.
+	if (&__ti == &_Sp_make_shared_tag::_S_ti()
+	    ||
 #if __cpp_rtti
-	// Callers compiled with old libstdc++ headers and RTTI enabled
-	// might pass this instead:
-	else if (__ti == typeid(_Sp_make_shared_tag))
-	  return const_cast<typename remove_cv<_Tp>::type*>(_M_ptr());
+	    __ti == typeid(_Sp_make_shared_tag)
+#else
+	    _Sp_make_shared_tag::_S_eq(__ti)
 #endif
+	   )
+	  return __ptr;
 	return nullptr;
       }
 
-    private:
       _Tp* _M_ptr() noexcept { return _M_impl._M_storage._M_ptr(); }
 
       _Impl _M_impl;
@@ -593,6 +606,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   template<_Lock_policy _Lp>
     class __shared_count
     {
+      template<typename _Tp>
+	struct __not_alloc_shared_tag { using type = void; };
+
+      template<typename _Tp>
+	struct __not_alloc_shared_tag<_Sp_alloc_shared_tag<_Tp>> { };
+
     public:
       constexpr __shared_count() noexcept : _M_pi(0)
       { }
@@ -622,12 +641,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	: __shared_count(__p, __sp_array_delete{}, allocator<void>())
 	{ }
 
-      template<typename _Ptr, typename _Deleter>
+      template<typename _Ptr, typename _Deleter,
+	       typename = typename __not_alloc_shared_tag<_Deleter>::type>
 	__shared_count(_Ptr __p, _Deleter __d)
 	: __shared_count(__p, std::move(__d), allocator<void>())
 	{ }
 
-      template<typename _Ptr, typename _Deleter, typename _Alloc>
+      template<typename _Ptr, typename _Deleter, typename _Alloc,
+	       typename = typename __not_alloc_shared_tag<_Deleter>::type>
 	__shared_count(_Ptr __p, _Deleter __d, _Alloc __a) : _M_pi(0)
 	{
 	  typedef _Sp_counted_deleter<_Ptr, _Deleter, _Alloc, _Lp> _Sp_cd_type;
@@ -648,17 +669,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	}
 
       template<typename _Tp, typename _Alloc, typename... _Args>
-	__shared_count(_Sp_make_shared_tag, _Tp*, const _Alloc& __a,
+	__shared_count(_Tp*& __p, _Sp_alloc_shared_tag<_Alloc> __a,
 		       _Args&&... __args)
-	: _M_pi(0)
 	{
 	  typedef _Sp_counted_ptr_inplace<_Tp, _Alloc, _Lp> _Sp_cp_type;
-	  typename _Sp_cp_type::__allocator_type __a2(__a);
+	  typename _Sp_cp_type::__allocator_type __a2(__a._M_a);
 	  auto __guard = std::__allocate_guarded(__a2);
 	  _Sp_cp_type* __mem = __guard.get();
-	  ::new (__mem) _Sp_cp_type(__a, std::forward<_Args>(__args)...);
-	  _M_pi = __mem;
+	  auto __pi = ::new (__mem)
+	    _Sp_cp_type(__a._M_a, std::forward<_Args>(__args)...);
 	  __guard = nullptr;
+	  _M_pi = __pi;
+	  __p = __pi->_M_ptr();
 	}
 
 #if _GLIBCXX_USE_DEPRECATED
@@ -1318,17 +1340,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     protected:
       // This constructor is non-standard, it is used by allocate_shared.
       template<typename _Alloc, typename... _Args>
-	__shared_ptr(_Sp_make_shared_tag __tag, const _Alloc& __a,
-		     _Args&&... __args)
-	: _M_ptr(), _M_refcount(__tag, (_Tp*)0, __a,
-				std::forward<_Args>(__args)...)
-	{
-	  // _M_ptr needs to point to the newly constructed object.
-	  // This relies on _Sp_counted_ptr_inplace::_M_get_deleter.
-	  void* __p = _M_refcount._M_get_deleter(_Sp_make_shared_tag::_S_ti());
-	  _M_ptr = static_cast<_Tp*>(__p);
-	  _M_enable_shared_from_this_with(_M_ptr);
-	}
+	__shared_ptr(_Sp_alloc_shared_tag<_Alloc> __tag, _Args&&... __args)
+	: _M_ptr(), _M_refcount(_M_ptr, __tag, std::forward<_Args>(__args)...)
+	{ _M_enable_shared_from_this_with(_M_ptr); }
 
       template<typename _Tp1, _Lock_policy _Lp1, typename _Alloc,
 	       typename... _Args>
@@ -1808,7 +1822,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     inline __shared_ptr<_Tp, _Lp>
     __allocate_shared(const _Alloc& __a, _Args&&... __args)
     {
-      return __shared_ptr<_Tp, _Lp>(_Sp_make_shared_tag(), __a,
+      return __shared_ptr<_Tp, _Lp>(_Sp_alloc_shared_tag<_Alloc>{__a},
 				    std::forward<_Args>(__args)...);
     }
 
diff --git a/libstdc++-v3/src/c++11/shared_ptr.cc b/libstdc++-v3/src/c++11/shared_ptr.cc
index 6e838de3c03..1f1323ef89f 100644
--- a/libstdc++-v3/src/c++11/shared_ptr.cc
+++ b/libstdc++-v3/src/c++11/shared_ptr.cc
@@ -94,5 +94,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   }
 #endif
 
+  bool
+  _Sp_make_shared_tag::_S_eq(const type_info& ti) noexcept
+  {
+#if __cpp_rtti
+    return ti == typeid(_Sp_make_shared_tag);
+#else
+    // If libstdc++ itself is built with -fno-rtti then just assume that
+    // make_shared and allocate_shared will never be used with -frtti.
+    return false;
+#endif
+  }
+
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace


More information about the Libstdc++ mailing list