Message ID | CAKqmYPZK8RHFs7SdXZKwb_gCEwrKRyJvbcgDNLh=RU0hYebh-A@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | assert that deleting by pointer to base in unique_ptr does not cause UB | expand |
On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > std::unique_ptr allows construction from std::unique_ptr of derived > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > std::default_delete is used with std::unique_ptr, then after such > construction a delete is called on a pointer to base. According to > [expr.delete] calling a delete on a non similar object without a > virtual destructor is an undefined behavior. > > This patch turns that undefined behavior into static assertions inside > std::unique_ptr. I don't understand the sizeof(_Tp) == sizeof(_Up) part in the static_assert. I fail to see how a same-size check suggests that the types are similar enough that a delete-expression works.
On Wed, 22 Sept 2021 at 18:09, Antony Polukhin wrote: > > std::unique_ptr allows construction from std::unique_ptr of derived > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > std::default_delete is used with std::unique_ptr, then after such > construction a delete is called on a pointer to base. According to > [expr.delete] calling a delete on a non similar object without a > virtual destructor is an undefined behavior. > > This patch turns that undefined behavior into static assertions inside > std::unique_ptr. The undefined behaviour only happens if the destructor is actually reached at runtime, but won't these static assertions make it ill-formed to instantiate these members, even if the UB never happens? For example, if you ensure that release() is called before destruction, the undefined delete never happens.
ср, 22 сент. 2021 г. в 20:23, Ville Voutilainen <ville.voutilainen@gmail.com>: > > On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: > > > > std::unique_ptr allows construction from std::unique_ptr of derived > > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > > std::default_delete is used with std::unique_ptr, then after such > > construction a delete is called on a pointer to base. According to > > [expr.delete] calling a delete on a non similar object without a > > virtual destructor is an undefined behavior. > > > > This patch turns that undefined behavior into static assertions inside > > std::unique_ptr. > > I don't understand the sizeof(_Tp) == sizeof(_Up) part in the > static_assert. I fail to see how > a same-size check suggests that the types are similar enough that a > delete-expression works. I used the following logic: [unique.ptr.single.*] sections have the constraint that "unique_ptr<U, E>::pointer is implicitly convertible to pointer". There's already a static assert that T in unique_ptr<T> is not void, so U either has to be the same type T, or a type derived from T. If a derived type adds members, then size changes and types are not similar as the decompositions won't have the qualification-decompositions with the same n.
On Wed, 22 Sept 2021 at 20:49, Antony Polukhin <antoshkka@gmail.com> wrote: > > ср, 22 сент. 2021 г. в 20:23, Ville Voutilainen <ville.voutilainen@gmail.com>: > > > > On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++ > > <libstdc++@gcc.gnu.org> wrote: > > > > > > std::unique_ptr allows construction from std::unique_ptr of derived > > > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > > > std::default_delete is used with std::unique_ptr, then after such > > > construction a delete is called on a pointer to base. According to > > > [expr.delete] calling a delete on a non similar object without a > > > virtual destructor is an undefined behavior. > > > > > > This patch turns that undefined behavior into static assertions inside > > > std::unique_ptr. > > > > I don't understand the sizeof(_Tp) == sizeof(_Up) part in the > > static_assert. I fail to see how > > a same-size check suggests that the types are similar enough that a > > delete-expression works. > > I used the following logic: > [unique.ptr.single.*] sections have the constraint that > "unique_ptr<U, E>::pointer is implicitly convertible to pointer". > There's already a static assert that T in unique_ptr<T> is not void, > so U either has to be the same type T, or a type derived from T. If a > derived type adds members, then size changes and types are not similar > as the decompositions won't have the qualification-decompositions with > the same n. Right, but the delete-expression on a non-polymorphic type where the static type and the dynamic type are different is UB regardless of whether the derived type adds members.
ср, 22 сент. 2021 г. в 20:44, Jonathan Wakely <jwakely.gcc@gmail.com>: > > On Wed, 22 Sept 2021 at 18:09, Antony Polukhin wrote: > > > > std::unique_ptr allows construction from std::unique_ptr of derived > > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > > std::default_delete is used with std::unique_ptr, then after such > > construction a delete is called on a pointer to base. According to > > [expr.delete] calling a delete on a non similar object without a > > virtual destructor is an undefined behavior. > > > > This patch turns that undefined behavior into static assertions inside > > std::unique_ptr. > > The undefined behaviour only happens if the destructor is actually > reached at runtime, but won't these static assertions make it > ill-formed to instantiate these members, even if the UB never happens? > > For example, if you ensure that release() is called before > destruction, the undefined delete never happens. Ugh... I've missed that use case. Patch is just wrong, discard it
On Wed, 22 Sept 2021 at 18:56, Antony Polukhin <antoshkka@gmail.com> wrote: > > ср, 22 сент. 2021 г. в 20:44, Jonathan Wakely <jwakely.gcc@gmail.com>: > > > > On Wed, 22 Sept 2021 at 18:09, Antony Polukhin wrote: > > > > > > std::unique_ptr allows construction from std::unique_ptr of derived > > > type as per [unique.ptr.single.asgn] and [unique.ptr.single.ctor]. If > > > std::default_delete is used with std::unique_ptr, then after such > > > construction a delete is called on a pointer to base. According to > > > [expr.delete] calling a delete on a non similar object without a > > > virtual destructor is an undefined behavior. > > > > > > This patch turns that undefined behavior into static assertions inside > > > std::unique_ptr. > > > > The undefined behaviour only happens if the destructor is actually > > reached at runtime, but won't these static assertions make it > > ill-formed to instantiate these members, even if the UB never happens? > > > > For example, if you ensure that release() is called before > > destruction, the undefined delete never happens. > > Ugh... I've missed that use case. Patch is just wrong, discard it It's a horrible (and probably unrealistic) use case, but we're required to accept it. I should a test case to the testsuite, just to make sure we continue to accept it without errors.
diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h index 6e55375..53a68f5 100644 --- a/libstdc++-v3/include/bits/unique_ptr.h +++ b/libstdc++-v3/include/bits/unique_ptr.h @@ -339,7 +339,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION is_convertible<_Ep, _Dp>>::type>> unique_ptr(unique_ptr<_Up, _Ep>&& __u) noexcept : _M_t(__u.release(), std::forward<_Ep>(__u.get_deleter())) - { } + { + static_assert(!is_same<_Dp, default_delete<_Tp>>::value + || has_virtual_destructor<typename remove_cv<_Tp>::type>::value + || sizeof(_Tp) == sizeof(_Up), + "type of pointer owned by __u must be similar to the type of pointer " + "owned by this object or the latter must have a virtual destructor"); + } #if _GLIBCXX_USE_DEPRECATED #pragma GCC diagnostic push @@ -385,6 +391,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION unique_ptr&>::type operator=(unique_ptr<_Up, _Ep>&& __u) noexcept { + static_assert(!is_same<_Dp, default_delete<_Tp>>::value + || has_virtual_destructor<typename remove_cv<_Tp>::type>::value + || sizeof(_Tp) == sizeof(_Up), + "type of pointer owned by __u must be similar to the type of pointer " + "owned by this object or the latter must have a virtual destructor"); + reset(__u.release()); get_deleter() = std::forward<_Ep>(__u.get_deleter()); return *this; diff --git a/libstdc++-v3/testsuite/20_util/unique_ptr/assign/slicing_neg.cc b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/slicing_neg.cc new file mode 100644 index 0000000..e93483a --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/unique_ptr/assign/slicing_neg.cc @@ -0,0 +1,86 @@ +// { dg-do compile { target c++11 } } +// { dg-prune-output "virtual destructor" } + +// Copyright (C) 2021 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +#include <memory> + +struct A { }; +struct B : A { }; +struct C : B { int i; }; + +struct Ac { char c; }; +struct Bc : Ac { }; +struct Cc : Bc { short s; }; + + +void test01() +{ + std::unique_ptr<B> upB; + + std::unique_ptr<const A> cA; + cA = std::move(upB); + + std::unique_ptr<volatile A> vA; + vA = std::move(upB); + + std::unique_ptr<const volatile A> cvA; + cvA = std::move(upB); +} + +void test02() +{ + std::unique_ptr<C> upC; + + std::unique_ptr<const A> cA{std::move(upC)}; // { dg-error "required from here" } + cA = std::move(upC); // { dg-error "required from here" } + + std::unique_ptr<volatile A> vA{std::move(upC)}; // { dg-error "required from here" } + vA = std::move(upC); // { dg-error "required from here" } + + std::unique_ptr<const volatile A> cvA{std::move(upC)}; // { dg-error "required from here" } + cvA = std::move(upC); // { dg-error "required from here" } +} + +void test03() +{ + std::unique_ptr<Bc> upB; + + std::unique_ptr<const Ac> cA; + cA = std::move(upB); + + std::unique_ptr<volatile Ac> vA; + vA = std::move(upB); + + std::unique_ptr<const volatile Ac> cvA; + cvA = std::move(upB); +} + +void test04() +{ + std::unique_ptr<Cc> upC; + + std::unique_ptr<const Ac> cA{std::move(upC)}; // { dg-error "required from here" } + cA = std::move(upC); // { dg-error "required from here" } + + std::unique_ptr<volatile Ac> vA{std::move(upC)}; // { dg-error "required from here" } + vA = std::move(upC); // { dg-error "required from here" } + + std::unique_ptr<const volatile Ac> cvA{std::move(upC)}; // { dg-error "required from here" } + cvA = std::move(upC); // { dg-error "required from here" } +}