vptr_vector: rebuild the vector, do not resize it in place - #104
Open
jll63 wants to merge 3 commits into
Open
Conversation
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr
|
An automated preview of the documentation is available at https://104.openmethod.prtest3.cppalliance.org/libs/openmethod/doc/html/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-09-12 13:37:04 UTC |
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #104 +/- ##
===========================================
- Coverage 93.46% 93.41% -0.06%
===========================================
Files 22 22
Lines 1653 1686 +33
Branches 500 507 +7
===========================================
+ Hits 1545 1575 +30
- Misses 64 66 +2
- Partials 44 45 +1
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(Written by Claude Code, on behalf of @jll63.)
vptr_vector::initializesized its vector withresize(size)and then filled entries inplace.
resizekeeps the elements that fit, so an index belonging to a class that is nolonger 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: registrars add themselves
in static constructors and remove themselves in the matching destructors. So
dlopen→initialize()→dlclose→initialize(), the flowshared_libraries.adocdocuments,leaves one stale slot per class the library contributed.
Reproduced deterministically (small integer type ids, no
type_hash, so index == type id):The change
Build a new vector and swap it in, the way
vptr_map::initializealready does. Every slotthe 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.vptr_vectorwas the only registry-persistent container ininclude/filled by in-placeindexed assignment; every other
resizeis on a per-run compiler object or is followed by afull
std::fill.Scope
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. 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: the heap-use-after-free above becomes a
null read at the same site.
Not fixed here. Under
runtime_checksthe cleared slot is still in bounds, sovptr()null-dereferences instead of reporting
missing_classthe wayvptr_mapdoes on a miss —which is what the doc comment on
vptr()promises. Closing that needs a null check plus aguarantee 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.
Test
test/test_initialize_dropped_class.cppdrops a class by letting a value-initialised,block-scoped
use_classesdie, which runs the same registrar destructor an unload runs. Itcovers the direct and indirect vector configurations and carries
vptr_mapas a control:before this commit the two vector cases fail and the map case passes.
Two things the test comments record, because both cost time to find:
override_aux::impl, a static member whoseinstantiation the registrar object merely forces, so scoping an overrider does nothing —
the dropped class must not have one;
static_link() = defaultleaves the list links uninitialised, and every registrar themacros emit lives in static storage where they are zeroed for free. An automatic
use_classeshas to be value-initialised orpush_backasserts on the garbage.Verification
ctestpass.toolset=gcc: 548 targets updated, no failures, including bothdynamic_loadingvariants.clang-format-22applied.🤖 Generated with Claude Code
https://claude.ai/code/session_01JQa4fuiwcfsheZYTCyfPPr