From 6872118cc4cc5b7776971ede62bfa74f37ca91e0 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Thu, 10 Sep 2026 21:15:41 -0400 Subject: [PATCH 1/2] initialize: move the last throwing statement before the commit #95 made initialize() transactional, but `transaction.commit()` was called one statement too early: the `++tr << "Installing\n"` that follows it goes through the `output` policy, which is user code and can throw. When it does, the transaction destructor sees a committed transaction and restores nothing, so the policies keep the v-table pointers they read out of `new_dispatch_data` - the local vector unwinding is about to free. The registry still holds the previous dispatch data (the swap never ran), so the next dispatch through the policy state reads freed memory: the exact use-after-free #95 fixed, reached by a different route. ASan, gcc 13, an `output` policy whose stream throws on "Installing" plus initialize(trace(true)): ERROR: AddressSanitizer: heap-use-after-free READ of size 8 ... in resolve_uni<...> freed by ... write_global_data() Move the trace write above commit(), where a throw rolls the policies back, and extract the writes that follow into `commit_global_data()`, declared `noexcept` so that a throwing statement added there terminates loudly instead of silently reopening this. The test grew a case for that path, and two of its existing assertions were vacuous: the failing initialize saw exactly the input the preceding successful one saw, so `fast_perfect_hash` - which re-seeds a fixed PRNG - recomputed identical factors, and `next` resolved to the same overrider. Both held whether or not anything was rolled back; reintroducing the dangling-`next` half of #81 left the suite reporting "No errors detected". Perturb the input between the two calls instead, with two function-local static registrars: an extra class changes the hash factors, the control table and the v-table pointers, and an extra inheritance edge changes what `next` would be set to. The hash assertion also compares the whole policy state now - factors and control table - rather than `hash_range()` alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RQG6CbE4o2agseE7bDVzHS --- include/boost/openmethod/initialize.hpp | 30 +++- test/test_initialize_transaction.cpp | 173 ++++++++++++++++++++++-- 2 files changed, 184 insertions(+), 19 deletions(-) diff --git a/include/boost/openmethod/initialize.hpp b/include/boost/openmethod/initialize.hpp index 50d4ce56..ac385556 100644 --- a/include/boost/openmethod/initialize.hpp +++ b/include/boost/openmethod/initialize.hpp @@ -727,6 +727,8 @@ struct registry::compiler : detail::generic_compiler { std::vector::const_iterator group, const bitvec& candidates, bool concrete); void write_global_data(); + void commit_global_data( + std::vector& new_dispatch_data) noexcept; void print(const method_report& report) const; void print_slots(); static void select_dominant_overriders( @@ -1766,10 +1768,10 @@ void registry::compiler::write_global_data() { // v-table pointer is staged in its class_, where the policies read it. // Only then are the shared locations patched - the method_infos' slots // and strides, the overriders' `next`, the class_infos' static_vptr - and - // the dispatch data swapped in; none of that can throw. If a policy - // throws, the registry still holds the previous dispatch state, complete - // and consistent, rather than pointers into a vector that unwinding has - // just freed. + // the dispatch data swapped in, by commit_global_data(), which is + // `noexcept`. If a policy throws, the registry still holds the previous + // dispatch state, complete and consistent, rather than pointers into a + // vector that unwinding has just freed. auto dispatch_data_size = std::accumulate( methods.begin(), methods.end(), std::size_t(0), @@ -1880,12 +1882,26 @@ void registry::compiler::write_global_data() { detail::registry_state_transaction transaction; detail::initialize_policies::fn(*this, options); - transaction.commit(); - - // Commit. Nothing from here on can throw. + // Last statement that can throw: the trace goes through the `output` + // policy, which is user-supplied. After the commit it would be the very + // bug this arrangement exists to prevent - the policies keeping the + // v-table pointers they just read from `new_dispatch_data`, which + // unwinding frees. ++tr << "Installing\n"; + transaction.commit(); + commit_global_data(new_dispatch_data); +} + +// The commit point. Called once every step that can fail has succeeded, and +// `noexcept` so that a throwing statement added here terminates loudly +// instead of leaving the policies pointing into `new_dispatch_data`, which +// the caller destroys on the way out. +template +template +void registry::compiler::commit_global_data( + std::vector& new_dispatch_data) noexcept { for (auto& m : methods) { auto first_info = m.infos[0]; diff --git a/test/test_initialize_transaction.cpp b/test/test_initialize_transaction.cpp index 551618df..900038a1 100644 --- a/test/test_initialize_transaction.cpp +++ b/test/test_initialize_transaction.cpp @@ -3,11 +3,13 @@ // See accompanying file LICENSE_1_0.txt // or copy at http://www.boost.org/LICENSE_1_0.txt) -// initialize() is transactional: if a policy's initialize throws, the -// registry keeps the dispatch state it had before the call - the static -// v-table pointers, the `next` pointers, the dispatch data and every policy's -// state - instead of pointers into a vector that unwinding has freed. It is -// marked as not initialized, though, until a call succeeds. +// initialize() is transactional: if anything between staging the new dispatch +// data and the commit point throws - a policy's initialize, or the trace +// write that announces the installation, which goes through the user-supplied +// `output` policy - the registry keeps the dispatch state it had before the +// call: the static v-table pointers, the `next` pointers, the dispatch data +// and every policy's state, instead of pointers into a vector that unwinding +// has freed. It is marked as not initialized, though, until a call succeeds. #include #include @@ -19,8 +21,10 @@ #include "test_util.hpp" +#include #include #include +#include #include using boost::mp11::mp_list; @@ -85,8 +89,10 @@ struct Animal { virtual ~Animal() = default; }; -struct Dog : Animal {}; +struct Carnivore : Animal {}; +struct Dog : Carnivore {}; struct Cat : Animal {}; +struct Bird : Animal {}; struct BOOST_OPENMETHOD_ID(poke); @@ -104,18 +110,24 @@ auto poke_dog(Dog& dog) -> std::string { return poke::template next>(dog) + " bark"; } +template +auto poke_carnivore(Carnivore& carnivore) -> std::string { + return poke::template next>(carnivore) + + " growl"; +} + template struct snapshot { using vptr_state = typename Registry::template policy::state; using type_hash = typename Registry::template policy; + using hash_state = typename type_hash::state; snapshot() : dispatch_data(Registry::state().dispatch_data.data()), dog_vptr(Registry::template static_vptr), cat_vptr(Registry::template static_vptr), next(poke::template next>), - hash_range(type_hash::hash_range()), policies(Registry::state().policies) { } @@ -123,11 +135,17 @@ struct snapshot { return (detail::get(policies).vptrs); } + // Everything `fast_perfect_hash::initialize` writes: the factors and the + // control table. `hash_range()` alone would miss a rollback that restored + // the range but not the factors. + auto hash() -> decltype(auto) { + return (detail::get(policies)); + } + const detail::word* dispatch_data; vptr_type dog_vptr; vptr_type cat_vptr; decltype(poke::template next>) next; - std::pair hash_range; decltype(Registry::state().policies) policies; }; @@ -138,11 +156,17 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( typename Registry::registry_type>; using vptr_state = typename snapshot::vptr_state; - BOOST_OPENMETHOD_REGISTER(use_classes); + // Dog is registered here with Animal as its direct base, although it + // really derives from Carnivore. The missing edge is added between the two + // initializes, below. + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(use_classes); BOOST_OPENMETHOD_REGISTER( typename poke::template override>); BOOST_OPENMETHOD_REGISTER( typename poke::template override>); + BOOST_OPENMETHOD_REGISTER( + typename poke::template override>); Dog dog; Cat cat; @@ -156,6 +180,21 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( snapshot before; + // Perturb the input of the call that is about to fail. Without this it + // would see exactly what the successful call saw, and recompute + // bit-identical values for everything compared below - `fast_perfect_hash` + // re-seeds a fixed PRNG over the same class set, and `next` + // resolves to the same overrider - so the assertions would hold whether or + // not the transaction rolled anything back. These registrars are + // function-local statics: they register on first pass through the + // declaration, here, not before main. `Bird` changes the class set that + // the hash factors, the control table and the v-table pointers are + // computed from; the Carnivore edge inserts `poke_carnivore` between + // `poke_dog` and `poke_animal`, changing what `next` resolves + // to. The final initialize below observes both. + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(use_classes); + explosive::armed = true; BOOST_CHECK_THROW(initialize(), std::runtime_error); explosive::armed = false; @@ -171,8 +210,15 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( // rejects under /WX (-Wmicrosoft-cast). BOOST_TEST( (poke::template next> == before.next)); - BOOST_TEST( - (snapshot::type_hash::hash_range() == before.hash_range)); + // Parenthesized for the same reason: Boost.Test cannot print hash factors + // or vectors of type ids. + auto& hash = + detail::get::hash_state>(st.policies); + BOOST_TEST((hash.fn.mult == before.hash().fn.mult)); + BOOST_TEST((hash.fn.shift == before.hash().fn.shift)); + BOOST_TEST((hash.fn.min_value == before.hash().fn.min_value)); + BOOST_TEST((hash.fn.max_value == before.hash().fn.max_value)); + BOOST_TEST((hash.control == before.hash().control)); BOOST_TEST((detail::get(st.policies).vptrs == before.vptrs())); // ...including the state of the policy that threw, after writing to it. BOOST_TEST(Registry::template state().generation == 1); @@ -180,10 +226,17 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( // ...but dispatch is refused until an initialize() succeeds. BOOST_CHECK_THROW(poke::fn(dog), not_initialized); + // A successful call installs what the failed one would have: the + // perturbation is visible in the result, which confirms that the + // assertions above compared values that really do differ between the two + // calls. initialize(); BOOST_TEST(st.initialized); - BOOST_TEST(poke::fn(dog) == "silence bark"); + BOOST_TEST(poke::fn(dog) == "silence growl bark"); BOOST_TEST(poke::fn(cat) == "silence"); + BOOST_TEST( + (poke::template next> != before.next)); + BOOST_TEST((hash.control != before.hash().control)); BOOST_TEST(Registry::template state().generation == 3); } @@ -222,3 +275,99 @@ BOOST_AUTO_TEST_CASE_TEMPLATE( BOOST_TEST(st.initialized); BOOST_TEST(poke::fn(dog) == "silence bark"); } + +// The other way an initialize can fail after the policies have run: the trace +// goes through the `output` policy, which is user code, so the statement that +// announces the installation can throw. It sits before the commit, and must +// stay there - after it, the policies would keep the v-table pointers they +// just read out of the staging vector, which unwinding frees. + +// Discards what it is given, and throws once, on the message named in `trap`. +struct trapping_stream { + static inline const char* trap = nullptr; + + void write(const char* str) { + if (trap != nullptr && std::strstr(str, trap) != nullptr) { + trap = nullptr; + throw std::runtime_error("output"); + } + } + + auto is_on() const -> bool { + return true; + } +}; + +inline auto operator<<(trapping_stream& os, const char* str) + -> trapping_stream& { + os.write(str); + return os; +} + +inline auto operator<<(trapping_stream& os, const std::string_view&) + -> trapping_stream& { + return os; +} + +inline auto operator<<(trapping_stream& os, const void*) -> trapping_stream& { + return os; +} + +inline auto operator<<(trapping_stream& os, void (*)()) -> trapping_stream& { + return os; +} + +inline auto operator<<(trapping_stream& os, std::size_t) -> trapping_stream& { + return os; +} + +struct trapping_output : policies::output { + template + struct fn { + struct state { + trapping_stream os; + }; + + static auto& stream() { + return Registry::template state().os; + } + }; +}; + +template +struct tracing_registry : + test_registry_::template with< + policies::runtime_checks, policies::throw_error_handler, + trapping_output> {}; + +BOOST_AUTO_TEST_CASE(a_throwing_trace_does_not_commit) { + using Registry = tracing_registry<__COUNTER__>; + using vptr_state = typename snapshot::vptr_state; + + BOOST_OPENMETHOD_REGISTER(use_classes); + BOOST_OPENMETHOD_REGISTER(poke::override>); + BOOST_OPENMETHOD_REGISTER(poke::override>); + + Dog dog; + auto& st = Registry::state(); + + initialize(); + BOOST_TEST(poke::fn(dog) == "silence bark"); + + snapshot before; + + trapping_stream::trap = "Installing"; + BOOST_CHECK_THROW(initialize(trace(true)), std::runtime_error); + BOOST_TEST(trapping_stream::trap == nullptr); // it did throw there + + BOOST_TEST(!st.initialized); + BOOST_TEST(st.dispatch_data.data() == before.dispatch_data); + BOOST_TEST(Registry::template static_vptr == before.dog_vptr); + // The one that matters: had the policies been committed, these would be + // pointers into the staging vector, which no longer exists. + BOOST_TEST((detail::get(st.policies).vptrs == before.vptrs())); + + initialize(); + BOOST_TEST(st.initialized); + BOOST_TEST(poke::fn(dog) == "silence bark"); +} From 69e26348c56a4103314e2e1c1d79884eefeb4bab Mon Sep 17 00:00:00 2001 From: Jean-Louis Leroy Date: Fri, 11 Sep 2026 20:08:54 -0400 Subject: [PATCH 2/2] doc: say what a call after a failed initialize() does The transaction section says the registry is marked as not initialized and that initialize() must be called again, but not what happens if a method is called before it is. Only `runtime_checks` diagnoses that; otherwise the call dispatches through the previous tables, which is harmless for a registry whose classes are all still loaded and a use-after-free for one whose overriders came out of a library that has since been dlclose'd - the case the section is written for. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr --- doc/modules/ROOT/pages/registries_and_policies.adoc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/doc/modules/ROOT/pages/registries_and_policies.adoc b/doc/modules/ROOT/pages/registries_and_policies.adoc index db0f9ed0..a691c415 100644 --- a/doc/modules/ROOT/pages/registries_and_policies.adoc +++ b/doc/modules/ROOT/pages/registries_and_policies.adoc @@ -160,7 +160,10 @@ say - every policy gets its previous state back, and nothing else in the registry is modified: the v-table pointers, `next` pointers and dispatch tables from the previous call all stay in place. The registry is marked as not initialized, though, since that state no longer reflects the registrations, -and cpp:initialize[] must be called again before calling a method. +and cpp:initialize[] must be called again before calling a method. Only a +registry with the cpp:runtime_checks[] policy diagnoses a call made in the +meantime; without it, the call dispatches through the previous tables, which - +after a `dlclose` - may point into unloaded code. A registry can also be created by copying an existing registry's policies, using the cpp:with[] and cpp:without[] nested templates. For example,