Fix data race in freeing control block
This commit is contained in:
@@ -172,19 +172,24 @@ private:
|
|||||||
*/
|
*/
|
||||||
void release() noexcept {
|
void release() noexcept {
|
||||||
if (control_block) {
|
if (control_block) {
|
||||||
|
// Prevent weak count from going to 0 concurrently
|
||||||
|
control_block->increment_weak();
|
||||||
|
|
||||||
uint32_t prev_strong = control_block->decrement_strong();
|
uint32_t prev_strong = control_block->decrement_strong();
|
||||||
|
|
||||||
// If this was the last strong reference, destroy the object
|
// If this was the last strong reference, destroy the object
|
||||||
if (prev_strong == 1) {
|
if (prev_strong == 1) {
|
||||||
T *obj = get();
|
T *obj = get();
|
||||||
obj->~T();
|
obj->~T();
|
||||||
|
}
|
||||||
|
|
||||||
// Check if there are any weak references
|
// Now release our temporary weak reference
|
||||||
uint32_t current_weak =
|
uint32_t prev_weak = control_block->decrement_weak();
|
||||||
control_block->weak_count.load(std::memory_order_acquire);
|
|
||||||
if (current_weak == 0) {
|
// If this was the last weak reference and we destroyed the object,
|
||||||
std::free(control_block);
|
// we can free the control block
|
||||||
}
|
if (prev_weak == 1 && prev_strong == 1) {
|
||||||
|
std::free(control_block);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -314,16 +319,14 @@ private:
|
|||||||
*/
|
*/
|
||||||
void release() noexcept {
|
void release() noexcept {
|
||||||
if (control_block) {
|
if (control_block) {
|
||||||
|
// Prevent strong count from going to 0 concurrently
|
||||||
|
auto strong = lock();
|
||||||
uint32_t prev_weak = control_block->decrement_weak();
|
uint32_t prev_weak = control_block->decrement_weak();
|
||||||
|
|
||||||
// If this was the last weak reference, check if we need to free control
|
// If this was the last weak reference, check if we need to free control
|
||||||
// block
|
// block
|
||||||
if (prev_weak == 1) {
|
if (prev_weak == 1 && !strong) {
|
||||||
uint32_t current_strong =
|
std::free(control_block);
|
||||||
control_block->strong_count.load(std::memory_order_acquire);
|
|
||||||
if (current_strong == 0) {
|
|
||||||
std::free(control_block);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
#include <barrier>
|
||||||
#include <doctest/doctest.h>
|
#include <doctest/doctest.h>
|
||||||
#include <latch>
|
#include <latch>
|
||||||
#include <thread>
|
#include <thread>
|
||||||
@@ -156,6 +157,63 @@ TEST_CASE("Ref thread safety") {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_CASE("Control block cleanup race condition test") {
|
||||||
|
// This test specifically targets the race condition where both
|
||||||
|
// the last strong reference and last weak reference are destroyed
|
||||||
|
// simultaneously, potentially causing double-free of control block
|
||||||
|
|
||||||
|
const int test_iterations = 10000;
|
||||||
|
|
||||||
|
// Shared state for passing references between threads
|
||||||
|
Ref<TestObject> ptr1;
|
||||||
|
WeakRef<TestObject> ptr2;
|
||||||
|
auto setup = [&]() {
|
||||||
|
ptr1 = make_ref<TestObject>(0);
|
||||||
|
ptr2 = ptr1;
|
||||||
|
};
|
||||||
|
|
||||||
|
// Barrier for synchronization - 2 participants (main thread + worker thread)
|
||||||
|
std::barrier sync_barrier{2};
|
||||||
|
|
||||||
|
std::thread worker_thread([&]() {
|
||||||
|
for (int iter = 0; iter < test_iterations; ++iter) {
|
||||||
|
// Wait for main thread to create the references
|
||||||
|
sync_barrier.arrive_and_wait();
|
||||||
|
|
||||||
|
// Worker thread destroys the weak reference simultaneously with main
|
||||||
|
// thread
|
||||||
|
ptr2.reset();
|
||||||
|
|
||||||
|
// Wait for next iteration
|
||||||
|
sync_barrier.arrive_and_wait();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
for (int iter = 0; iter < test_iterations; ++iter) {
|
||||||
|
// Create references
|
||||||
|
setup();
|
||||||
|
|
||||||
|
// Both threads are ready - synchronize for simultaneous destruction
|
||||||
|
sync_barrier.arrive_and_wait();
|
||||||
|
|
||||||
|
// Main thread destroys the strong reference at the same time
|
||||||
|
// as worker thread destroys the weak reference
|
||||||
|
ptr1.reset();
|
||||||
|
|
||||||
|
// Wait for both destructions to complete
|
||||||
|
sync_barrier.arrive_and_wait();
|
||||||
|
|
||||||
|
// Clean up for next iteration
|
||||||
|
ptr1.reset();
|
||||||
|
ptr2.reset();
|
||||||
|
}
|
||||||
|
|
||||||
|
worker_thread.join();
|
||||||
|
|
||||||
|
// If we reach here without segfault/double-free, the test passes
|
||||||
|
// The bug would manifest as a crash or memory corruption
|
||||||
|
}
|
||||||
|
|
||||||
TEST_CASE("WeakRef prevents circular references") {
|
TEST_CASE("WeakRef prevents circular references") {
|
||||||
SUBCASE("simple weak reference lifecycle") {
|
SUBCASE("simple weak reference lifecycle") {
|
||||||
WeakRef<TestObject> weak_ref;
|
WeakRef<TestObject> weak_ref;
|
||||||
|
|||||||
Reference in New Issue
Block a user