Skip to content

Commit

Permalink
[libc++] fix shared_ptr's incorrect constraints
Browse files Browse the repository at this point in the history 
Fix several bugs:
1. https://llvm.org/PR60258
   The conversion constructors' constraint `__compatible_with` incorrectly allow array types conversion to scalar types
2. https://llvm.org/PR53368
   The constructor that takes `unique_ptr` are not suffiently constrained.
3. The constructors that take raw pointers incorretly use `__compatible_with`. They have different constraints

Differential Revision: https://reviews.llvm.org/D143346
  • Loading branch information
@huixie90
huixie90 committed Feb 11, 2023
1 parent 23cb32c commit a38a465
Show file tree
Hide file tree
Showing 13 changed files with 290 additions and 120 deletions.
83 changes: 64 additions & 19 deletions libcxx/include/__memory/shared_ptr.h
View file
Expand Up @@ -367,13 +367,57 @@ class _LIBCPP_TEMPLATE_VIS allocator<__shared_ptr_dummy_rebind_allocator_type>

template<class _Tp> class _LIBCPP_TEMPLATE_VIS enable_shared_from_this;

template<class _Tp, class _Up>
// http://eel.is/c++draft/util.sharedptr#util.smartptr.shared.general-6
// A pointer type Y* is said to be compatible with a pointer type T*
// when either Y* is convertible to T* or Y is U[N] and T is cv U[].
#if _LIBCPP_STD_VER >= 17
template <class _Yp, class _Tp>
struct __bounded_convertible_to_unbounded : false_type {};

template <class _Up, std::size_t _Np, class _Tp>
struct __bounded_convertible_to_unbounded<_Up[_Np], _Tp>
: is_same<__remove_cv_t<_Tp>, _Up[]> {};

template <class _Yp, class _Tp>
struct __compatible_with
#if _LIBCPP_STD_VER > 14
: is_convertible<remove_extent_t<_Tp>*, remove_extent_t<_Up>*> {};
: _Or<
is_convertible<_Yp*, _Tp*>,
__bounded_convertible_to_unbounded<_Yp, _Tp>
> {};
#else
: is_convertible<_Tp*, _Up*> {};
#endif // _LIBCPP_STD_VER > 14
template <class _Yp, class _Tp>
struct __compatible_with
: is_convertible<_Yp*, _Tp*> {};
#endif // _LIBCPP_STD_VER >= 17

// Constructors that take raw pointers have a different set of "compatible" constraints
// http://eel.is/c++draft/util.sharedptr#util.smartptr.shared.const-9.1
// - If T is an array type, then either T is U[N] and Y(*)[N] is convertible to T*,
// or T is U[] and Y(*)[] is convertible to T*.
// - If T is not an array type, then Y* is convertible to T*.
#if _LIBCPP_STD_VER >= 17
template <class _Yp, class _Tp, class = void>
struct __raw_pointer_compatible_with : _And<
_Not<is_array<_Tp>>,
is_convertible<_Yp*, _Tp*>
> {};

template <class _Yp, class _Up, std::size_t _Np>
struct __raw_pointer_compatible_with<_Yp, _Up[_Np], __enable_if_t<
is_convertible<_Yp(*)[_Np], _Up(*)[_Np]>::value> >
: true_type {};

template <class _Yp, class _Up>
struct __raw_pointer_compatible_with<_Yp, _Up[], __enable_if_t<
is_convertible<_Yp(*)[], _Up(*)[]>::value> >
: true_type {};

#else
template <class _Yp, class _Tp>
struct __raw_pointer_compatible_with
: is_convertible<_Yp*, _Tp*> {};
#endif // _LIBCPP_STD_VER >= 17


template <class _Ptr, class = void>
struct __is_deletable : false_type { };
Expand All @@ -395,12 +439,12 @@ static false_type __well_formed_deleter_test(...);
template <class _Dp, class _Pt>
struct __well_formed_deleter : decltype(std::__well_formed_deleter_test<_Dp, _Pt>(0)) {};

template<class _Dp, class _Tp, class _Yp>
template<class _Dp, class _Yp, class _Tp>
struct __shared_ptr_deleter_ctor_reqs
{
static const bool value = __compatible_with<_Tp, _Yp>::value &&
static const bool value = __raw_pointer_compatible_with<_Yp, _Tp>::value &&
is_move_constructible<_Dp>::value &&
__well_formed_deleter<_Dp, _Tp*>::value;
__well_formed_deleter<_Dp, _Yp*>::value;
};

