Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Jonathan Wakely via Gcc-patches]

On Wed, 22 Sept 2021 at 18:56, Antony Polukhin  wrote:
>
> ср, 22 сент. 2021 г. в 20:44, Jonathan Wakely :
> >
> > 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.

Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Antony Polukhin via Gcc-patches]

ср, 22 сент. 2021 г. в 20:44, Jonathan Wakely :
>
> 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

-- 
Best regards,
Antony Polukhin

Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Ville Voutilainen via Gcc-patches]

On Wed, 22 Sept 2021 at 20:49, Antony Polukhin  wrote:
>
> ср, 22 сент. 2021 г. в 20:23, Ville Voutilainen :
> >
> > On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++
> >  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::pointer is implicitly convertible to pointer".
> There's already a static assert that T in unique_ptr 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.

Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Antony Polukhin via Gcc-patches]

ср, 22 сент. 2021 г. в 20:23, Ville Voutilainen :
>
> On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++
>  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::pointer is implicitly convertible to pointer".
There's already a static assert that T in unique_ptr 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.

-- 
Best regards,
Antony Polukhin

Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Jonathan Wakely via Gcc-patches]

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.

Re: [PATCH] assert that deleting by pointer to base in unique_ptr does not cause UB

[Ville Voutilainen via Gcc-patches]

On Wed, 22 Sept 2021 at 20:09, Antony Polukhin via Libstdc++
 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.

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

[Antony Polukhin via Gcc-patches]

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.


-- 
Best regards,
Antony Polukhin
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::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::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 000..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
+// .
+
+#include 
+
+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 upB;
+
+  std::unique_ptr cA;
+  cA = std::move(upB);
+
+  std::unique_ptr vA;
+  vA = std::move(upB);
+
+  std::unique_ptr cvA;
+  cvA = std::move(upB);
+}
+
+void test02()
+{
+  std::unique_ptr upC;
+
+  std::unique_ptr cA{std::move(upC)};  // { dg-error "required from 
here" }
+  cA = std::move(upC);  // { dg-error "required from here" }
+
+  std::unique_ptr vA{std::move(upC)};  // { dg-error "required 
from here" }
+  vA = std::move(upC);  // { dg-error "required from here" }
+
+  std::unique_ptr cvA{std::move(upC)};  // { dg-error 
"required from here" }
+  cvA = std::move(upC);  // { dg-error "required from here" }
+}
+
+void test03()
+{
+  std::unique_ptr upB;
+
+  std::unique_ptr cA;
+  cA = std::move(upB);
+
+  std::unique_ptr vA;
+  vA = std::move(upB);
+
+  std::unique_ptr cvA;
+  cvA = std::move(upB);
+}
+
+void test04()
+{
+  std::unique_ptr upC;
+
+  std::unique_ptr cA{std::move(upC)};  // { dg-error "required from 
here" }
+  cA = std::move(upC);  // { dg-error "required from here" }
+
+  std::unique_ptr vA{std::move(upC)};  // { dg-error "required 
from here" }
+  vA = std::move(upC);  // { dg-error "required from here" }
+
+  std::unique_ptr cvA{std::move(upC)};  // { dg-error 
"required from here" }
+  cvA = std::move(upC);  // { dg-error "required from here" }
+}