From 53be81931f35313f70affc9826bdfed9820cce2c Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 9 Nov 2017 21:44:05 -0400 Subject: [PATCH 1/5] Don't attempt to constructor an impossible shared_ptr Fixes #1178 It's possible for a non-destructible base class to be declared; such a holder *can* be obtained via `std::enable_shared_from_this`, but a shared_ptr cannot be constructed from a returned pointer. This commit puts the holder constructor behind a `std::is_destructible` check, giving a runtime failure if we are ever given such a pointer without a valid `shared_from_this()` shared pointer. --- include/pybind11/pybind11.h | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b489bb2493..ae448e406f 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1262,6 +1262,24 @@ class class_ : public detail::generic_type { } private: + template ::value, int> = 0> + static void init_shared_holder_from_pointer(detail::value_and_holder &v_h) { + new (&v_h.holder()) holder_type(v_h.value_ptr()); + v_h.set_holder_constructed(); + } + + template ::value, int> = 0> + static void init_shared_holder_from_pointer(detail::value_and_holder &) { + pybind11_fail("Unable to construct C++ holder" +#if defined(NDEBUG) + " (compile in debug mode for type details)" +#else + ": cannot construct a `" + type_id() + "' holder around a non-destructible `" + + type_id() + "' pointer" +#endif + ); + } + /// Initialize holder object, variant 1: object derives from enable_shared_from_this template static void init_holder(detail::instance *inst, detail::value_and_holder &v_h, @@ -1276,8 +1294,7 @@ class class_ : public detail::generic_type { } catch (const std::bad_weak_ptr &) {} if (!v_h.holder_constructed() && inst->owned) { - new (&v_h.holder()) holder_type(v_h.value_ptr()); - v_h.set_holder_constructed(); + init_shared_holder_from_pointer(v_h); } } From 2a0c6687a89f104596476e456b3beb1227ea73fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B6bke=20Geenen?= Date: Mon, 13 Jan 2020 14:21:18 +0100 Subject: [PATCH 2/5] Use init_shared_holder_from_pointer() also for variant 2: 'try to construct from existing holder object, if possible' --- include/pybind11/pybind11.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 139f135e63..362ebc5194 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1351,8 +1351,7 @@ class class_ : public detail::generic_type { init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible()); v_h.set_holder_constructed(); } else if (inst->owned || detail::always_construct_holder::value) { - new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); - v_h.set_holder_constructed(); + init_shared_holder_from_pointer(v_h); } } From 2b38c9e089f7d4545c5d7fd86d31914be7290aa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B6bke=20Geenen?= Date: Mon, 13 Jan 2020 17:27:32 +0100 Subject: [PATCH 3/5] Only check destructibility for shared_ptr holder, non-destructibility with another holder type should still be a compile error. Manually caught and converted exception to Python exception, otherwise it escapes and std::terminate() is called. --- include/pybind11/pybind11.h | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 362ebc5194..c79b5259e3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -778,7 +778,12 @@ class cpp_function : public function { } else { if (overloads->is_constructor && !self_value_and_holder.holder_constructed()) { auto *pi = reinterpret_cast(parent.ptr()); - self_value_and_holder.type->init_instance(pi, nullptr); + try { + self_value_and_holder.type->init_instance(pi, nullptr); + } catch (const std::runtime_error &e) { + PyErr_SetString(PyExc_RuntimeError, e.what()); + return nullptr; + } } return result.ptr(); } @@ -1298,17 +1303,23 @@ class class_ : public detail::generic_type { } private: - template ::value, int> = 0> + /// Initialize holder from pointer when the underlying object is destructible (public destructor) + template ::value || + std::is_destructible::value, int> = 0> static void init_shared_holder_from_pointer(detail::value_and_holder &v_h) { - new (&v_h.holder()) holder_type(v_h.value_ptr()); + new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); v_h.set_holder_constructed(); } - template ::value, int> = 0> + /// Do *not* initialize holder from pointer when the underlying object is *not* destructible + template ::value && + !std::is_destructible::value, int> = 0> static void init_shared_holder_from_pointer(detail::value_and_holder &) { pybind11_fail("Unable to construct C++ holder" #if defined(NDEBUG) - " (compile in debug mode for type details)" + " (compile in debug mode for type information)" #else ": cannot construct a `" + type_id() + "' holder around a non-destructible `" + type_id() + "' pointer" From 99070f4037af0b3b8ddaa5744891a1d0fcd3c060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B6bke=20Geenen?= Date: Mon, 13 Jan 2020 17:28:29 +0100 Subject: [PATCH 4/5] Added test cases for types with non-public destructor in a shared_ptr holder. --- tests/test_smart_ptr.cpp | 20 ++++++++++++++++++++ tests/test_smart_ptr.py | 14 ++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 87c9be8c2b..c9289e884d 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -212,6 +212,26 @@ TEST_SUBMODULE(smart_ptr, m) { py::class_(m, "MyObject4b") .def(py::init()); + // Object with non-public destructor in a shared_ptr + class MyObject4c { + public: + MyObject4c() { print_created(this); } + protected: + ~MyObject4c() { print_destroyed(this); } + }; + py::class_>(m, "MyObject4c") + .def(py::init()); + + // Object with non-public destructor in a shared_ptr with std::enable_shared_from_this<> + class MyObject4d : public std::enable_shared_from_this { + public: + MyObject4d() { print_created(this); } + protected: + ~MyObject4d() { print_destroyed(this); } + }; + py::class_>(m, "MyObject4d") + .def(py::init()); + // test_large_holder class MyObject5 { // managed by huge_unique_ptr public: diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index c6627043bd..5df2e4e0a0 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -136,6 +136,20 @@ def test_unique_deleter(): assert cstats4b.alive() == 0 # Should be deleted +def test_shared_ptr_non_public_destructor(): + """#1178: Shared Pointers and Protected Destructors""" + with pytest.raises(RuntimeError) as excinfo: + m.MyObject4c() + assert "Unable to construct C++ holder" in str(excinfo.value) + + +def test_shared_ptr_from_this_non_public_destructor(): + """#1178: Shared Pointers and Protected Destructors""" + with pytest.raises(RuntimeError) as excinfo: + m.MyObject4d() + assert "Unable to construct C++ holder" in str(excinfo.value) + + def test_large_holder(): o = m.MyObject5(5) assert o.value == 5 From 29787f6a312c462a3df234dcf51d0c49dc33a72b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B6bke=20Geenen?= Date: Tue, 14 Jan 2020 11:35:40 +0100 Subject: [PATCH 5/5] Fix: MSVC does not like enable_if_t = 0. Known issue: MSVC incorrectly rejects "enable_if_t = 0" SFINAE https://developercommunity.visualstudio.com/content/problem/848104/msvc-incorrectly-rejects-enable-if-t-0-sfinae.html --- include/pybind11/pybind11.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c79b5259e3..080d4e06e8 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1304,19 +1304,23 @@ class class_ : public detail::generic_type { private: /// Initialize holder from pointer when the underlying object is destructible (public destructor) - template + detail::enable_if_t< !detail::is_instantiation::value || - std::is_destructible::value, int> = 0> - static void init_shared_holder_from_pointer(detail::value_and_holder &v_h) { + std::is_destructible::value + , void> + static init_shared_holder_from_pointer(detail::value_and_holder &v_h) { new (std::addressof(v_h.holder())) holder_type(v_h.value_ptr()); v_h.set_holder_constructed(); } /// Do *not* initialize holder from pointer when the underlying object is *not* destructible - template + detail::enable_if_t< detail::is_instantiation::value && - !std::is_destructible::value, int> = 0> - static void init_shared_holder_from_pointer(detail::value_and_holder &) { + !std::is_destructible::value + , void> + static init_shared_holder_from_pointer(detail::value_and_holder &) { pybind11_fail("Unable to construct C++ holder" #if defined(NDEBUG) " (compile in debug mode for type information)"