Fix invalid casts in <functional>.

[eugenis]

Summary

static_cast of a pointer to object before the start of the object's
lifetime has undefined behavior (c++14 p3.8)

This code triggers CFI warnings.

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context. That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter
that the standard).

[EricWF]

This code triggers CFI warnings.

What are CFI warnings? Sorry if that's a dumb question, I just don't recognize "CFI".

[eugenis]

http://clang.llvm.org/docs/ControlFlowIntegrity.html
Basically it says that the cast to base is done on a memory that does not contain an object of type base (based on the vptr value).

[EricWF]

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context.

I agree that using void* to represent raw memory is the better approach than reinterpret_cast<>().
However I'm concerned that changing the signature (and mangling) of virtual void __clone(...) could cause ABI problems.
I *think* this should be "safe" because the VTable's mangled name doesn't change. but if I'm wrong we must use reinterpret_cast<> for calls to __clone(...).

The parts of the patch that don't affect __clone(...) LGTM. You can commit them separably if you want.

That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter that the standard).

I'm sure there are alternative ways to make CFI shut up. Perhaps we could do the Buffer* -> Base* conversion inside a blacklisted function (akin to std::launder)?
It would also be nice to have "__attribute__((__no_sanitize__("cfi"))).

include/functional

1443

Doesn't this break the ABI because it changes the mangling of a virtual function? I understand the vtable should remain layout compatible but I suspect the mangling change is an issue.

1743

__f_ = ::new((void*)&__buf_) _FF(_VSTD::move(__f));

[eugenis]

In D16738#340175, @EricWF wrote:

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context.

I agree that using void* to represent raw memory is the better approach than reinterpret_cast<>().
However I'm concerned that changing the signature (and mangling) of virtual void __clone(...) could cause ABI problems.
I *think* this should be "safe" because the VTable's mangled name doesn't change. but if I'm wrong we must use reinterpret_cast<> for calls to __clone(...).

The parts of the patch that don't affect __clone(...) LGTM. You can commit them separably if you want.

That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter that the standard).

I'm sure there are alternative ways to make CFI shut up. Perhaps we could do the Buffer* -> Base* conversion inside a blacklisted function (akin to std::launder)?
It would also be nice to have "__attribute__((__no_sanitize__("cfi"))).

We do have this attribute.

[eugenis]

How about this?

[EricWF]

I prefer using the (void*) casts when possible. In particular when doing the pointer comparisons. Could you change those back to void* casts then use the __as_base function for the rest?

[eugenis]

In D16738#345696, @EricWF wrote:

I prefer using the (void*) casts when possible. In particular when doing the pointer comparisons. Could you change those back to void* casts then use the __as_base function for the rest?

done

[EricWF]

LGTM. Thanks for putting up with my pickyness. It looks like there are exception safety issues with the order of __f_ = __as_base(&__buf_) and __f->__f_..__clone(__f) in a bunch of places but those are existing and so I'll fix them separately.

Also we are not applying this fix to the C++03 versions of std::function, but I'm OK with that because I want those versions to die.

[eugenis]

Thanks for the review!
Committed as r260441.

include/__config

 # elif __BYTE_ORDER == __BIG_ENDIAN
 #  define _LIBCPP_LITTLE_ENDIAN 0
 #  define _LIBCPP_BIG_ENDIAN    1
 # else  // __BYTE_ORDER == __BIG_ENDIAN
 #  error unable to determine endian
 # endif
 #endif  // !defined(_LIBCPP_LITTLE_ENDIAN) || !defined(_LIBCPP_BIG_ENDIAN)
 
+#if __has_attribute(__no_sanitize__)
+#define _LIBCPP_NO_CFI __attribute__((__no_sanitize__("cfi")))
+#else
+#define _LIBCPP_NO_CFI
+#endif
+
 #ifdef _WIN32
 
 // only really useful for a DLL
 #ifdef _LIBCPP_DLL // this should be a compiler builtin define ideally...
 # ifdef cxx_EXPORTS
 #  define _LIBCPP_HIDDEN
 #  define _LIBCPP_FUNC_VIS __declspec(dllexport)
 #  define _LIBCPP_TYPE_VIS __declspec(dllexport)

