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

From: Joe Gottman (jgottman_at_[hidden])
Date: 2007-04-14 19:33:26

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. 
> 

    I disagree.  Suppose we have a function foo(shared_ptr<int> &&p) 
that might or might not move from p.  For instance, foo might be one of 
several potential consumers of the shared_ptr.  In this case, having the 
move constructor automatically zero out p would be a nice way to let the 
client know that p was actually moved from. It would enable code like 
the following:

shared_ptr<int> p(new int(1));
foo(move(p));
if (p) { // foo didn't want it, try the next potential consumer
     bar(move(p));
} else {
     return; // foo consumed our shared_ptr, so we are done.
}

Also, leaving the shared_ptr in an inconsistent state can cause 
intermittent crashes far away from where the move occurred.  This is 
unfair to the person who will be debugging the crash, who may well be 
someone different from the person who wrote the code with the move 
statement.

Joe Gottman