Skip to content

[bugfix] Fix assignment operators for moved-from external-heap tracked_ptrs - #13

Open
jairov4 wants to merge 2 commits into
pebal:mainfrom
jairov4:fix/moved-from-external-heap-assignment
Open

[bugfix] Fix assignment operators for moved-from external-heap tracked_ptrs#13
jairov4 wants to merge 2 commits into
pebal:mainfrom
jairov4:fix/moved-from-external-heap-assignment

Conversation

@jairov4

@jairov4 jairov4 commented Jul 30, 2026

Copy link
Copy Markdown

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() returns nullptr) and allocate a fresh Pointer instead 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 internal Pointer* 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_ptr now guard the store with if (auto self_ptr = _ptr()) and allocate a new Pointer in 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_t assignment, and unique_ptr assignment to moved-from external-heap tracked_ptrs. All 203 existing tests pass.

Additional fix: static-initialization-order crash in PageInfo<T>::child_pointers

Root cause

PageInfo<T>::child_pointers (in sgcl/detail/page_info.h) was a plain,
eagerly-initialized inline static data member:

inline static ChildPointers child_pointers {!MayContainTracked<Type>::value, ObjectSize};

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants