fix(asan): pointer used after free in autoref_ptr_test

[foreverneverer]

It reports heap-use-after-free error that the pointer self_ptr_ is used after free in autoref_ptr_test.ScopedRefPtrToSelfPointerAssignment when run test with AddressSanitizer tool.

Coredump:

#0 0x4cfb64 in operator= /home/mi/work/PegasusDB/pegasus/rdsn/include/dsn/utility/autoref_ptr.h:155
#1 0x4cfb64 in RefCountedUnitTest_ScopedRefPtrToSelfPointerAssignment_Test::TestBody() /home/mi/work/PegasusDB/pegasus/rdsn/src/core/tests/autoref_ptr_test.cpp:157
#2 0x6aab1e in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/mi/work/PegasusDB/pegasus/rdsn/builder/src/core/tests/dsn.core.tests+0x6aab1e)

Related code:

rdsn/src/core/tests/autoref_ptr_test.cpp

Line 157 in b35cf26

check->self_ptr_ = nullptr;

TEST(RefCountedUnitTest, ScopedRefPtrToSelfPointerAssignment)
{
    ScopedRefPtrToSelf::reset_was_destroyed();

    ScopedRefPtrToSelf *check = new ScopedRefPtrToSelf();
    EXPECT_FALSE(ScopedRefPtrToSelf::was_destroyed());
    check->self_ptr_ = nullptr;
    EXPECT_TRUE(ScopedRefPtrToSelf::was_destroyed());
}

rdsn/include/dsn/utility/autoref_ptr.h

Line 155 in 8095786

_obj = obj;

 146     ref_ptr<T> &operator=(T *obj)
 147     {
 148         if (_obj == obj)
 149             return *this;
 150 
 151         if (nullptr != _obj) {
 152             _obj->release_ref(); // Call ~ScopedRefPtrToSelf, then call ~ref_ptr(). `this` is freed.
 153         }
 154 
 155         _obj = obj; // `this` was freed, memory of _obj was freed, it's illegal to be accessed now.
 156 
 157         if (obj != nullptr) {
 158             _obj->add_ref();
 159         }
 160 
 161         return *this;
 162     }

Solution

In the semantic of shared_ptr::operator=, ScopedRefPtrToSelf should be deleted after the call.

std::shared_ptr<T>::operator=
If *this already owns an object and it is the last shared_ptr owning it, and r is not the same as *this, the object is destroyed through the owned deleter.

Therefore we should not use this pointer after we call release_ref, until the last line return *this, because C++ compiler will run RVO to eliminate the return.

[neverchanje]

autoref_ptr_test is mostly copied from chromium. Be sure we keep consistent with those codes unless they are confirmed incorrect.

[levy5307]

autoref_ptr_test is mostly copied from chromium. Be sure we keep consistent with those codes unless they are confirmed incorrect.

这个问题昨天我和贾硕一起看的，确实是有问题的。
ScopedRefPtrToSef这个类型里有一个指向自己的ref_ptr，其结构如下图：

                          _______    <----------------------------
                         |_______|                                 |
                         |_______|                     ____        |
                         |_______|                    |_obj_|------|
             ref_ptr类型 |_sef_ptr_|------------------>|_____|
                         |_______|                    |_____|
                         |_______|                    

在ScopedRefPtrToSelfPointerAssignment这个单测里，代码如下：

    ScopedRefPtrToSelf::reset_was_destroyed();
    ScopedRefPtrToSelf *check = new ScopedRefPtrToSelf();
    EXPECT_FALSE(ScopedRefPtrToSelf::was_destroyed());
    check->self_ptr_ = nullptr;
    EXPECT_TRUE(ScopedRefPtrToSelf::was_destroyed());

在执行到check->self_ptr = nullptr的时候，会调用ScopedRefPtrToSelf的=运算符重载：

    ref_ptr<T> &operator=(T *obj)
    {
        if (_obj == obj)
            return *this;
        if (nullptr != _obj) {                      **// label A**
            _obj->release_ref();
        }
        _obj = obj;                                  **// label B**
        if (obj != nullptr) {
            _obj->add_ref();
        }
        return *this;
    }

