[bugfix] Fix assignment operators for moved-from external-heap tracked_ptrs - #13
Open
jairov4 wants to merge 2 commits into
Open
[bugfix] Fix assignment operators for moved-from external-heap tracked_ptrs#13jairov4 wants to merge 2 commits into
jairov4 wants to merge 2 commits into
Conversation
All assignment operators (nullptr_t, const tracked_ptr&, const tracked_ptr<U>&, unique_ptr<U>&&) now check for moved-from state (where _ptr() returns nullptr) and allocate a fresh Pointer instead of unconditionally calling _ptr()->store(...). This fixes a crash in std::stable_sort (via libc++'s __insertion_sort_move) where a tracked_ptr is move-constructed from, then copy-assigned to. When both tracked_ptrs live on the C++ heap (external-heap mode), the move constructor steals the internal Pointer, leaving the source with null _ptr(). Without this fix, the subsequent copy assignment dereferences the null pointer. Adds regression test AssignmentToMovedFromExternalHeap.
PageInfo<T>::child_pointers was a plain eagerly-initialized `inline
static` data member whose constructor does real dynamic (heap-
allocating) initialization via ChildPointers' std::vector `map`
member. Unlike its sibling members in the same struct --
private_metadata() and array_metadata() -- which already use the safe
lazy-singleton (function-local static) pattern, child_pointers was
not lazy.
This is a classic static-initialization-order fiasco: if the very
first program-wide construction of a tracked type T happens as (or
nested inside) another translation unit's own static/dynamic
initializer -- e.g. a global variable's constructor calling a factory
function in another TU, whose constructor itself nested-constructs
another tracked type -- PageInfo<T>::child_pointers may not yet be
dynamically initialized when Maker's "first construction of this
type" code path fills the object's memory with a sentinel and expects
child_pointers.map to already be correctly sized. When it isn't,
Pointer's constructor hits:
Assertion failed: (offset / 8 < pointers.map->size()), function
Pointer, file pointer.h, line 25.
Convert child_pointers into a lazily-constructed function-local
static, mirroring private_metadata()/array_metadata() in the same
struct. A function-local static is guaranteed by the standard to
initialize exactly once, on first use, regardless of static-
initialization order -- unlike a plain inline static data member,
whose initialization order relative to other translation units'
static-duration objects is unspecified.
Updates the three call sites in maker.h and the two callers in
metadata.h/array_metadata.h from member access to function call.
Adds tests/static_init.{h,cpp}/static_init_factory.cpp, a standalone
test binary (see tests/CMakeLists.txt) that reproduces the exact
failure scenario: a tracked type constructed for the first time from
a global variable's dynamic initializer, calling a factory function
defined in a separate translation unit, whose constructor nested-
constructs another tracked type for the first time too. Verified this
crashes with the pre-fix child_pointers and passes with the fix. Kept
out of the shared `tests` binary since it deliberately keeps a
tracked object alive for the whole process lifetime and constructing
during static init is a whole-program concern that shouldn't share
collector/thread state with the rest of that suite's exact
live-object-count assertions.
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
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.
All assignment operators (
nullptr_t,const tracked_ptr&,const tracked_ptr<U>&,unique_ptr<U>&&,tracked_ptr&&) now check for moved-from state (where_ptr()returnsnullptr) and allocate a freshPointerinstead of unconditionally calling_ptr()->store(...).Root cause
When a
tracked_ptr<T>lives on the C++ heap (external-heap mode), the move constructor steals the internalPointer*from the source, leaving its_ptr()null. The original copy/null/unique_ptr assignment operators then call_ptr()->store(...)unconditionally → null-pointer segfault.This surfaced in
std::stable_sort(via libc++'s__insertion_sort_move):it move-constructs a temporary, then copy-assigns back to the moved-from original.
Fix
Five operators in
tracked_ptrnow guard the store withif (auto self_ptr = _ptr())and allocate a newPointerin the else branch. The move-assignment operators were already safe (they had the guard), but were left unchanged.Test
Added
AssignmentToMovedFromExternalHeap— exercises copy assignment, move assignment,nullptr_tassignment, andunique_ptrassignment to moved-from external-heap tracked_ptrs. All 203 existing tests pass.Additional fix: static-initialization-order crash in
PageInfo<T>::child_pointersRoot cause
PageInfo<T>::child_pointers(insgcl/detail/page_info.h) was a plain,eagerly-initialized
inline staticdata member:Unlike its sibling members in the same struct — private_metadata() and array_metadata() — which already use the lazy function-local-static singleton pattern, child_pointers was not lazy, even though its constructor does real dynamic (heap-allocating) initialization via ChildPointers' std::vector<std::atomic<uint8_t>> map member.
This is a textbook static-initialization-order fiasco: if the very first program-wide construction of a tracked type T happens as (or nested inside) another translation unit's own static/dynamic initializer — e.g. a global variable's constructor calling a factory function defined in another TU, whose constructor itself nested-constructs another tracked type — then PageInfo::child_pointers may not yet be dynamically initialized when Maker's "first construction of this type" code path (in maker.h) fills the object's memory with a sentinel and expects child_pointers.map to already be correctly sized. When it isn't, Pointer's constructor hits:
Assertion failed: (offset / 8 < pointers.map->size()), function Pointer, file pointer.h, line 25.