From 136c87c8db3391e9ddebe5338f8ddcd084b437b4 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Fri, 11 Sep 2026 21:23:32 -0400 Subject: [PATCH 1/3] vptr_vector: rebuild the vector, do not resize it in place `initialize` sized the vector with `resize(size)` and then filled entries in place. `resize` keeps the elements that fit, so an index belonging to a class that is no longer registered kept the v-table pointer written by a previous call - a pointer into the dispatch data the commit-time swap has just freed. Classes stop being registered when a shared library is unloaded: the registrars add themselves in static constructors and remove themselves in the matching destructors, so `dlopen` -> initialize -> `dlclose` -> initialize, the flow shared_libraries.adoc documents, leaves one stale slot per class the library contributed. Dispatching on such a class then reads freed memory, and, after a `dlclose`, jumps into unloaded code. Build a new vector and swap it in, the way `vptr_map::initialize` already does. Every slot the fill loop does not write is null rather than whatever the previous run left there. As a side effect the live vector is untouched until the swap, so the policy no longer depends on the initialize() transaction to undo a half-written fill. Scope, stated plainly: reading one of these slots requires dispatching on a class that is not currently registered, which is an erroneous call to begin with - every registered class gets its slot written. So this is a robustness fix, not a case of correct code breaking. What it buys is that the erroneous call now fails immediately and locally: ASan turns a heap-use-after-free in resolve_uni into a null read at the same site. What it does not fix: under `runtime_checks` the cleared slot is still in bounds, so `vptr()` null-dereferences instead of reporting `missing_class` the way `vptr_map` does on a miss - which is what the doc comment on `vptr()` promises. Closing that needs a null check plus a guarantee that a registered class never holds a null v-table pointer, which does not hold today: a registry with classes and no methods has empty dispatch data, and every class in it gets a null vptr. Left for a separate change. The test drops a class by letting a value-initialised, block-scoped `use_classes` die, which runs the same registrar destructor an unload runs. It covers the direct and indirect vector configurations, and carries `vptr_map` as a control: before this commit the two vector cases fail and the map case passes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr --- .../boost/openmethod/policies/vptr_vector.hpp | 18 +- test/test_initialize_dropped_class.cpp | 231 ++++++++++++++++++ 2 files changed, 246 insertions(+), 3 deletions(-) create mode 100644 test/test_initialize_dropped_class.cpp 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..10f446fa --- /dev/null +++ b/test/test_initialize_dropped_class.cpp @@ -0,0 +1,231 @@ +// 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; + + { + // Deliberately automatic, not the `static` that + // BOOST_OPENMETHOD_REGISTER emits: destroying it unregisters Tiger, + // the way unloading a library unregisters the classes it brought. + // + // The braces are load-bearing. A registrar links itself into the + // registry's static_list, whose `static_link() = default` leaves the + // links uninitialised (detail/static_list.hpp:25-34); every registrar + // the macros emit lives in static storage, where they are zeroed for + // free - hence the `coverity[uninit] - zero-initialized static + // storage` note on the push_back in core.hpp. An automatic one 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) == "..."); +} From 1200be25080cf3fbab522c4427a34f985f3e7ffa Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Sat, 12 Sep 2026 09:22:14 -0400 Subject: [PATCH 2/3] test: say why the registrar links carry no initializer The comment explained that a value-initialised registrar is needed because static storage "is zeroed for free", which reads as a happy accident and invites someone to give the links initializers instead. It is the opposite: the links deliberately have no dynamic initializer so that registration does not depend on static initialization order. Registrars live in static storage, zeroed before any dynamic initialization, so one can link itself in whatever order the translation units' constructors run - and no list head can be constructed after it and wipe the registrations. Adding an initializer would reintroduce the static initialization order fiasco. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr --- test/test_initialize_dropped_class.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/test_initialize_dropped_class.cpp b/test/test_initialize_dropped_class.cpp index 10f446fa..16f1afa1 100644 --- a/test/test_initialize_dropped_class.cpp +++ b/test/test_initialize_dropped_class.cpp @@ -198,12 +198,16 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( // the way unloading a library unregisters the classes it brought. // // The braces are load-bearing. A registrar links itself into the - // registry's static_list, whose `static_link() = default` leaves the - // links uninitialised (detail/static_list.hpp:25-34); every registrar - // the macros emit lives in static storage, where they are zeroed for - // free - hence the `coverity[uninit] - zero-initialized static - // storage` note on the push_back in core.hpp. An automatic one has to - // be value-initialised, or push_back asserts on the garbage. + // 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(); From b382b023fd2282ccf8dfed92442ab331448ed7aa Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Sat, 12 Sep 2026 09:26:32 -0400 Subject: [PATCH 3/3] test: mark the stack registrar as a test-only spelling A registrar is documented to be a static object - core.hpp says so on `override`, macros.hpp on BOOST_OPENMETHOD_REGISTER, shared_libraries.adoc on the registrars generally. This test puts one on the stack because a static never dies before the program does, and the test needs the registration to go away between two initialize() calls, which is what unloading a library does. Say so, so the spelling is not lifted into an example. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr --- test/test_initialize_dropped_class.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/test_initialize_dropped_class.cpp b/test/test_initialize_dropped_class.cpp index 16f1afa1..3ef579ad 100644 --- a/test/test_initialize_dropped_class.cpp +++ b/test/test_initialize_dropped_class.cpp @@ -193,9 +193,13 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( decltype(entry_for()) tiger_entry; { - // Deliberately automatic, not the `static` that - // BOOST_OPENMETHOD_REGISTER emits: destroying it unregisters Tiger, - // the way unloading a library unregisters the classes it brought. + // 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