VC++ 2017 std::unique_ptr::reset implementation

[gicutza]

From VC++ 2017 15.7.1 sources for std::unique_ptr::reset :

void reset(pointer _Ptr = pointer()) _NOEXCEPT
{    // establish new pointer
    pointer _Old = get();
    this->_Myptr() = _Ptr;
    if (_Old != pointer())
    {
        this->get_deleter()(_Old);
    }
}

Sample code that will crash-boom-bang:

#include "stdafx.h"
#include <memory>
#include <thread>
#include <functional>
#include <mutex>
#include <atomic>
#include <iostream>

class A
{
public:
    A()
    {
        m_stop = false;
        m_thr = std::thread(&A::worker, this);
    }

    ~A()
    {
        m_stop = true;
        if (m_thr.joinable())
            m_thr.join();
    }

    void set_f(std::function<void()> f)
    {
        std::lock_guard<std::recursive_mutex> lock(m_mtx);
        m_f = std::move(f);
    }

    void print()
    {
        std::lock_guard<std::recursive_mutex> lock(m_mtx);
        std::cout << "print\n";
    }

private:
    void worker()
    {
        while (!m_stop)
            std::this_thread::sleep_for(std::chrono::milliseconds(100));
        call_f();
    }

    void call_f()
    {
        std::lock_guard<std::recursive_mutex> lock(m_mtx);
        if (m_f)
            m_f();
    }

private:
    std::atomic_bool m_stop;
    std::thread m_thr;
    std::recursive_mutex m_mtx;
    std::function<void()> m_f;
};

int main()
{
    auto p = std::make_unique<A>();
    p->set_f([&p]() { p->print(); });
    p.reset();

    return 0;
}

The code crash because:
- main thread execs reset
- the reset implementation puts the unique_ptr in empty state and calls A dtor
- when the program is in A dtor, other thread use that unique_ptr (see the sample) and the unique_ptris empty => crash
- but the A dtor waits for the other thread to end

I don't have libstdc++, but i found this: https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-4.4/a01404.html

void
00339       reset(pointer __p = pointer()) 
00340       {
00341     if (__p != get())
00342     {
00343       get_deleter()(get());
00344       std::get<0>(_M_t) = __p;
00345     }
00346       }

libstdc++ calls A dtor first and that implementation will not crash with my sample.

[gicutza]

crash call stack:

     msvcp140d.dll!0f563c93()    Unknown
     [Frames below may be incorrect and/or missing, no symbols loaded for msvcp140d.dll]    
     msvcp140d.dll!0f56425e()    Unknown
>    crash_up.exe!std::_Mtx_lockX(_Mtx_internal_imp_t * _Mtx) Line 72    C++
     crash_up.exe!std::_Mutex_base::lock() Line 49    C++
     crash_up.exe!std::lock_guard<std::recursive_mutex>::lock_guard<std::recursive_mutex>(std::recursive_mutex & _Mtx) Line 514    C++
     crash_up.exe!A::print() Line 33    C++
     crash_up.exe!main::__l2::<lambda>() Line 62    C++
     crash_up.exe!std::_Invoker_functor::_Call<void <lambda>(void) &>(main::__l2::void <lambda>(void) & _Obj)    C++
     crash_up.exe!std::invoke<void <lambda>(void) &>(main::__l2::void <lambda>(void) & _Obj)    C++
     crash_up.exe!std::_Invoker_ret<void,1>::_Call<void <lambda>(void) &>(main::__l2::void <lambda>(void) & <_Vals_0>)    C++
     crash_up.exe!std::_Func_impl_no_alloc<void <lambda>(void),void>::_Do_call()    C++
     crash_up.exe!std::_Func_class<void>::operator()()    C++
     crash_up.exe!A::call_f() Line 50    C++
     crash_up.exe!A::worker() Line 43    C++
     crash_up.exe!std::_Invoker_pmf_pointer::_Call<void (__thiscall A::*)(void),A *>(void(A::*)() _Pmf, A * && _Arg1)    C++
     crash_up.exe!std::invoke<void (__thiscall A::*)(void),A *>(void(A::*)() && _Obj, A * && <_Args_0>)    C++
     crash_up.exe!std::_LaunchPad<std::unique_ptr<std::tuple<void (__thiscall A::*)(void),A *>,std::default_delete<std::tuple<void (__thiscall A::*)(void),A *> > > >::_Execute<0,1>(std::tuple<void (__thiscall A::*)(void),A *> & _Tup, std::integer_sequence<unsigned int,0,1> __formal) Line 238    C++
     crash_up.exe!std::_LaunchPad<std::unique_ptr<std::tuple<void (__thiscall A::*)(void),A *>,std::default_delete<std::tuple<void (__thiscall A::*)(void),A *> > > >::_Run(std::_LaunchPad<std::unique_ptr<std::tuple<void (__thiscall A::*)(void),A *>,std::default_delete<std::tuple<void (__thiscall A::*)(void),A *> > > > * _Ln) Line 245    C++
     crash_up.exe!std::_LaunchPad<std::unique_ptr<std::tuple<void (__thiscall A::*)(void),A *>,std::default_delete<std::tuple<void (__thiscall A::*)(void),A *> > > >::_Go() Line 230    C++
     crash_up.exe!std::_Pad::_Call_func(void * _Data) Line 208    C++
     ucrtbased.dll!0f2f6a58()    Unknown
     ucrtbased.dll!0f2f6789()    Unknown
     kernel32.dll!773c7c04()    Unknown
     ntdll.dll!77c9ad2f()    Unknown
     ntdll.dll!77c9acfa()    Unknown

