[libcxx-commits] [PATCH] D90536: [libcxx/test] Fix UB when deleting D through a pointer to B.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone created this revision.
Quuxplusone added a reviewer: ldionne.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
Quuxplusone requested review of this revision.

After I apply Lénárd Szolnoki's proposal to disable implicit conversion of `default_delete<D>` to `default_delete<B>`, this particular `std::optional` test begins to fail:

  /llvm-project/libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp:243:13: error: no viable overloaded '='
          opt = std::move(other);
          ~~~ ^ ~~~~~~~~~~~~~~~~
  /llvm-project/libcxx/include/optional:768:41: note: candidate function not viable: no known conversion from 'optional<unique_ptr<D, default_delete<D>>>' to 'optional<unique_ptr<B, default_delete<B>>>' for 1st argument
      _LIBCPP_INLINE_VISIBILITY optional& operator=(optional&&) = default;
                                          ^

I believe this is a true positive. The offending code in this test is circa line 243:

https://github.com/llvm/llvm-project/blob/master/libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp#L240-L248

  {
      optional<std::unique_ptr<B>> opt;
      optional<std::unique_ptr<D>> other(new D());
      opt = std::move(other);
      assert(static_cast<bool>(opt) == true);
      assert(static_cast<bool>(other) == true);
      assert(opt->get() != nullptr);
      assert(other->get() == nullptr);
  }

At the closing curly brace, we destroy `opt`, which deletes `opt->get()` (a `B*` which dynamically points to a `D` object), which has UB because http://eel.is/c++draft/expr.delete#3 — am I right?

Giving `B` a virtual dtor fixes the UB without affecting anything else about this test.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90536

Files:
  libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp


Index: libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp
===================================================================
--- libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp
+++ libcxx/test/std/utilities/optional/optional.object/optional.object.assign/optional_U.pass.cpp
@@ -51,7 +51,7 @@
     Y2& operator=(const int&) { return *this; }
 };
 
-class B {};
+struct B { virtual ~B() = default; };
 class D : public B {};
 
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90536.302104.patch
Type: text/x-patch
Size: 518 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20201031/8e1f2b27/attachment.bin>