diff --git a/include/boost/openmethod/policies/vptr_vector.hpp b/include/boost/openmethod/policies/vptr_vector.hpp index c1716ef9..493f695a 100644 --- a/include/boost/openmethod/policies/vptr_vector.hpp +++ b/include/boost/openmethod/policies/vptr_vector.hpp @@ -86,6 +86,10 @@ struct vptr_vector : vptr { //! function is called. Its result determines the size of the vector. //! The v-table pointers are copied into the vector. //! + //! The vector is rebuilt from scratch on every call, so a class that + //! was registered during a previous call, and is not registered any + //! more, does not keep its entry. + //! //! @tparam Context An @ref InitializeContext. //! @tparam Options... Zero or more option types. //! @param ctx A Context object. @@ -113,7 +117,13 @@ struct vptr_vector : vptr { ++size; } - st().vptrs.resize(size); + // Build a new vector and swap it in, rather than writing into the + // old one. Resizing keeps the elements that fit, so an index that + // belonged to a class that is no longer registered - one from a + // library that has since been unloaded, say - would keep pointing + // into the dispatch data the commit frees. Every slot the loop + // below does not write is null instead. + decltype(st().vptrs) new_vptrs(size); for (auto iter = ctx.classes_begin(); iter != ctx.classes_end(); ++iter) { @@ -128,12 +138,14 @@ struct vptr_vector : vptr { } if constexpr (Registry::has_indirect_vptr) { - st().vptrs[index] = iter->static_vptr(); + new_vptrs[index] = iter->static_vptr(); } else { - st().vptrs[index] = iter->vptr(); + new_vptrs[index] = iter->vptr(); } } } + + st().vptrs.swap(new_vptrs); } //! Returns a *reference* to a v-table pointer for an object. diff --git a/test/test_initialize_dropped_class.cpp b/test/test_initialize_dropped_class.cpp new file mode 100644 index 00000000..3ef579ad --- /dev/null +++ b/test/test_initialize_dropped_class.cpp @@ -0,0 +1,239 @@ +// Copyright (c) 2017-2026 Jean-Louis Leroy +// Distributed under the Boost Software License, Version 1.0. +// See accompanying file LICENSE_1_0.txt +// or copy at http://www.boost.org/LICENSE_1_0.txt) + +// A class stops being registered when the registrar that added it is +// destroyed - which is what happens to every class a shared library +// contributes when the library is unloaded. The next initialize() must not +// leave that class' v-table pointer behind: it points into the dispatch data +// the commit frees, so a later dispatch on the dropped class would read freed +// memory, and, after a dlclose, jump into unloaded code. +// +// The classes here are dropped by letting a block-scoped `use_classes` object +// die, which runs the same registrar destructor a library unload runs, in a +// single process. + +#include +#include +#include + +#define BOOST_TEST_MODULE initialize_dropped_class +#include + +#include "test_util.hpp" + +#include +#include +#include + +using boost::mp11::mp_list; +using namespace boost::openmethod; + +// Small, dense type ids: without a type_hash policy they are used as indices +// into vptr_vector's vector directly, so which slot a class occupies is exact +// rather than a function of the hash factors, which are re-drawn whenever the +// set of classes changes. +struct Animal { + explicit Animal(std::size_t type) : type(type) { + } + + virtual ~Animal() = default; + + static constexpr std::size_t static_type = 1; + std::size_t type; +}; + +struct Dog : Animal { + explicit Dog(std::size_t type = static_type) : Animal(type) { + } + + static constexpr std::size_t static_type = 2; +}; + +// Tiger sits between Dog and Cat so that dropping it does not shrink the +// vector: Cat still requires a slot past Tiger's. A truncating resize would +// hide the bug. +struct Tiger : Animal { + explicit Tiger(std::size_t type = static_type) : Animal(type) { + } + + static constexpr std::size_t static_type = 3; +}; + +struct Cat : Animal { + explicit Cat(std::size_t type = static_type) : Animal(type) { + } + + static constexpr std::size_t static_type = 4; +}; + +namespace { + +// Everything that is not in the hierarchy - methods, overriders, void, char - +// also goes through static_type(). Give those ids of their own, still small +// so they cannot inflate the vector. +inline auto next_other_id() -> std::size_t { + static std::size_t counter = 100; + return ++counter; +} + +template +inline auto other_static_type() -> std::size_t { + static std::size_t value = next_other_id(); + return value; +} + +} // namespace + +struct small_rtti : policies::rtti { + template + struct fn : defaults { + template + static constexpr bool is_polymorphic = std::is_base_of_v; + + template + static auto static_type() -> type_id { + if constexpr (is_polymorphic) { + return type_id(T::static_type); + } else { + return type_id(other_static_type()); + } + } + + template + static auto dynamic_type(const T& obj) -> type_id { + if constexpr (is_polymorphic) { + return type_id(obj.type); + } else { + return type_id(other_static_type()); + } + } + }; +}; + +template +struct vector_registry : + test_registry_::template with::template without< + policies::type_hash> {}; + +template +struct indirect_vector_registry : + vector_registry::template with {}; + +// vptr_map already rebuilds its map and swaps it in, so it is the control: +// these cases pass on it before the fix as well as after. +template +struct map_registry : + vector_registry::template with> {}; + +template +using registries = + mp_list, indirect_vector_registry, map_registry>; + +struct BOOST_OPENMETHOD_ID(speak); + +template +using speak = method< + BOOST_OPENMETHOD_ID(speak), auto(virtual_)->std::string, Registry>; + +template +auto speak_animal(Animal&) -> std::string { + return "..."; +} + +// Tiger deliberately has no overrider of its own. An overrider registers +// itself through `override_aux::impl`, a *static* member whose instantiation +// the registrar object merely forces (core.hpp:2329-2332), so it lives until +// the program exits no matter how the object is scoped. Dropping Tiger's class +// while an overrider still named it would just make the next initialize() +// report `unknown class Tiger`. Tiger inherits Animal's overrider instead, and +// what this test watches is its *slot*, not its overrider. + +template +constexpr bool is_map = false; + +template +constexpr bool is_map> = + true; + +// The entry a registry keeps for `Class`: the v-table pointer itself, or, under +// indirect_vptr, the address of the class' static_vptr. Either way, comparing +// it before and after says whether the slot was carried over. Absent - a hole +// in the vector, or no key in the map - reads as null. +template +auto entry_for() { + using vptr_state = + typename Registry::template policy::state; + auto& vptrs = detail::get(Registry::state().policies).vptrs; + auto type = Registry::rtti::template static_type(); + + if constexpr (is_map>) { + auto iter = vptrs.find(type); + typename std::decay_t::mapped_type entry = nullptr; + + if (iter != vptrs.end()) { + entry = iter->second; + } + + return entry; + } else { + auto index = std::size_t(type); + + return index < vptrs.size() ? vptrs[index] : nullptr; + } +} + +BOOST_AUTO_TEST_CASE_TEMPLATE( + dropped_class_does_not_keep_its_vptr, Registry, registries<__COUNTER__>) { + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER( + typename speak::template override>); + + decltype(entry_for()) tiger_entry; + + { + // A test-only spelling: a registrar is documented to be a static + // object (core.hpp, on `override`; macros.hpp, on + // BOOST_OPENMETHOD_REGISTER; shared_libraries.adoc). One is used here + // because a static never dies before the program does, and this test + // needs the registration to go away between the two initialize() + // calls - which is what unloading a library does to the classes it + // brought. Do not copy this into an example. + // + // The braces are load-bearing. A registrar links itself into the + // registry's static_list, whose links carry no initializer + // (`static_link() = default`, detail/static_list.hpp:25-34) - on + // purpose: registrars live in static storage, which is zeroed before + // any dynamic initialization, so a registrar can link itself in + // whatever order the translation units' constructors run, and the list + // head cannot be constructed after it and wipe the registrations. + // That is what the `coverity[uninit] - zero-initialized static + // storage` note on the push_back in core.hpp is recording. An + // automatic registrar gets none of that, so it has to be + // value-initialised, or push_back asserts on the garbage. + use_classes tiger_classes{}; + + initialize(); + + Tiger tiger; + BOOST_TEST(speak::fn(tiger) == "..."); + + tiger_entry = entry_for(); + BOOST_TEST(tiger_entry != nullptr); + } + + // Tiger is gone; the dispatch data its v-table lived in is freed and + // replaced by this call. + initialize(); + + auto tiger_entry_now = entry_for(); + BOOST_TEST(tiger_entry_now == nullptr); + BOOST_TEST(tiger_entry_now != tiger_entry); + + // The classes that are still registered keep working. + Dog dog; + Cat cat; + BOOST_TEST(speak::fn(dog) == "..."); + BOOST_TEST(speak::fn(cat) == "..."); +}