std::shared_ptr copy constructor thread safety

Question

std::shared_ptr specification guarentees that only one thread will invoke delete on the internal pointer. This answer has a really nice explanation about the required memory ordering on the shared_ptr refrence count manipulation in order to guarentee that the delete will be invoked on a synchronized memory.

What I don't understand is the following:

If a shared_ptr is initialized by a copy constructor, is it guarenteed that it will either be empty or a valid shared_ptr?

I am looking at MVCC implementation of shared_ptr copy constructor. and I think I can identify at least one race condition.

template<class _Ty2>
    void _Copy_construct_from(const shared_ptr<_Ty2>& _Other)
    {   // implement shared_ptr's (converting) copy ctor
    if (_Other._Rep)
        {
        _Other._Rep->_Incref();
        }

    _Ptr = _Other._Ptr;
    _Rep = _Other._Rep;
    }

The implementation checks that the control block is valid, then incerement its reference count, and copy assigns the internal fields.

Assuming _Other is owned by a different thread then the one calling the copy constructor. If between the lines if (_Other._Rep) and _Other._Rep->_Incref(); this thread calls the destructor that happens to delete the control block and the pointer, then _Other._Rep->_Incref() will dereference a deleted pointer.

Further Clarification

Here is a code that illustrates the corner case I am talking about. I will tweak share_ptr copy constructor implementation to simulate a context switch:

template<class _Ty2>
    void _Copy_construct_from(const shared_ptr<_Ty2>& _Other)
    {   // implement shared_ptr's (converting) copy ctor
    if (_Other._Rep)
        {
        // now lets put here a really long loop or sleep to simulate a context switch
        int count = 0;
        for (int i = 0; i < 99999999; ++i)
        {           
            for (int j = 0; j < 99999999; ++j)
            {
              count++;
           }
        }

        // by the time we get here, the owning thread may already destroy the  shared_ptr that was passed to this constructor
        _Other._Rep->_Incref();
        }

    _Ptr = _Other._Ptr;
    _Rep = _Other._Rep;
    }

And here is a code that will probably show the problem:

int main()
{
    {
        std::shared_ptr<int> sh1 = std::make_shared<int>(123);
        auto lambda = [&]()
        {
            auto sh2 = sh1;
            std::cout << sh2.use_count(); // this prints garbage, -572662306 in my case
        };

        std::thread t1(lambda);
        t1.detach();
        // main thread destroys the shared_ptr
        // background thread probably did not yet finished executing the copy constructor
    }



    Sleep(10000);

}

Are you suggesting that an object can be copied in one thread while the same object is being destroyed in another? — curiousguy
"now lets put here a really long loop or sleep to simulate a context switch" int count = 0; make it volatile: the compiler is only expected to produce the same observable behavior of the program, that is the same interactions with the external world as those of an execution of the virtual machine as described by the C++ specification. An empty loop has no observable behavior at all, it can be removed. An access to a volatile qualified object by definition is an observable and thus a loop accessing such object cannot be removed. — curiousguy

Remy Lebeau Remy Lebeau · Accepted Answer · 2019-01-20T19:02:44

When shared_ptr is used properly, what your describe can never happen.

The shared_ptr that is being copied from has incremented the refcount before being passed to the copy constructor, and it can't be destructed until after the copy constructor has exited since it is a local parameter of the constructor.

Thus, another thread will not destroy the object that is being shared. The refcount of _Other.Rep will always be at least 1 upon entry into the copy constructor if _Other.Rep is not null.

UPDATE: your use case is faulty. The lambda captures a reference to the main thread's shared_ptr instance but the thread does not make a copy of that shared_ptr until after it has already gone out of scope and been destroyed by main. Your thread has a dangling reference causing your code to have undefined behavior. That is not a fault of the shared_ptr implementation. Your lambda needs to capture the shared_ptr by value instead of by reference instead, so its refcount is incremented immediately, before the thread is created, not when the thread starts running.

std::shared_ptr copy constructor thread safety

Further Clarification

2 Answers