#if defined(_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI)
Expand Down Expand Up @@ -439,7 +483,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr

template<class _Yp, class = __enable_if_t<
_And<
__compatible_with<_Yp, _Tp>
__raw_pointer_compatible_with<_Yp, _Tp>
// In C++03 we get errors when trying to do SFINAE with the
// delete operator, so we always pretend that it's deletable.
// The same happens on GCC.
Expand All @@ -457,7 +501,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
__enable_weak_this(__p, __p);
}

template<class _Yp, class _Dp, class = __enable_if_t<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value> >
template<class _Yp, class _Dp, class = __enable_if_t<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, _Tp>::value> >
_LIBCPP_HIDE_FROM_ABI
shared_ptr(_Yp* __p, _Dp __d)
: __ptr_(__p)
Expand All @@ -484,7 +528,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
#endif // _LIBCPP_NO_EXCEPTIONS
}

template<class _Yp, class _Dp, class _Alloc, class = __enable_if_t<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, element_type>::value> >
template<class _Yp, class _Dp, class _Alloc, class = __enable_if_t<__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, _Tp>::value> >
_LIBCPP_HIDE_FROM_ABI
shared_ptr(_Yp* __p, _Dp __d, _Alloc __a)
: __ptr_(__p)
Expand Down Expand Up @@ -646,6 +690,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr

template <class _Yp, class _Dp, class = __enable_if_t<
!is_lvalue_reference<_Dp>::value &&
__compatible_with<_Yp, _Tp>::value &&
is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>::value
> >
_LIBCPP_HIDE_FROM_ABI
Expand All @@ -668,6 +713,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr

template <class _Yp, class _Dp, class = void, class = __enable_if_t<
is_lvalue_reference<_Dp>::value &&
__compatible_with<_Yp, _Tp>::value &&
is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>::value
> >
_LIBCPP_HIDE_FROM_ABI
Expand Down Expand Up @@ -740,9 +786,10 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
}
#endif

template <class _Yp, class _Dp, class = __enable_if_t<
is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>::value
> >
template <class _Yp, class _Dp, class = __enable_if_t<_And<
__compatible_with<_Yp, _Tp>,
is_convertible<typename unique_ptr<_Yp, _Dp>::pointer, element_type*>
>::value> >
_LIBCPP_HIDE_FROM_ABI
shared_ptr<_Tp>& operator=(unique_ptr<_Yp, _Dp>&& __r)
{
Expand All @@ -764,7 +811,7 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
}

template<class _Yp, class = __enable_if_t<
__compatible_with<_Yp, _Tp>::value
__raw_pointer_compatible_with<_Yp, _Tp>::value
> >
_LIBCPP_HIDE_FROM_ABI
void reset(_Yp* __p)
Expand All @@ -773,17 +820,15 @@ class _LIBCPP_SHARED_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS shared_ptr
}

template<class _Yp, class _Dp, class = __enable_if_t<
__compatible_with<_Yp, _Tp>::value
> >
__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, _Tp>::value> >
_LIBCPP_HIDE_FROM_ABI
void reset(_Yp* __p, _Dp __d)
{
shared_ptr(__p, __d).swap(*this);
}

template<class _Yp, class _Dp, class _Alloc, class = __enable_if_t<
__compatible_with<_Yp, _Tp>::value
> >
__shared_ptr_deleter_ctor_reqs<_Dp, _Yp, _Tp>::value> >
_LIBCPP_HIDE_FROM_ABI
void reset(_Yp* __p, _Dp __d, _Alloc __a)
{
Expand Down
60 changes: 10 additions & 50 deletions ...mory/util.smartptr/util.smartptr.shared/util.smartptr.shared.assign/unique_ptr_Y.pass.cpp
View file
Expand Up @@ -55,6 +55,16 @@ struct StatefulArrayDeleter {
}
};

