From c80eafb3e465f122b256454cca5546c5ebd804b5 Mon Sep 17 00:00:00 2001 From: Jairo Velasco Date: Thu, 30 Jul 2026 11:17:42 -0500 Subject: [PATCH 1/2] Fix assignment operators for moved-from external-heap tracked_ptrs All assignment operators (nullptr_t, const tracked_ptr&, const tracked_ptr&, unique_ptr&&) 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. --- sgcl/tracked_ptr.h | 36 +++++++++++++++++++++++++++---- tests/tracked_ptr.cpp | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/sgcl/tracked_ptr.h b/sgcl/tracked_ptr.h index 687313e..34ec72c 100644 --- a/sgcl/tracked_ptr.h +++ b/sgcl/tracked_ptr.h @@ -108,25 +108,53 @@ namespace sgcl { } tracked_ptr& operator=(std::nullptr_t) noexcept { - _ptr()->store(nullptr); + if (auto ptr = _ptr()) { + ptr->store(nullptr); + } else { + auto new_ptr = make_tracked(); + auto ref = new_ptr.release(); + _raw_ptr_ref = _set_flag(ref, ExternalHeapFlag); + ref->store(nullptr); + } return *this; } tracked_ptr& operator=(const tracked_ptr& p) noexcept { - _ptr()->store(p.get()); + if (auto self_ptr = _ptr()) { + self_ptr->store(p.get()); + } else { + auto new_ptr = make_tracked(); + auto ref = new_ptr.release(); + _raw_ptr_ref = _set_flag(ref, ExternalHeapFlag); + ref->store(p.get()); + } return *this; } template::element_type*, element_type*>, int> = 0> tracked_ptr& operator=(const tracked_ptr& p) noexcept { - _ptr()->store(static_cast(p.get())); + if (auto self_ptr = _ptr()) { + self_ptr->store(static_cast(p.get())); + } else { + auto new_ptr = make_tracked(); + auto ref = new_ptr.release(); + _raw_ptr_ref = _set_flag(ref, ExternalHeapFlag); + ref->store(static_cast(p.get())); + } return *this; } template::element_type*, element_type*>, int> = 0> tracked_ptr& operator=(unique_ptr&& u) noexcept { auto p = u.release(); - _ptr()->store(static_cast(p)); + if (auto self_ptr = _ptr()) { + self_ptr->store(static_cast(p)); + } else { + auto new_ptr = make_tracked(); + auto ref = new_ptr.release(); + _raw_ptr_ref = _set_flag(ref, ExternalHeapFlag); + ref->store(static_cast(p)); + } return *this; } diff --git a/tests/tracked_ptr.cpp b/tests/tracked_ptr.cpp index 3b54ca9..70243e0 100644 --- a/tests/tracked_ptr.cpp +++ b/tests/tracked_ptr.cpp @@ -406,3 +406,52 @@ TEST(TrackedPtr_Tests, Casts) { auto pbar = const_pointer_cast(cbar); EXPECT_EQ(pbar->get_value(), 5); } + +// Regression test: tracked_ptrs on the C++ heap (external-heap mode) can be +// left in a "moved-from" state where _ptr() returns nullptr after a move +// construction where both source and destination are external-heap tracked_ptrs. +// All assignment operators must handle this case gracefully by re-allocating +// a fresh Pointer rather than calling _ptr()->store(...) unconditionally. +TEST(TrackedPtr_Tests, AssignmentToMovedFromExternalHeap) { + // Create an external-heap tracked_ptr with a value + auto src = std::make_unique>(make_tracked(42)); + ASSERT_NE(*src, nullptr); + EXPECT_EQ((*src)->get_value(), 42); + EXPECT_TRUE(src->allocated_on_external_heap()); + + // Move-construct another external-heap tracked_ptr from src. + // Both being on the C++ heap (external-heap mode) triggers the move + // constructor's Pointer-steal path, leaving *src moved-from with null _ptr(). + auto dest = std::make_unique>(std::move(*src)); + ASSERT_NE(*dest, nullptr); + EXPECT_EQ((*dest)->get_value(), 42); + EXPECT_TRUE(dest->allocated_on_external_heap()); + // src is now moved-from; do not dereference it before reassigning. + + // At this point src is moved-from: _ptr() returns nullptr. + // Verify that copy assignment recovers gracefully. + *src = *dest; + ASSERT_NE(*src, nullptr); + EXPECT_EQ((*src)->get_value(), 42); + EXPECT_TRUE(src->allocated_on_external_heap()); + + // Move-assign to the recovered src + auto dest2 = std::make_unique>(make_tracked(99)); + *src = std::move(*dest2); + ASSERT_NE(*src, nullptr); + EXPECT_EQ((*src)->get_value(), 99); + + // Nullptr assignment to a moved-from tracked_ptr + auto src2 = std::make_unique>(make_tracked(7)); + auto dest3 = std::make_unique>(std::move(*src2)); + *src2 = nullptr; + EXPECT_EQ(*src2, nullptr); + EXPECT_TRUE(src2->allocated_on_external_heap()); + + // Unique_ptr assignment to a moved-from tracked_ptr + *src2 = make_tracked(21); + ASSERT_NE(*src2, nullptr); + EXPECT_EQ(**src2, 21); + + // Cleanup (destructors run automatically) +} From 01aa75d59de1f0d1afea732e163d359094e6fa54 Mon Sep 17 00:00:00 2001 From: Jairo Velasco Date: Mon, 3 Aug 2026 07:22:51 -0500 Subject: [PATCH 2/2] Fix static-initialization-order crash in PageInfo::child_pointers PageInfo::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::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. --- sgcl/detail/array_metadata.h | 2 +- sgcl/detail/maker.h | 18 +++++++-------- sgcl/detail/metadata.h | 2 +- sgcl/detail/page_info.h | 19 +++++++++++++++- tests/CMakeLists.txt | 22 ++++++++++++++++++ tests/static_init.cpp | 40 ++++++++++++++++++++++++++++++++ tests/static_init.h | 43 +++++++++++++++++++++++++++++++++++ tests/static_init_factory.cpp | 14 ++++++++++++ 8 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 tests/static_init.cpp create mode 100644 tests/static_init.h create mode 100644 tests/static_init_factory.cpp diff --git a/sgcl/detail/array_metadata.h b/sgcl/detail/array_metadata.h index 05c81fd..b24a842 100644 --- a/sgcl/detail/array_metadata.h +++ b/sgcl/detail/array_metadata.h @@ -11,7 +11,7 @@ namespace sgcl::detail { struct ArrayMetadata { template ArrayMetadata(T*) noexcept - : child_pointers(TypeInfo::child_pointers) + : child_pointers(TypeInfo::child_pointers()) , destroy(ArrayBase::get_destroy_function()) , type_info(typeid(T[])) , object_size(TypeInfo::ObjectSize) diff --git a/sgcl/detail/maker.h b/sgcl/detail/maker.h index d1545e1..7557626 100644 --- a/sgcl/detail/maker.h +++ b/sgcl/detail/maker.h @@ -46,15 +46,15 @@ namespace sgcl::detail { if constexpr(Info::MayContainTracked) { auto& thread = current_thread(); auto range_guard = thread.use_alloc_range({(uintptr_t)(p), sizeof(T)}); - if (!Info::child_pointers.final.load(std::memory_order_acquire)) { + if (!Info::child_pointers().final.load(std::memory_order_acquire)) { auto count = sizeof(Type) / sizeof(RawPointer); auto mem = (RawPointer*)p; for (int i = 0; i < count; ++i) { mem[i].store((void*)size_t(1), std::memory_order_relaxed); } - auto range_guard = thread.use_child_pointers({(uintptr_t)p, &Info::child_pointers.map}); + auto range_guard = thread.use_child_pointers({(uintptr_t)p, &Info::child_pointers().map}); _construct(p, std::forward(a)...); - Info::child_pointers.final.store(true, std::memory_order_release); + Info::child_pointers().final.store(true, std::memory_order_release); } else { _construct(p, std::forward(a)...); } @@ -90,11 +90,11 @@ namespace sgcl::detail { auto mem = allocator.alloc(); if constexpr(Info::MayContainTracked) { auto range_guard = thread.use_alloc_range({(uintptr_t)(mem), sizeof(T)}); - if (!Info::child_pointers.final.load(std::memory_order_acquire)) { + if (!Info::child_pointers().final.load(std::memory_order_acquire)) { std::fill_n((size_t*)mem, sizeof(T) / sizeof(size_t), size_t(1)); - auto child_guard = thread.use_child_pointers({(uintptr_t)mem, &Info::child_pointers.map}); + auto child_guard = thread.use_child_pointers({(uintptr_t)mem, &Info::child_pointers().map}); _construct_and_register(mem, std::forward(a)...); - Info::child_pointers.final.store(true, std::memory_order_release); + Info::child_pointers().final.store(true, std::memory_order_release); } else { std::memset(mem, 0, sizeof(T)); _construct_and_register(mem, std::forward(a)...); @@ -245,13 +245,13 @@ namespace sgcl::detail { int offset; auto& thread = current_thread(); auto range_guard = thread.use_alloc_range({(uintptr_t)(array.data), sizeof(Type) * array.capacity}); - if (array.size && !Info::child_pointers.final.load(std::memory_order_acquire)) { + if (array.size && !Info::child_pointers().final.load(std::memory_order_acquire)) { std::fill_n((size_t*)array.data, sizeof(Type) / sizeof(size_t), size_t(1)); std::memset((void*)((Type*)array.data + 1), 0, sizeof(Type) * (array.capacity - 1)); array.metadata.store(&Info::array_metadata(), std::memory_order_release); - auto child_guard = thread.use_child_pointers({(uintptr_t)array.data, &Info::child_pointers.map}); + auto child_guard = thread.use_child_pointers({(uintptr_t)array.data, &Info::child_pointers().map}); _init((Type*)array.data, 0, 1, std::forward(a)...); - Info::child_pointers.final.store(true, std::memory_order_release); + Info::child_pointers().final.store(true, std::memory_order_release); offset = 1; } else { std::memset(array.data, 0, sizeof(Type) * array.capacity); diff --git a/sgcl/detail/metadata.h b/sgcl/detail/metadata.h index 8a741bc..7c792f9 100644 --- a/sgcl/detail/metadata.h +++ b/sgcl/detail/metadata.h @@ -12,7 +12,7 @@ namespace sgcl::detail { struct Metadata { template Metadata(T*) noexcept - : child_pointers(TypeInfo::child_pointers) + : child_pointers(TypeInfo::child_pointers()) , destroy(TypeInfo::get_destroy_function()) , free(TypeInfo::Allocator::free) , object_size(TypeInfo::ObjectSize) diff --git a/sgcl/detail/page_info.h b/sgcl/detail/page_info.h index 6bdde2e..fcd333a 100644 --- a/sgcl/detail/page_info.h +++ b/sgcl/detail/page_info.h @@ -43,7 +43,24 @@ namespace sgcl::detail { return *metadata; } - inline static ChildPointers child_pointers {!MayContainTracked::value, ObjectSize}; + // Lazily constructed (mirrors private_metadata()/array_metadata() + // above), not a plain eagerly-initialized static data member: this + // member's constructor does real dynamic (heap-allocating) + // initialization via ChildPointers' std::vector `map`, so an eager + // `inline static` here would have unspecified initialization order + // relative to every other translation unit's own static-duration + // objects. A caller whose own static/dynamic initializer is the + // first thing to construct a tracked object of this type -- e.g. a + // module-level variable initializer that runs during C++ static + // initialization, before main() -- could otherwise observe this + // member not yet constructed (an under-sized/absent `map`), corrupting + // the child-pointer bitmap offset check in Pointer's constructor. + // A function-local static is guaranteed to initialize exactly once, + // on first call, regardless of static-initialization order. + inline static ChildPointers& child_pointers() { + static ChildPointers cp{!MayContainTracked::value, ObjectSize}; + return cp; + } private: static void _destroy(void* p) noexcept { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 37422c2..6f4d6f3 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,7 +4,29 @@ project(SGCL_Tests LANGUAGES CXX) enable_testing() file(GLOB TEST_SOURCES "*.cpp") + +# static_init.cpp/static_init_factory.cpp reproduce a tracked object being +# constructed during C++ static/dynamic initialization (before main()). This +# exercises whole-program/process startup behavior and deliberately keeps a +# tracked object alive for the process's entire lifetime, so it is built as +# its own standalone binary rather than folded into the shared `tests` +# executable below, which asserts exact collector::get_live_object_count() +# baselines throughout and must not share collector/thread state with it. +list(REMOVE_ITEM TEST_SOURCES + "${CMAKE_CURRENT_SOURCE_DIR}/static_init.cpp" + "${CMAKE_CURRENT_SOURCE_DIR}/static_init_factory.cpp" +) + add_executable(tests ${TEST_SOURCES} types.h) target_link_libraries(tests gtest gtest_main sgcl) target_include_directories(tests PRIVATE ${CMAKE_SOURCE_DIR}) add_test(NAME tests COMMAND tests) + +add_executable(static_init_test + static_init.cpp + static_init_factory.cpp + static_init.h +) +target_link_libraries(static_init_test gtest gtest_main sgcl) +target_include_directories(static_init_test PRIVATE ${CMAKE_SOURCE_DIR}) +add_test(NAME static_init_test COMMAND static_init_test) diff --git a/tests/static_init.cpp b/tests/static_init.cpp new file mode 100644 index 0000000..b482766 --- /dev/null +++ b/tests/static_init.cpp @@ -0,0 +1,40 @@ +//------------------------------------------------------------------------------ +// SGCL: Smart Garbage Collection Library +// Copyright (c) 2022-2025 Sebastian Nibisz +// SPDX-License-Identifier: Apache-2.0 +//------------------------------------------------------------------------------ +#include "static_init.h" + +// Constructed during C++ static/dynamic initialization, before main() (and +// therefore before any TEST body) runs. This is the very first construction +// of StaticInitOuter (and, nested inside its constructor, the very first +// construction of StaticInitInner) anywhere in the program. This reproduces +// the scenario that used to crash: PageInfo::child_pointers was a plain +// eagerly-initialized `inline static` data member (see sgcl/detail/page_info.h) +// whose own dynamic initialization order relative to *this* variable's +// initializer, across translation units, was unspecified by the standard. If +// this initializer ran before the other translation unit's own static +// initialization got around to constructing `child_pointers`, the nested +// make_tracked() call inside StaticInitOuter's constructor +// would hit `Pointer`'s assertion: +// Assertion failed: (offset / 8 < pointers.map->size()), function Pointer, ... +// The fix converts `child_pointers` into a lazily-constructed function-local +// static (see page_info.h), which the C++ standard guarantees is initialized +// on first use regardless of static-initialization order. +// +// This is built as its own standalone test binary (see tests/CMakeLists.txt) +// rather than folded into the shared `tests` executable: constructing a +// tracked object during static initialization -- before any SGCL machinery +// has necessarily run, and before the collector has otherwise been touched +// by the process -- is a whole-program concern, and this reproduction should +// not share collector/thread state with the rest of the (much larger) test +// suite, which asserts exact collector::get_live_object_count() baselines +// throughout. +tracked_ptr g_static_init_outer = make_static_init_outer(); + +TEST(StaticInit_Tests, GlobalConstructedDuringStaticInitialization) { + ASSERT_NE(g_static_init_outer, nullptr); + EXPECT_EQ(g_static_init_outer->value, 7); + ASSERT_NE(g_static_init_outer->ptr, nullptr); + EXPECT_EQ(g_static_init_outer->ptr->value, 42); +} diff --git a/tests/static_init.h b/tests/static_init.h new file mode 100644 index 0000000..2403def --- /dev/null +++ b/tests/static_init.h @@ -0,0 +1,43 @@ +//------------------------------------------------------------------------------ +// SGCL: Smart Garbage Collection Library +// Copyright (c) 2022-2025 Sebastian Nibisz +// SPDX-License-Identifier: Apache-2.0 +//------------------------------------------------------------------------------ +#pragma once + +#include "sgcl/sgcl.h" + +#include + +using namespace sgcl; + +// Types used to reproduce the "first construction of a tracked type happens +// during static/dynamic initialization of a global object, before main()" +// scenario. StaticInitOuter is itself constructed for the first time inside +// another translation unit's global initializer, and its own constructor, in +// turn, triggers the first-ever construction of the nested tracked type +// StaticInitInner via a tracked_ptr field. This mirrors the real-world crash: +// a module-level global variable's initializer calling a factory function +// defined in a different translation unit, whose constructor nested- +// constructs yet another tracked type, all before main() runs. +struct StaticInitInner { + int value = 42; +}; + +struct StaticInitOuter { + int value; + tracked_ptr ptr; + + StaticInitOuter() { + value = 7; + ptr = make_tracked(); + } +}; + +// Defined in a separate translation unit (static_init_factory.cpp) so that +// the first instantiation/use of PageInfo and +// PageInfo happens in that other TU, while the call into it +// happens from this TU's own global variable initializer -- reproducing the +// cross translation-unit static-initialization-order dependency that the +// original bug exposed. +tracked_ptr make_static_init_outer(); diff --git a/tests/static_init_factory.cpp b/tests/static_init_factory.cpp new file mode 100644 index 0000000..89c737f --- /dev/null +++ b/tests/static_init_factory.cpp @@ -0,0 +1,14 @@ +//------------------------------------------------------------------------------ +// SGCL: Smart Garbage Collection Library +// Copyright (c) 2022-2025 Sebastian Nibisz +// SPDX-License-Identifier: Apache-2.0 +//------------------------------------------------------------------------------ +#include "static_init.h" + +// Defined in its own translation unit deliberately: this is where +// PageInfo/PageInfo get instantiated, kept +// separate from the translation unit that calls this function from a global +// variable's own dynamic initializer (see static_init.cpp). +tracked_ptr make_static_init_outer() { + return make_tracked(); +}