<memory>: weak_ptr conversions don't preserve control blocks for expired objects

[StephanTLavavej]

Bug Report

C:\Temp>type weakptrtest.cpp

#include <iostream>
#include <memory>

template <typename T, typename W>
bool EqualOwnership(const std::weak_ptr<T>& p, const std::weak_ptr<W>& q)
{
        return !p.owner_before(q) && !q.owner_before(p);
}

template <typename T>
bool IsEmpty(const std::weak_ptr<T>& p)
{
        return EqualOwnership(p, std::weak_ptr<T>{});
}

class Base {};
class Derived : public Base {};

int main()
{
        std::weak_ptr<Derived> wpDerived;

        {
                std::shared_ptr<Derived> sp = std::make_shared<Derived>();
                wpDerived = sp;
        }

        if (IsEmpty(wpDerived))
        {
                std::cout << "This line isn't printed, which is good, wpDerived should not be empty even though it is expired \n";
        }

        // Test copy constructing a pointer of same type
        std::weak_ptr<Derived> wpDerived_CopyConstruct(wpDerived);
        if (IsEmpty(wpDerived_CopyConstruct) || !EqualOwnership(wpDerived_CopyConstruct, wpDerived))
        {
                std::cout << "This line isn't printed, which is good, copy construction should not change ownership \n";
        }

        // Test assigning to a pointer of same type
        std::weak_ptr<Derived> wpDerived_Assign;
        wpDerived_Assign = wpDerived;
        if (IsEmpty(wpDerived_Assign) || !EqualOwnership(wpDerived_Assign, wpDerived))
        {
                std::cout << "This line isn't printed, which is good, assignment should not change ownership \n";
        }

        // Test copy constructing a base pointer
        std::weak_ptr<Base> wpBase_CopyConstruct(wpDerived);
        if (IsEmpty(wpBase_CopyConstruct))
        {
                std::cout << "Error! Copy construction from a non-empty pointer should not result in an empty pointer \n";
        }

        if (!EqualOwnership(wpBase_CopyConstruct, wpDerived))
        {
                std::cout << "Error! Copy construction should not change ownership \n";
        }

        // Test assigning to a base pointer
        std::weak_ptr<Base> wpBase_Assign;
        wpBase_Assign = wpDerived;
        if (IsEmpty(wpBase_Assign))
        {
                std::cout << "Error! Assignment from a non-empty pointer should not result in an empty pointer \n";
        }

        if (!EqualOwnership(wpBase_Assign, wpDerived))
        {
                std::cout << "Error! Assignment should not change ownership \n";
        }
}

C:\Temp>cl /EHsc /nologo /W4 weakptrtest.cpp
weakptrtest.cpp

C:\Temp>weakptrtest
Error! Copy construction from a non-empty pointer should not result in an empty pointer
Error! Copy construction should not change ownership
Error! Assignment from a non-empty pointer should not result in an empty pointer
Error! Assignment should not change ownership

Originally reported as DevCom-1127204. Also tracked by VSO-1163049 / AB#1163049.

Analysis

This happens because weak_ptr contains the following code:

STL/stl/inc/memory

Lines 2269 to 2272 in 35ce1cf

	template <class _Ty2, enable_if_t<_SP_pointer_compatible<_Ty2, _Ty>::value, int> = 0>
	weak_ptr(const weak_ptr<_Ty2>& _Other) noexcept { // construct weak_ptr object for resource pointed to by _Other
	this->_Weakly_construct_from(_Other.lock());
	}

STL/stl/inc/memory

Lines 2278 to 2282 in 35ce1cf

	template <class _Ty2, enable_if_t<_SP_pointer_compatible<_Ty2, _Ty>::value, int> = 0>
	weak_ptr(weak_ptr<_Ty2>&& _Other) noexcept { // move construct from _Other
	this->_Weakly_construct_from(_Other.lock());
	_Other.reset();
	}

The reason for _Other.lock() is extremely subtle, to handle the following scenario:

STL/tests/std/tests/Dev10_851347_weak_ptr_virtual_inheritance/test.cpp

Lines 10 to 51 in 35ce1cf

	struct A {
	int a;
	};
	struct B : virtual public A {
	int b;
	};
	struct C : virtual public A {
	int c;
	};
	struct D : public B, public C {
	int d;
	};
	int main() {
	shared_ptr<D> spd(new D);
	weak_ptr<D> wpd(spd);
	weak_ptr<D> wpd2(spd);
	spd.reset();
	weak_ptr<A> wpa1(wpd);
	assert(wpa1.expired());
	weak_ptr<A> wpa2;
	wpa2 = wpd;
	assert(wpa2.expired());
	weak_ptr<A> wpa3(move(wpd));
	assert(wpa3.expired());
	weak_ptr<A> wpa4;
	wpa4 = move(wpd2);
	assert(wpa4.expired());
	}

That is, when converting weak_ptr<Source> to weak_ptr<Destination>, we have to convert Source* to Destination* although the Source object may have been destroyed. Usually, this pointer conversion can be performed with compile-time information, as either no pointer adjustment is necessary (e.g. conversions to void*, conversions adding const, conversions in linear inheritance scenarios), or a statically-known offset needs to be added (e.g. conversions in multiple inheritance scenarios).

However, sometimes the pointer conversion needs run-time information requiring the Source object to be alive. This happens in virtual inheritance scenarios, where the offset from Source to Destination depends on the layout of the most-derived object. The Standardese for this is WG21-N4861 [basic.life]/6.3.

The weak_ptr Standardese doesn't mention virtual inheritance at all, which is potentially a defect. However, it may be possible to make everything work, without adding a special case to the Standardese or requesting a compiler builtin to detect when pointer conversions would involve traversing virtual bases.

I believe the key is that while weak_ptr's control block can be observed through owner_before() as in the test case above, the stored pointer can't be directly observed. The only way to observe it (which is why we need to store it) is by lock()ing to shared_ptr. That's specified as expired() ? shared_ptr<T>() : shared_ptr<T>(*this), and expired() is use_count() == 0, and use_count() returns "0 if *this is empty; otherwise, the number of shared_ptr instances that share ownership with *this".

So, when the weak_ptr is empty (no control block) or there are no shared_ptr instances left (the object is dead), then expired() is true and lock() returns shared_ptr<T>() with both a null stored pointer and no control block. So, we don't need to convert the addresses of dead objects, as the result is unobservable.

I believe we can fix this bug by:

• If there's no control block, then store a null object pointer and null control block.
• Otherwise, attempt to lock() and then:
  • If we're successful, do what we're doing today - convert the object pointer while it's guaranteed to be alive, and add a weak reference to the control block.
  • If we're unsuccessful, store a null object pointer (as the value doesn't matter), and add a weak reference to the control block (fixing this bug).

(We can probably write this directly, like _Construct_from_weak and _Weakly_construct_from.)

A compiler builtin would make this more efficient (no reason to lock() in non-virtual-inheritance scenarios) but I don't think it's needed for correctness.