include/functional

 class __base<_Rp(_ArgTypes...)>
 {
     __base(const __base&);
     __base& operator=(const __base&);
 public:
     _LIBCPP_INLINE_VISIBILITY __base() {}
     _LIBCPP_INLINE_VISIBILITY virtual ~__base() {}
     virtual __base* __clone() const = 0;
     virtual void __clone(__base*) const = 0;
     virtual void destroy() _NOEXCEPT = 0;
     virtual void destroy_deallocate() _NOEXCEPT = 0;
     virtual _Rp operator()(_ArgTypes&& ...) = 0;
 #ifndef _LIBCPP_NO_RTTI
     virtual const void* target(const type_info&) const _NOEXCEPT = 0;
     virtual const std::type_info& target_type() const _NOEXCEPT = 0;
 #endif  // _LIBCPP_NO_RTTI
 };

 class _LIBCPP_TYPE_VIS_ONLY function<_Rp(_ArgTypes...)>
     : public __function::__maybe_derive_from_unary_function<_Rp(_ArgTypes...)>,
       public __function::__maybe_derive_from_binary_function<_Rp(_ArgTypes...)>
 {
     typedef __function::__base<_Rp(_ArgTypes...)> __base;
     typename aligned_storage<3*sizeof(void*)>::type __buf_;
     __base* __f_;
 
+    _LIBCPP_NO_CFI static __base *__as_base(void *p) {
+      return reinterpret_cast<__base*>(p);
+    }
+
     template <class _Fp, bool = !is_same<_Fp, function>::value &&
                                 __invokable<_Fp&, _ArgTypes...>::value>
         struct __callable;
     template <class _Fp>
         struct __callable<_Fp, true>
         {
             static const bool value = is_same<void, _Rp>::value ||
                 is_convertible<typename __invoke_of<_Fp&, _ArgTypes...>::type,

 #endif  // _LIBCPP_NO_RTTI
 };
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>::function(const function& __f)
 {
     if (__f.__f_ == 0)
         __f_ = 0;
-    else if (__f.__f_ == (const __base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
     else
         __f_ = __f.__f_->__clone();
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Alloc>
 function<_Rp(_ArgTypes...)>::function(allocator_arg_t, const _Alloc&,
                                      const function& __f)
 {
     if (__f.__f_ == 0)
         __f_ = 0;
-    else if (__f.__f_ == (const __base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
     else
         __f_ = __f.__f_->__clone();
 }
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>::function(function&& __f) _NOEXCEPT
 {
     if (__f.__f_ == 0)
         __f_ = 0;
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
     else
     {
         __f_ = __f.__f_;
         __f.__f_ = 0;
     }
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Alloc>
 function<_Rp(_ArgTypes...)>::function(allocator_arg_t, const _Alloc&,
                                      function&& __f)
 {
     if (__f.__f_ == 0)
         __f_ = 0;
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
     else
     {
         __f_ = __f.__f_;
         __f.__f_ = 0;
     }
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Fp>
 function<_Rp(_ArgTypes...)>::function(_Fp __f,
                                      typename enable_if
                                      <
                                         __callable<_Fp>::value &&
                                         !is_same<_Fp, function>::value
                                      >::type*)
     : __f_(0)
 {
     if (__function::__not_null(__f))
     {
         typedef __function::__func<_Fp, allocator<_Fp>, _Rp(_ArgTypes...)> _FF;
         if (sizeof(_FF) <= sizeof(__buf_) && is_nothrow_copy_constructible<_Fp>::value)
         {
-            __f_ = (__base*)&__buf_;
-            ::new (__f_) _FF(_VSTD::move(__f));
+            __f_ = ::new((void*)&__buf_) _FF(_VSTD::move(__f));
         }
         else
         {
             typedef allocator<_FF> _Ap;
             _Ap __a;
             typedef __allocator_destructor<_Ap> _Dp;
             unique_ptr<__base, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
             ::new (__hold.get()) _FF(_VSTD::move(__f), allocator<_Fp>(__a));
function<_Rp(_ArgTypes...)>::function(allocator_arg_t, const _Alloc& __a0, _Fp __f,
     if (__function::__not_null(__f))
     {
         typedef __function::__func<_Fp, _Alloc, _Rp(_ArgTypes...)> _FF;
         typedef typename __rebind_alloc_helper<__alloc_traits, _FF>::type _Ap;
         _Ap __a(__a0);
         if (sizeof(_FF) <= sizeof(__buf_) && 
             is_nothrow_copy_constructible<_Fp>::value && is_nothrow_copy_constructible<_Ap>::value)
         {
-            __f_ = (__base*)&__buf_;
-            ::new (__f_) _FF(_VSTD::move(__f), _Alloc(__a));
+            __f_ = ::new((void*)&__buf_) _FF(_VSTD::move(__f), _Alloc(__a));
         }
         else
         {
             typedef __allocator_destructor<_Ap> _Dp;
             unique_ptr<__base, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
             ::new (__hold.get()) _FF(_VSTD::move(__f), _Alloc(__a));
             __f_ = __hold.release();
         }
     }
 }
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(const function& __f)
 {
     function(__f).swap(*this);
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(function&& __f) _NOEXCEPT
 {
-    if (__f_ == (__base*)&__buf_)
+    if ((void *)__f_ == &__buf_)
         __f_->destroy();
     else if (__f_)
         __f_->destroy_deallocate();
     __f_ = 0;
     if (__f.__f_ == 0)
         __f_ = 0;
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
         __f.__f_->__clone(__f_);
     }
     else
     {
         __f_ = __f.__f_;
         __f.__f_ = 0;
     }
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>&
 function<_Rp(_ArgTypes...)>::operator=(nullptr_t) _NOEXCEPT
 {
-    if (__f_ == (__base*)&__buf_)
+    if ((void *)__f_ == &__buf_)
         __f_->destroy();
     else if (__f_)
         __f_->destroy_deallocate();
     __f_ = 0;
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Fp>
 typename enable_if
 <
     function<_Rp(_ArgTypes...)>::template __callable<typename decay<_Fp>::type>::value &&
     !is_same<typename remove_reference<_Fp>::type, function<_Rp(_ArgTypes...)>>::value,
     function<_Rp(_ArgTypes...)>&
 >::type
 function<_Rp(_ArgTypes...)>::operator=(_Fp&& __f)
 {
     function(_VSTD::forward<_Fp>(__f)).swap(*this);
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 function<_Rp(_ArgTypes...)>::~function()
 {
-    if (__f_ == (__base*)&__buf_)
+    if ((void *)__f_ == &__buf_)
         __f_->destroy();
     else if (__f_)
         __f_->destroy_deallocate();
 }
 
 template<class _Rp, class ..._ArgTypes>
 void
 function<_Rp(_ArgTypes...)>::swap(function& __f) _NOEXCEPT
 {
-    if (__f_ == (__base*)&__buf_ && __f.__f_ == (__base*)&__f.__buf_)
+    if ((void *)__f_ == &__buf_ && (void *)__f.__f_ == &__f.__buf_)
     {
         typename aligned_storage<sizeof(__buf_)>::type __tempbuf;
-        __base* __t = (__base*)&__tempbuf;
+        __base* __t = __as_base(&__tempbuf);
         __f_->__clone(__t);
         __f_->destroy();
         __f_ = 0;
-        __f.__f_->__clone((__base*)&__buf_);
+        __f.__f_->__clone(__as_base(&__buf_));
         __f.__f_->destroy();
         __f.__f_ = 0;
-        __f_ = (__base*)&__buf_;
-        __t->__clone((__base*)&__f.__buf_);
+        __f_ = __as_base(&__buf_);
+        __t->__clone(__as_base(&__f.__buf_));
         __t->destroy();
-        __f.__f_ = (__base*)&__f.__buf_;
+        __f.__f_ = __as_base(&__f.__buf_);
     }
-    else if (__f_ == (__base*)&__buf_)
+    else if ((void *)__f_ == &__buf_)
     {
-        __f_->__clone((__base*)&__f.__buf_);
+        __f_->__clone(__as_base(&__f.__buf_));
         __f_->destroy();
         __f_ = __f.__f_;
-        __f.__f_ = (__base*)&__f.__buf_;
+        __f.__f_ = __as_base(&__f.__buf_);
     }
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if ((void *)__f.__f_ == &__f.__buf_)
     {
-        __f.__f_->__clone((__base*)&__buf_);
+        __f.__f_->__clone(__as_base(&__buf_));
         __f.__f_->destroy();
         __f.__f_ = __f_;
-        __f_ = (__base*)&__buf_;
+        __f_ = __as_base(&__buf_);
     }
     else
         _VSTD::swap(__f_, __f.__f_);
 }
 
 template<class _Rp, class ..._ArgTypes>
 _Rp
 function<_Rp(_ArgTypes...)>::operator()(_ArgTypes... __arg) const