From 543447971fb0c6cbbe408528bccedfc376e5154a Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Fri, 12 Sep 2025 12:05:07 -0400 Subject: [PATCH] Fix polymorphic WeakRef bug --- src/reference.hpp | 36 ++++++++++++++-------------- tests/test_reference.cpp | 51 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 19 deletions(-) diff --git a/src/reference.hpp b/src/reference.hpp index 714c6a3..ed663dd 100644 --- a/src/reference.hpp +++ b/src/reference.hpp @@ -221,7 +221,7 @@ template struct Ref { if (control_block) { control_block->increment_weak(); } - return WeakRef(control_block); + return WeakRef(ptr, control_block); } /** @@ -328,7 +328,7 @@ template struct WeakRef { expected_strong, expected_strong + 1, std::memory_order_acquire, std::memory_order_relaxed)) { // Success - we incremented the strong count - return Ref(get_object_ptr(), control_block); + return Ref(ptr, control_block); } // CAS failed, expected_strong now contains the current value, retry } @@ -384,7 +384,8 @@ template struct WeakRef { template WeakRef(WeakRef &&other) noexcept requires std::is_convertible_v - : control_block(other.control_block) { + : ptr(other.ptr), control_block(other.control_block) { + other.ptr = nullptr; other.control_block = nullptr; } @@ -396,7 +397,9 @@ template struct WeakRef { requires std::is_convertible_v { release(); + ptr = other.ptr; control_block = other.control_block; + other.ptr = nullptr; other.control_block = nullptr; return *this; } @@ -416,7 +419,9 @@ template struct WeakRef { /** * @brief Move constructor */ - WeakRef(WeakRef &&other) noexcept : control_block(other.control_block) { + WeakRef(WeakRef &&other) noexcept + : ptr(other.ptr), control_block(other.control_block) { + other.ptr = nullptr; other.control_block = nullptr; } @@ -426,7 +431,9 @@ template struct WeakRef { WeakRef &operator=(WeakRef &&other) noexcept { if (this != &other) { release(); + ptr = other.ptr; control_block = other.control_block; + other.ptr = nullptr; other.control_block = nullptr; } return *this; @@ -440,7 +447,7 @@ template struct WeakRef { if (control_block) { control_block->increment_weak(); } - return WeakRef(control_block); + return WeakRef(ptr, control_block); } /** @@ -448,31 +455,22 @@ template struct WeakRef { */ void reset() noexcept { release(); + ptr = nullptr; control_block = nullptr; } /** * @brief Default constructor - creates empty WeakRef */ - WeakRef() : control_block(nullptr) {} + WeakRef() : ptr(nullptr), control_block(nullptr) {} private: - explicit WeakRef(detail::ControlBlock *cb) : control_block(cb) {} + explicit WeakRef(T *object_ptr, detail::ControlBlock *cb) + : ptr(object_ptr), control_block(cb) {} + T *ptr; detail::ControlBlock *control_block; - // Helper to calculate object pointer from control block - T *get_object_ptr() const { - if (!control_block) - return nullptr; - constexpr size_t cb_size = sizeof(detail::ControlBlock); - constexpr size_t alignment = alignof(T); - constexpr size_t padded_cb_size = - (cb_size + alignment - 1) & ~(alignment - 1); - return reinterpret_cast(reinterpret_cast(control_block) + - padded_cb_size); - } - /** * @brief Release current weak reference and handle cleanup */ diff --git a/tests/test_reference.cpp b/tests/test_reference.cpp index e4d41ae..957d478 100644 --- a/tests/test_reference.cpp +++ b/tests/test_reference.cpp @@ -41,6 +41,26 @@ struct AnotherDerived : public Base { : Base(base_v), another_value(another_v) {} int get_value() const override { return base_value * another_value; } }; + +// Classes to test polymorphic pointer address changes +struct Interface1 { + int interface1_data = 1; + virtual ~Interface1() = default; + virtual int get_interface1() const { return interface1_data; } +}; + +struct Interface2 { + int interface2_data = 2; + virtual ~Interface2() = default; + virtual int get_interface2() const { return interface2_data; } +}; + +// Multiple inheritance - this will cause pointer address changes +struct MultipleInheritance : public Interface1, public Interface2 { + int own_data; + explicit MultipleInheritance(int data) : own_data(data) {} + int get_own_data() const { return own_data; } +}; } // anonymous namespace TEST_CASE("Ref basic functionality") { @@ -444,6 +464,37 @@ TEST_CASE("Polymorphic edge cases") { CHECK(base_ref_from_weak->get_value() == 10); // 5 + 5 CHECK(base_ref_from_weak.get() == derived_ref.get()); } + + SUBCASE("multiple inheritance pointer address bug test") { + auto multi_ref = make_ref(42); + + // Get pointers to different base classes - these will have different + // addresses + Interface1 *interface1_ptr = multi_ref.get(); + Interface2 *interface2_ptr = multi_ref.get(); + MultipleInheritance *multi_ptr = multi_ref.get(); + + // Verify that pointers are indeed different (demonstrating the issue) + CHECK(static_cast(interface1_ptr) != + static_cast(interface2_ptr)); + + // Create WeakRef to Interface2 (which has a different pointer address) + WeakRef weak_interface2 = multi_ref.as_weak(); + + // Lock should return the correct Interface2 pointer, not miscalculated one + auto locked_interface2 = weak_interface2.lock(); + CHECK(locked_interface2); + CHECK(locked_interface2.get() == + interface2_ptr); // This might fail due to the bug! + CHECK(locked_interface2->get_interface2() == 2); + + // Also test Interface1 + WeakRef weak_interface1 = multi_ref.as_weak(); + auto locked_interface1 = weak_interface1.lock(); + CHECK(locked_interface1); + CHECK(locked_interface1.get() == interface1_ptr); // This might also fail! + CHECK(locked_interface1->get_interface1() == 1); + } } // Should be run with asan or valgrind