// https://llvm.org/PR53368
// Bogus unique_ptr-to-shared_ptr conversions should be forbidden
#if TEST_STD_VER >= 17
static_assert( std::is_assignable<std::shared_ptr<A>&, std::unique_ptr<A>&&>::value, "");
static_assert( std::is_assignable<std::shared_ptr<A[]>&, std::unique_ptr<A[]>&&>::value, "");
static_assert(!std::is_assignable<std::shared_ptr<A>&, std::unique_ptr<A[]>&&>::value, "");
static_assert(!std::is_assignable<std::shared_ptr<B[]>&, std::unique_ptr<A[]>&&>::value, "");
static_assert(!std::is_assignable<std::shared_ptr<B>&, std::unique_ptr<A[]>&&>::value, "");
#endif

int main(int, char**)
{
{
Expand Down Expand Up @@ -126,40 +136,6 @@ int main(int, char**)
assert(B::count == 0);
assert(A::count == 0);

#ifdef _LIBCPP_VERSION // https://llvm.org/PR53368
{
std::unique_ptr<A[]> ptr(new A[8]);
A* raw_ptr = ptr.get();
std::shared_ptr<B> p;
p = std::move(ptr);
assert(A::count == 8);
assert(B::count == 8);
assert(p.use_count() == 1);
assert(p.get() == raw_ptr);
assert(ptr.get() == 0);
}
assert(A::count == 0);
assert(B::count == 0);

{
std::unique_ptr<A[]> ptr(new A[8]);
A* raw_ptr = ptr.get();
std::shared_ptr<A> p;
p = std::move(ptr);
assert(A::count == 8);
assert(p.use_count() == 1);
assert(p.get() == raw_ptr);
assert(ptr.get() == 0);
}
assert(A::count == 0);

{
std::unique_ptr<int[]> ptr(new int[8]);
std::shared_ptr<int> p;
p = std::move(ptr);
}
#endif // _LIBCPP_VERSION

#if TEST_STD_VER > 14
{
StatefulArrayDeleter<A> d;
Expand All @@ -172,22 +148,6 @@ int main(int, char**)
assert(A::count == 0);
assert(B::count == 0);

#ifdef _LIBCPP_VERSION // https://llvm.org/PR53368
{
std::unique_ptr<A[]> ptr(new A[8]);
A* raw_ptr = ptr.get();
std::shared_ptr<B[]> p;
p = std::move(ptr);
assert(A::count == 8);
assert(B::count == 8);
assert(p.use_count() == 1);
assert(p.get() == raw_ptr);
assert(ptr.get() == 0);
}
assert(A::count == 0);
assert(B::count == 0);
#endif // _LIBCPP_VERSION

{
std::unique_ptr<A[]> ptr(new A[8]);
A* raw_ptr = ptr.get();
Expand Down
31 changes: 29 additions & 2 deletions ...ies/memory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer.pass.cpp
View file
Expand Up @@ -10,8 +10,9 @@

// template<class Y> explicit shared_ptr(Y* p);

#include <memory>
#include <cassert>
#include <memory>
#include <type_traits>

#include "test_macros.h"

Expand All @@ -26,6 +27,25 @@ struct A

int A::count = 0;

struct Derived : A {};

// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert( std::is_constructible<std::shared_ptr<int>, int*>::value, "");
static_assert( std::is_constructible<std::shared_ptr<A>, Derived*>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<A>, int*>::value, "");

#if TEST_STD_VER >= 17
static_assert( std::is_constructible<std::shared_ptr<int[]>, int*>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[]>::value, "");
static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5]>::value, "");
#endif

// Test explicit
static_assert(std::is_constructible<std::shared_ptr<int>, int*>::value, "");
static_assert(!std::is_convertible<int*, std::shared_ptr<int> >::value, "");

int main(int, char**)
{
{
Expand Down Expand Up @@ -71,11 +91,18 @@ int main(int, char**)
}

{
assert(A::count == 0);
assert(A::count == 0);
std::shared_ptr<const A[]> pA(new A[8]);
assert(pA.use_count() == 1);
assert(A::count == 8);
}

{
assert(A::count == 0);
std::shared_ptr<A> pA(new Derived);
assert(pA.use_count() == 1);
assert(A::count == 1);
}
#endif

return 0;
Expand Down
16 changes: 16 additions & 0 deletions ...ry/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter.pass.cpp
View file
Expand Up @@ -60,6 +60,22 @@ class MoveDeleter
void operator()(T *ptr) { delete ptr; }
};

// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert( std::is_constructible<std::shared_ptr<int>, int*, test_deleter<int> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int>, int*, bad_deleter>::value, "");
static_assert( std::is_constructible<std::shared_ptr<Base>, Derived*, test_deleter<Base> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<A>, int*, test_deleter<A> >::value, "");

#if TEST_STD_VER >= 17
static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int*, bad_deleter>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int>>::value, "");
static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int*, bad_deleter>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int>>::value, "");
#endif

