diff mbox series

assert that deleting by pointer to base in unique_ptr does not cause UB

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

Commit Message

Antony Polukhin Sept. 22, 2021, 5:08 p.m. UTC 
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.

Changelog:
* include/bits/unique_ptr.h: Add static asserts that
deleting by pointer to base in unique_ptr does not cause UB
* testsuite/20_util/unique_ptr/assign/slicing_neg.cc:
New test.

Comments

Ville Voutilainen Sept. 22, 2021, 5:22 p.m. UTC | #1 
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.
Jonathan Wakely Sept. 22, 2021, 5:44 p.m. UTC | #2 
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.
Antony Polukhin Sept. 22, 2021, 5:49 p.m. UTC | #3 
ср, 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.
Ville Voutilainen Sept. 22, 2021, 5:53 p.m. UTC | #4 
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.
Antony Polukhin Sept. 22, 2021, 5:56 p.m. UTC | #5 
ср, 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
Jonathan Wakely Sept. 23, 2021, 10:21 a.m. UTC | #6 
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 mbox series

Patch

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" }
+}