From e4d6d860b6d2cc510401b298c655283ffbefb072 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 14 Jan 2021 10:29:23 -0800 Subject: [PATCH] Fixing bugs discovered by ASAN. The code is now ASAN, MSAN, UBSAN clean. --- include/pybind11/classh.h | 32 ++++++++++++++++---------------- tests/test_classh_wip.cpp | 14 +++++++++----- tests/test_classh_wip.py | 9 +-------- 3 files changed, 26 insertions(+), 29 deletions(-) diff --git a/include/pybind11/classh.h b/include/pybind11/classh.h index 77a9a9328..96510077c 100644 --- a/include/pybind11/classh.h +++ b/include/pybind11/classh.h @@ -266,37 +266,37 @@ public: } private: - /// Initialize holder object, variant 1: object derives from enable_shared_from_this template - static void init_holder(detail::value_and_holder &/*v_h*/, - const holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { + static void init_holder(bool /*owned*/, detail::value_and_holder &/*v_h*/, + holder_type * /* unused */, const std::enable_shared_from_this * /* dummy */) { throw std::runtime_error("Not implemented: classh::init_holder enable_shared_from_this."); } - /// Initialize holder object, variant 2: try to construct from existing holder object, if possible - static void init_holder(detail::value_and_holder &v_h, - const holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this) */) { + static void init_holder(bool owned, detail::value_and_holder &v_h, + holder_type *holder_ptr, const void * /* dummy -- not enable_shared_from_this) */) { if (holder_ptr) { - new (std::addressof(v_h.holder())) holder_type(std::move(*holder_ptr)); - v_h.set_holder_constructed(); - } else { // Was: if (inst->owned || detail::always_construct_holder::value) new (std::addressof(v_h.holder())) holder_type( - std::move(holder_type::from_raw_ptr_take_ownership(v_h.value_ptr()))); - v_h.set_holder_constructed(); + std::move(*holder_ptr)); + } else if (owned) { + new (std::addressof(v_h.holder())) holder_type( + holder_type::from_raw_ptr_take_ownership(v_h.value_ptr())); } + else { + new (std::addressof(v_h.holder())) holder_type( + holder_type::from_raw_ptr_unowned(v_h.value_ptr())); + } + v_h.set_holder_constructed(); } - /// Performs instance initialization including constructing a holder and registering the known - /// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes an - /// optional pointer to an existing holder to use; if not specified and the instance is - /// `.owned`, a new holder will be constructed to manage the value pointer. static void init_instance(detail::instance *inst, const void *holder_ptr) { auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); if (!v_h.instance_registered()) { register_instance(inst, v_h.value_ptr(), v_h.type); v_h.set_instance_registered(); } - init_holder(v_h, static_cast(holder_ptr), v_h.value_ptr()); + // Need for const_cast is a consequence of the type_info::init_instance type: + // void (*init_instance)(instance *, const void *); + init_holder(inst->owned, v_h, static_cast(const_cast(holder_ptr)), v_h.value_ptr()); } /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. diff --git a/tests/test_classh_wip.cpp b/tests/test_classh_wip.cpp index c864673d5..860736c27 100644 --- a/tests/test_classh_wip.cpp +++ b/tests/test_classh_wip.cpp @@ -15,11 +15,11 @@ struct mpty { // clang-format off mpty rtrn_mpty_valu() { mpty obj{"rtrn_valu"}; return obj; } -mpty&& rtrn_mpty_rref() { mpty obj{"rtrn_rref"}; return std::move(obj); } +mpty&& rtrn_mpty_rref() { static mpty obj; obj.mtxt = "rtrn_rref"; return std::move(obj); } mpty const& rtrn_mpty_cref() { static mpty obj; obj.mtxt = "rtrn_cref"; return obj; } mpty& rtrn_mpty_mref() { static mpty obj; obj.mtxt = "rtrn_mref"; return obj; } -mpty const* rtrn_mpty_cptr() { static mpty obj; obj.mtxt = "rtrn_cptr"; return &obj; } -mpty* rtrn_mpty_mptr() { static mpty obj; obj.mtxt = "rtrn_mptr"; return &obj; } +mpty const* rtrn_mpty_cptr() { return new mpty{"rtrn_cptr"}; } +mpty* rtrn_mpty_mptr() { return new mpty{"rtrn_mptr"}; } std::string pass_mpty_valu(mpty obj) { return "pass_valu:" + obj.mtxt; } std::string pass_mpty_rref(mpty&& obj) { return "pass_rref:" + obj.mtxt; } @@ -315,7 +315,9 @@ struct type_caster> : smart_holder_type_caster_load object inst = reinterpret_steal(make_new_instance(tinfo->type)); instance *inst_raw_ptr = reinterpret_cast(inst.ptr()); - inst_raw_ptr->owned = false; // Not actually used. + inst_raw_ptr->owned = true; + void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); + valueptr = src_raw_void_ptr; auto smhldr = pybindit::memory::smart_holder::from_shared_ptr(src); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); @@ -380,7 +382,9 @@ struct type_caster> : smart_holder_type_caster_load object inst = reinterpret_steal(make_new_instance(tinfo->type)); instance *inst_raw_ptr = reinterpret_cast(inst.ptr()); - inst_raw_ptr->owned = false; // Not actually used. + inst_raw_ptr->owned = true; + void *&valueptr = values_and_holders(inst_raw_ptr).begin()->value_ptr(); + valueptr = src_raw_void_ptr; auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src)); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); diff --git a/tests/test_classh_wip.py b/tests/test_classh_wip.py index f17b0aecb..f816af06f 100644 --- a/tests/test_classh_wip.py +++ b/tests/test_classh_wip.py @@ -15,20 +15,13 @@ def test_mpty_constructors(): def test_cast(): assert m.get_mtxt(m.rtrn_mpty_valu()) == "rtrn_valu" - # rtrn_rref exercised separately. + assert m.get_mtxt(m.rtrn_mpty_rref()) == "rtrn_rref" assert m.get_mtxt(m.rtrn_mpty_cref()) == "rtrn_cref" assert m.get_mtxt(m.rtrn_mpty_mref()) == "rtrn_mref" assert m.get_mtxt(m.rtrn_mpty_cptr()) == "rtrn_cptr" assert m.get_mtxt(m.rtrn_mpty_mptr()) == "rtrn_mptr" -def test_cast_rref(): - e = m.rtrn_mpty_rref() - assert e.__class__.__name__ == "mpty" - with pytest.raises(RuntimeError): - m.get_mtxt(e) # E.g. basic_string::_M_construct null not valid - - def test_load(): assert m.pass_mpty_valu(m.mpty("Valu")) == "pass_valu:Valu" assert m.pass_mpty_rref(m.mpty("Rref")) == "pass_rref:Rref"