Page MenuHomePhabricator

[libcxx/test] Fix UB when deleting D through a pointer to B.
ClosedPublic

Authored by Quuxplusone on Oct 31 2020, 11:06 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Quuxplusone created this revision.Oct 31 2020, 11:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2020, 11:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested review of this revision.Oct 31 2020, 11:06 AM
ldionne accepted this revision.Nov 2 2020, 9:43 AM

Your reasoning looks right to me. Thanks for fixing.

This revision is now accepted and ready to land.Nov 2 2020, 9:43 AM

Awesome. (As usual @ldionne you'll have to land this for me.)

This revision was landed with ongoing or failed builds.Nov 4 2020, 2:35 PM
This revision was automatically updated to reflect the committed changes.