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

From: Peter Dimov (pdimov_at_[hidden])
Date: 2007-04-14 08:28:37

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.