在执行到label A的时候，此时执行了_obj->release_ref()将自己释放了。然后在执行到label B的时候，会发现使用了已经释放的对象的成员，所以就报错了

[qinzuoyan]

在我看来，合理的做法应当是修改ref_counter类，而不是改测试代码。这里的测试代码，就是为了测试在selfAssign情况下ref_counter能否正常工作。

修改如下：保证release_ref()在最后被调用，避免release自己后，再assign给自己

    ref_ptr<T> &operator=(T *obj)
    {
        if (_obj == obj)
            return *this;
        T* old = _obj;
        _obj = obj;
        if (_obj != nullptr) {
            _obj->add_ref();
        }
        if (old != nullptr) {
            old->release_ref();
        }
        return *this;
    }

同理，其他类似函数也应当修改，譬如ref_ptr<T> &operator=(ref_ptr<T> &&obj)

[levy5307]

在我看来，合理的做法应当是修改obj_counter类，而不是改测试代码。这里的测试代码，就是为了测试在selfAssign情况下obj_counter能否正常工作。

修改如下：保证release_ref()在最后被调用，避免release自己后，再assign给自己

    ref_ptr<T> &operator=(T *obj)
    {
        if (_obj == obj)
            return *this;
        T* old = _obj;
        _obj = obj;
        if (_obj != nullptr) {
            _obj->add_ref();
        }
        if (old != nullptr) {
            old->release_ref();
        }
        return *this;
    }

同理，其他类似函数也应当修改，譬如ref_ptr<T> &operator=(ref_ptr<T> &&obj)

这样改，return *this的时候会不会有问题呢？同样也是访问了已释放对象的成员(this)

[qinzuoyan]

按理这样用也不对。但是返回解引用，但是没有操作数据，还是安全得多。如果使用者的方式没有问题的话，返回的引用就不应该再被使用，也就不会有问题。

[vagetablechicken]

https://git.szzt.com/yixing/ticai_src/blob/138090dfc3e89d1e3cff80f2d64f60dc3cdf8281/LA.UM.5.6/LINUX/android/external/libweave/third_party/chromium/base/memory/ref_counted_unittest.cc#L137

[foreverneverer]

https://git.szzt.com/yixing/ticai_src/blob/138090dfc3e89d1e3cff80f2d64f60dc3cdf8281/LA.UM.5.6/LINUX/android/external/libweave/third_party/chromium/base/memory/ref_counted_unittest.cc#L137

我直接恢复成原来的单测代码了，按照@qinzuoyan的修改可以fix

[foreverneverer]

https://chromium.googlesource.com/chromium/src/+/master/base/memory/scoped_refptr.h

chromium 的ref_ptr实现

[acelyc111]

https://chromium.googlesource.com/chromium/src/+/master/base/memory/scoped_refptr.h

chromium 的ref_ptr实现

最新版实现的真够简洁又隐晦的

[neverchanje]

  ref_ptr& operator=(T* p) {
    return *this = ref_ptr(p); // Call `ref_ptr& operator=(ref_ptr p)`.
  }
 
  ref_ptr& operator=(ref_ptr r) noexcept {
    std::swap(ptr_, r.ptr_);
    return *this;
  }

为什么不用这种写法？这种写法同样可以保证一直到 return *this 的时候才会访问到 this 的内存，而且更简单。

[foreverneverer]

  ref_ptr& operator=(T* p) {
    return *this = ref_ptr(p); // Call `ref_ptr& operator=(ref_ptr p)`.
  }
 
  ref_ptr& operator=(ref_ptr r) noexcept {
    std::swap(ptr_, r.ptr_);
    return *this;
  }

为什么不用这种写法？这种写法同样可以保证一直到 return *this 的时候才会访问到 this 的内存，而且更简单。

恩，我再改一下代码