CODEWITHSUNDEEP
c++multithreadingthread-safetyshared-ptrsmart-pointers
Elad Maimoni
Elad Maimoni

Reputation: 4575

std::shared_ptr copy constructor thread safety

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:

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);

}

Upvotes: 2

Views: 2062

Answers (2)

Nicol Bolas
Nicol Bolas

Reputation: 473212

Manipulation of the state shared by shared_ptr objects is thread-safe; shared_ptr itself is not thread-safe. You cannot manipulate the same shared_ptr object from different threads at the same time; attempting to do so is a data race and thus UB.

So your code would be fine if lambda copied the pointer before being shipped off to another thread.

It should also be noted that your specific example could never work, no matter how shared_ptr was written. The type could be atomic<int> and it would still be just as broken. You gave the lambda a reference to an object that may not exist before the lambda gets to execute the copy operation.

No amount of internal thread-safety can save you there. Passing a reference to a stack variable to another thread should always be looked on as a code smell.

Upvotes: 3

Remy Lebeau
Remy Lebeau

Reputation: 595377

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.

Upvotes: 4

Related Questions