Re: [boost] [shared_ptr] dangerous implementation of move constructor

From: Peter Dimov (pdimov_at_[hidden])
Date: 2007-04-14 19:35:11

Howard Hinnant wrote:
> On Apr 14, 2007, at 1:28 PM, Peter Dimov wrote:
>
>> Joe Gottman wrote:
>>>    I see that rvalue reference support has just been added to the
>>> shared_ptr template, and I think that there's a subtle bug in the
>>> implementation.  It is implemented as follows:
>>>
>>> shared_ptr( shared_ptr && r ): px( r.px ), pn() // never throws
>>>        {
>>>            pn.swap( r.pn );
>>>        }
>>>
>>> The problem is that after the move constructor has been called the
>>> moved -from shared_ptr, r, is in a dangerously inconsistent state.
>>> Its has an empty shared count, but it contains a non-null pointer.

[...]

> In this particular case, I think it might be worth the extra
> assignment to null r.px just to make the class invariant simpler.

I don't mind clearing r.px in the Boost implementation of the move 
constructors, but I'm not sure whether the specification ought to mandate 
it. The current implementation is a reformulation of

shared_ptr( shared_ptr && r ): px( move(r.px) ), pn( move(r.pn) )
{
}

I'm not sure that we want to prohibit it.

If we decide to clear r.px, there's also the matter of the move assignments. 
One obvious implementation of

shared_ptr& operator=( shared_ptr && r );

is to just call swap( r ). I'm not positive that we want to prohibit that, 
either.

We can enforce a postcondition of "r is empty" for the assignments too by 
using shared_ptr( r ).swap( *this ) as usual, but this strikes me as 
overspecification.

FWIW, shared_ptr by itself does not have an invariant that ties the pointer 
(px) and the ownership group (pn) together, although it's pretty common for 
the surrounding code to enforce or assume such an invariant. So I could go 
either way on that one.