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

From: Howard Hinnant (howard.hinnant_at_[hidden])
Date: 2007-04-14 19:12:55

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.
>> Usually it
>> should be either destructed or assigned to shortly after the move
>> constructor is called, in which case all will be well.  However,  
>> if it
>> remains unchanged then there is the danger that someone might try to
>> dereference it after r.px has been deleted.  This, of course, could
>> result in a hard-to-track access violation.  To see what I mean,
>> consider the following rather silly code:
>>
>>     shared_ptr<int> r(new int(1));
>>     shared_ptr<int> s(std::move(r));
>>     s.reset();
>>     if (r) { //r.get() != 0, so this returns true
>>        cout << *r; // Uh-oh
>>     }
>
> The problem with the above code does indeed exist, but it is not a  
> bug in
> the implementation (let alone a subtle one) - in the sense that the
> implementation is deliberately written in the way it's written, to not
> provide a guarantee about the consistency of r after the move. You  
> are right
> that we can clear r.px easily for shared_ptr, but it's more  
> important to
> think about the general case: is the user allowed to assume  
> anything about
> the state of an object after it has been moved from?
>
> void f( T t1 )
> {
>     T t2( std::move( t1 ) );
>     // What happens if I use t1 here?
> }
>
> Using t1 after the move looks like a programming error to me.

Fwiw Peter asked me to look at this.  Imho after a move from an  
object it should be in a self-consistent state.  All internal  
invariants should hold.  This answer is purposefully vague, and could  
be used to support either outcome.

A very reasonable invariant for a shared_ptr r would be that if (r)  
then *r is safe.

Another very reasonable invariant for a shared_ptr r would be that  
after a move, if (r) does not imply that you can *r.

There is nothing in the rvalue ref proposals that really nails this  
down.  It is up to each class to define its behavior after a move.   
At the very least it should support assignment and destruction in  
order to work with generic code.  Beyond that I'd say the class  
should be free to implement whatever semantics / interface it feels  
best.

In this particular case, I think it might be worth the extra  
assignment to null r.px just to make the class invariant simpler.   
However that statement falls well short of saying that the standard  
should require such behavior, or even that this behavior isn't ok  
with respect to putting this shared_ptr into std::containers or using  
with std::algorithms.  Furthermore I'm anxious to see what "best  
practices" in this area are born out by field experience.  And boost  
is a great place to have such experiments! :-)

-Howard