Fix polymorphic WeakRef bug

This commit is contained in:
2025-09-12 12:05:07 -04:00
parent f89868058a
commit 543447971f
2 changed files with 68 additions and 19 deletions

View File

@@ -221,7 +221,7 @@ template <typename T> struct Ref {
if (control_block) { if (control_block) {
control_block->increment_weak(); control_block->increment_weak();
} }
return WeakRef<T>(control_block); return WeakRef<T>(ptr, control_block);
} }
/** /**
@@ -328,7 +328,7 @@ template <typename T> struct WeakRef {
expected_strong, expected_strong + 1, std::memory_order_acquire, expected_strong, expected_strong + 1, std::memory_order_acquire,
std::memory_order_relaxed)) { std::memory_order_relaxed)) {
// Success - we incremented the strong count // Success - we incremented the strong count
return Ref<T>(get_object_ptr(), control_block); return Ref<T>(ptr, control_block);
} }
// CAS failed, expected_strong now contains the current value, retry // CAS failed, expected_strong now contains the current value, retry
} }
@@ -384,7 +384,8 @@ template <typename T> struct WeakRef {
template <typename U> template <typename U>
WeakRef(WeakRef<U> &&other) noexcept WeakRef(WeakRef<U> &&other) noexcept
requires std::is_convertible_v<U *, T *> requires std::is_convertible_v<U *, T *>
: control_block(other.control_block) { : ptr(other.ptr), control_block(other.control_block) {
other.ptr = nullptr;
other.control_block = nullptr; other.control_block = nullptr;
} }
@@ -396,7 +397,9 @@ template <typename T> struct WeakRef {
requires std::is_convertible_v<U *, T *> requires std::is_convertible_v<U *, T *>
{ {
release(); release();
ptr = other.ptr;
control_block = other.control_block; control_block = other.control_block;
other.ptr = nullptr;
other.control_block = nullptr; other.control_block = nullptr;
return *this; return *this;
} }
@@ -416,7 +419,9 @@ template <typename T> struct WeakRef {
/** /**
* @brief Move constructor * @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; other.control_block = nullptr;
} }
@@ -426,7 +431,9 @@ template <typename T> struct WeakRef {
WeakRef &operator=(WeakRef &&other) noexcept { WeakRef &operator=(WeakRef &&other) noexcept {
if (this != &other) { if (this != &other) {
release(); release();
ptr = other.ptr;
control_block = other.control_block; control_block = other.control_block;
other.ptr = nullptr;
other.control_block = nullptr; other.control_block = nullptr;
} }
return *this; return *this;
@@ -440,7 +447,7 @@ template <typename T> struct WeakRef {
if (control_block) { if (control_block) {
control_block->increment_weak(); control_block->increment_weak();
} }
return WeakRef(control_block); return WeakRef(ptr, control_block);
} }
/** /**
@@ -448,31 +455,22 @@ template <typename T> struct WeakRef {
*/ */
void reset() noexcept { void reset() noexcept {
release(); release();
ptr = nullptr;
control_block = nullptr; control_block = nullptr;
} }
/** /**
* @brief Default constructor - creates empty WeakRef * @brief Default constructor - creates empty WeakRef
*/ */
WeakRef() : control_block(nullptr) {} WeakRef() : ptr(nullptr), control_block(nullptr) {}
private: 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; 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<T *>(reinterpret_cast<char *>(control_block) +
padded_cb_size);
}
/** /**
* @brief Release current weak reference and handle cleanup * @brief Release current weak reference and handle cleanup
*/ */

View File

@@ -41,6 +41,26 @@ struct AnotherDerived : public Base {
: Base(base_v), another_value(another_v) {} : Base(base_v), another_value(another_v) {}
int get_value() const override { return base_value * another_value; } 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 } // anonymous namespace
TEST_CASE("Ref basic functionality") { 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_value() == 10); // 5 + 5
CHECK(base_ref_from_weak.get() == derived_ref.get()); CHECK(base_ref_from_weak.get() == derived_ref.get());
} }
SUBCASE("multiple inheritance pointer address bug test") {
auto multi_ref = make_ref<MultipleInheritance>(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<void *>(interface1_ptr) !=
static_cast<void *>(interface2_ptr));
// Create WeakRef to Interface2 (which has a different pointer address)
WeakRef<Interface2> 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<Interface1> 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 // Should be run with asan or valgrind