[s...@nuwen.net]

MSVC is precisely following N4741 23.11.1.2.5 [unique.ptr.single.modifiers]/4 "Effects: Assigns p to the stored pointer, and then if and only if the old value of the stored pointer, old_p, was not equal to nullptr, calls get_deleter()(old_p). [ Note: The order of these operations is significant because the call to get_deleter() may destroy *this. —end note ]".

Stephan T. Lavavej

Principal Software Engineer, Visual C++ Libraries

[Ville Voutilainen]

On 16 May 2018 at 21:44, <s...@nuwen.net> wrote:
> MSVC is precisely following N4741 23.11.1.2.5
> [unique.ptr.single.modifiers]/4 "Effects: Assigns p to the stored pointer,
> and then if and only if the old value of the stored pointer, old_p, was not
> equal to nullptr, calls get_deleter()(old_p). [ Note: The order of these
> operations is significant because the call to get_deleter() may destroy
> *this. —end note ]".

libstdc++ does the same, the code quoted is old. The sample segfaults
all the way back to GCC 4.8, which
is far beyond supported.

[gicutza]

if I don't use unique_ptr in main then there is no crash :)

int main()
{
    auto p = new A;

    p->set_f([&p]() { p->print(); });

    delete p;

    return 0;
}

So, I don't understand where is the defect.

[Tony V E]

The defect is in your code.

The main thread is writing to p while the worker thread is using p. That's a race.

For example, you could be just starting the call to m_f() when p is reset. Now the lambda does p-> and there is nothing there.

Sent from my BlackBerry portable Babbage Device

From: 'gicutza' via ISO C++ Standard - Discussion Sent: Wednesday, May 16, 2018 3:37 PM To: ISO C++ Standard - Discussion Reply To: std-dis...@isocpp.org Cc: danel...@yahoo.com Subject: [std-discussion] Re: VC++ 2017 std::unique_ptr::reset implementation

--

---
You received this message because you are subscribed to the Google Groups "ISO C++ Standard - Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to std-discussio...@isocpp.org.
To post to this group, send email to std-dis...@isocpp.org.
Visit this group at https://groups.google.com/a/isocpp.org/group/std-discussion/.

[danel...@yahoo.com]

I know why the code is crashing. At first I did not know it and I found out after seeing the reset implementation. My sample is extracted from a more complex code and the easy fix for me was to not use unique_ptr.
The code is protected to not use the pointer after is deleted and is work fine in the second version, without unique_ptr.
I don't like in the first version, with unique_ptr, because I must know the implementation of unique_ptr::reset to avoid the crash. But, I don't really want to know every implementation from stl.

[Ville Voutilainen]

So, for those with an archaeological mind:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43183

[Tony V E]

You don't really need to know the implementation of unique_ptr - the data race exists regardless of implementation, it just might not manifest itself.

You have two threads accessing/modifying p without a lock protecting p.

You can still use unique_ptr for lifetime, but capture the raw pointer in the lambda. But it is subtle, so needs a comment:

int main()
{
    auto p = std::make_unique<A>();

    // unique_ptr is not protected by lock,

    // but raw pointer is (internally)

    // so only use raw pointer in lambda‎:
    p->set_f([p=p.get()]() { p->print(); });
    p.reset();

    return 0;
}

Sent from my BlackBerry portable Babbage Device

	Original Message

From: danel_danel via ISO C++ Standard - Discussion Sent: Thursday, May 17, 2018 2:00 AM

To: ISO C++ Standard - Discussion Reply To: std-dis...@isocpp.org Cc: danel...@yahoo.com

Subject: Re: [std-discussion] Re: VC++ 2017 std::unique_ptr::reset implementation