int main(int, char**)
{
{
Expand Down
17 changes: 17 additions & 0 deletions ...artptr/util.smartptr.shared/util.smartptr.shared.const/pointer_deleter_allocator.pass.cpp
View file
Expand Up @@ -60,6 +60,23 @@ class MoveDeleter
void operator()(T *ptr) { delete ptr; }
};

// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert( std::is_constructible<std::shared_ptr<int>, int*, test_deleter<int>, test_allocator<int> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int>, int*, bad_deleter, test_allocator<int> >::value, "");
static_assert( std::is_constructible<std::shared_ptr<Base>, Derived*, test_deleter<Base>, test_allocator<Base> >::value, "");
static_assert(!std::is_constructible<std::shared_ptr<A>, int*, test_deleter<A>, test_allocator<A> >::value, "");

#if TEST_STD_VER >= 17
static_assert( std::is_constructible<std::shared_ptr<int[]>, int*, test_deleter<int>, test_allocator<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int*, bad_deleter, test_allocator<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, int(*)[], test_deleter<int>, test_allocator<int>>::value, "");
static_assert( std::is_constructible<std::shared_ptr<int[5]>, int*, test_deleter<int>, test_allocator<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int*, bad_deleter, test_allocator<int>>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, int(*)[5], test_deleter<int>, test_allocator<int>>::value, "");
#endif


int main(int, char**)
{
{
Expand Down
17 changes: 17 additions & 0 deletions ...emory/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_Y.pass.cpp
View file
Expand Up @@ -74,6 +74,23 @@ class private_delete_arr_op
}
};

// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert(!std::is_constructible<std::shared_ptr<int>, const std::shared_ptr<long>&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<B>, const std::shared_ptr<A>&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<const A>, const std::shared_ptr<A>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<A>, const std::shared_ptr<const A>&>::value, "");

#if TEST_STD_VER >= 17
static_assert(!std::is_constructible<std::shared_ptr<int>, const std::shared_ptr<int[]>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int>, const std::shared_ptr<int[5]>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, const std::shared_ptr<int>&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<int[]>, const std::shared_ptr<int[5]>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, const std::shared_ptr<int>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, const std::shared_ptr<int[]>&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[7]>, const std::shared_ptr<int[5]>&>::value, "");
#endif

int main(int, char**)
{
static_assert(( std::is_convertible<std::shared_ptr<A>, std::shared_ptr<B> >::value), "");
Expand Down
17 changes: 17 additions & 0 deletions ...ry/util.smartptr/util.smartptr.shared/util.smartptr.shared.const/shared_ptr_Y_rv.pass.cpp
View file
Expand Up @@ -55,6 +55,23 @@ struct C

int C::count = 0;

// https://llvm.org/PR60258
// Invalid constructor SFINAE for std::shared_ptr's array ctors
static_assert(!std::is_constructible<std::shared_ptr<int>, std::shared_ptr<long>&&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<B>, std::shared_ptr<A>&&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<const A>, std::shared_ptr<A>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<A>, std::shared_ptr<const A>&&>::value, "");

#if TEST_STD_VER >= 17
static_assert(!std::is_constructible<std::shared_ptr<int>, std::shared_ptr<int[]>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int>, std::shared_ptr<int[5]>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[]>, std::shared_ptr<int>&&>::value, "");
static_assert( std::is_constructible<std::shared_ptr<int[]>, std::shared_ptr<int[5]>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::shared_ptr<int>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[5]>, std::shared_ptr<int[]>&&>::value, "");
static_assert(!std::is_constructible<std::shared_ptr<int[7]>, std::shared_ptr<int[5]>&&>::value, "");
#endif

int main(int, char**)
{
static_assert(( std::is_convertible<std::shared_ptr<A>, std::shared_ptr<B> >::value), "");
Expand Down

0 comments on commit a38a465

Please sign in to comment.